Bug 1398711. P2 - write to mClosed only when the cache monitor is held. r=gerald
authorJW Wang <jwwang@mozilla.com>
Fri, 08 Sep 2017 17:46:56 +0800
changeset 429988 b420bb292e68194ad0ec7a956a2ff6e8887113f1
parent 429987 84abfad4ada97255d6e1e4dbbb44029b1600eca0
child 429989 8a1c6347f7955a05e4808c6c9cbf8a416015361a
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald
bugs1398711
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1398711. P2 - write to mClosed only when the cache monitor is held. r=gerald 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