Bug 1398711. P2 - write to mClosed only when the cache monitor is held. draft
authorJW Wang <jwwang@mozilla.com>
Fri, 08 Sep 2017 17:46:56 +0800
changeset 663473 7d7ebd3ff8ea6748c7577a76c5e452d79a063670
parent 663472 2a3dd0d96111bf7d43cd52c2f52c6dc165b9649b
child 663494 5a0fbb3a79e8136e2fd03e5017562a96a914bf5d
push id79458
push userjwwang@mozilla.com
push dateWed, 13 Sep 2017 04:44:02 +0000
bugs1398711
milestone57.0a1
Bug 1398711. P2 - write to mClosed only when the cache monitor is held. This fixes the data race when Seek() read mClosed off the main thread. MozReview-Commit-ID: GO7Kk5VgVpg
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -445,17 +445,16 @@ MediaCacheFlusher::Observe(nsISupports *
   }
   return NS_OK;
 }
 
 MediaCacheStream::MediaCacheStream(ChannelMediaResource* aClient,
                                    bool aIsPrivateBrowsing)
   : mMediaCache(nullptr)
   , mClient(aClient)
-  , mClosed(false)
   , mDidNotifyDataEnded(false)
   , mResourceID(0)
   , mIsTransportSeekable(false)
   , mCacheSuspended(false)
   , mChannelEnded(false)
   , mChannelOffset(0)
   , mStreamLength(-1)
   , mStreamOffset(0)
@@ -1873,16 +1872,19 @@ MediaCacheStream::UpdatePrincipal(nsIPri
   return nsContentUtils::CombineResourcePrincipals(&mPrincipal, aPrincipal);
 }
 
 void
 MediaCacheStream::NotifyDataReceived(int64_t aSize, const char* aData,
     nsIPrincipal* aPrincipal)
 {
   // This might happen off the main thread.
+
+  // It is safe to read mClosed without holding the monitor because this
+  // function is guaranteed to happen before Close().
   MOZ_DIAGNOSTIC_ASSERT(!mClosed);
 
   // Update principals before putting the data in the cache. This is important,
   // we want to make sure all principals are updated before any consumer
   // can see the new data.
   // We do this without holding the cache monitor, in case the client wants
   // to do something that takes a lock.
   {
@@ -2105,24 +2107,23 @@ void
 MediaCacheStream::Close()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   if (!mMediaCache || mClosed) {
     return;
   }
 
-  mClosed = true;
-
   // Closing a stream will change the return value of
   // MediaCacheStream::AreAllStreamsForResourceSuspended as well as
   // ChannelMediaResource::IsSuspendedByCache. Let's notify it.
   mMediaCache->QueueSuspendedStatusUpdate(mResourceID);
 
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
+  mClosed = true;
   mMediaCache->ReleaseStreamBlocks(this);
   // Wake up any blocked readers
   mon.NotifyAll();
 
   // Queue an Update since we may have created more free space. Don't do
   // it from CloseInternal since that gets called by Update() itself
   // sometimes, and we try to not to queue updates from Update().
   mMediaCache->QueueUpdate();
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -216,19 +216,21 @@ public:
   // be called while the stream is at channel offset 0. Seekability can
   // change during the lifetime of the MediaCacheStream --- every time
   // we do an HTTP load the seekability may be different (and sometimes
   // is, in practice, due to the effects of caching proxies).
   void SetTransportSeekable(bool aIsTransportSeekable);
   // This must be called (and return) before the ChannelMediaResource
   // used to create this MediaCacheStream is deleted.
   void Close();
-  // This returns true when the stream has been closed
+  // This returns true when the stream has been closed.
+  // Must be used on the main thread or while holding the cache lock.
   bool IsClosed() const { return mClosed; }
-  // Returns true when this stream is can be shared by a new resource load
+  // Returns true when this stream is can be shared by a new resource load.
+  // Called on the main thread only.
   bool IsAvailableForSharing() const
   {
     return !mClosed && !mIsPrivateBrowsing &&
       (!mDidNotifyDataEnded || NS_SUCCEEDED(mNotifyDataEndedStatus));
   }
   // Get the principal for this stream. Anything accessing the contents of
   // this stream must have a principal that subsumes this principal.
   nsIPrincipal* GetCurrentPrincipal() { return mPrincipal; }
@@ -431,26 +433,26 @@ private:
   bool UpdatePrincipal(nsIPrincipal* aPrincipal);
 
   // Instance of MediaCache to use with this MediaCacheStream.
   RefPtr<MediaCache> mMediaCache;
 
   // These fields are main-thread-only.
   ChannelMediaResource*  mClient;
   nsCOMPtr<nsIPrincipal> mPrincipal;
-  // Set to true when the stream has been closed either explicitly or
-  // due to an internal cache error
-  bool                   mClosed;
   // True if CacheClientNotifyDataEnded has been called for this stream.
   bool                   mDidNotifyDataEnded;
 
   // The following fields must be written holding the cache's monitor and
   // only on the main thread, thus can be read either on the main thread
   // or while holding the cache's monitor.
 
+  // Set to true when the stream has been closed either explicitly or
+  // due to an internal cache error
+  bool mClosed = false;
   // This is a unique ID representing the resource we're loading.
   // All streams with the same mResourceID are loading the same
   // underlying resource and should share data.
   int64_t mResourceID;
   // The last reported seekability state for the underlying channel
   bool mIsTransportSeekable;
   // True if the cache has suspended our channel because the cache is
   // full and the priority of the data that would be received is lower