Bug 1394724. P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived(). draft
authorJW Wang <jwwang@mozilla.com>
Tue, 29 Aug 2017 16:41:17 +0800
changeset 656338 6fe18257507f2f939fa24d03e69e1120ae1c0edb
parent 656337 cb67004d0d50446104aa33819339693366d08c4c
child 656339 f4824bc31723ee2dc3bd0c2405aeee7b40680d54
push id77176
push userjwwang@mozilla.com
push dateThu, 31 Aug 2017 02:42:53 +0000
bugs1394724
milestone57.0a1
Bug 1394724. P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived(). 1. mCacheStream.Close() should happen after CloseChannel() to avoid data race in reading mClosed in MediaCacheStream::NotifyDataReceived(). 2. MediaCache::Update() and CloseStreamsForPrivateBrowsing() should call ChannelMediaResource::Close() to ensure mCacheStream.Close() happens after CloseChannel(). MozReview-Commit-ID: 1o3yPbm3Gy6
dom/media/MediaCache.cpp
dom/media/MediaResource.cpp
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -670,17 +670,17 @@ MediaCache::Flush()
 }
 
 void
 MediaCache::CloseStreamsForPrivateBrowsing()
 {
   MOZ_ASSERT(NS_IsMainThread());
   for (MediaCacheStream* s : mStreams) {
     if (s->mIsPrivateBrowsing) {
-      s->Close();
+      s->mClient->Close();
     }
   }
 }
 
 /* static */ RefPtr<MediaCache>
 MediaCache::GetMediaCache(int64_t aContentLength)
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
@@ -1454,18 +1454,17 @@ MediaCache::Update()
       rv = NS_OK;
       break;
     }
 
     if (NS_FAILED(rv)) {
       // Close the streams that failed due to error. This will cause all
       // client Read and Seek operations on those streams to fail. Blocked
       // Reads will also be woken up.
-      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
-      stream->CloseInternal(mon);
+      stream->mClient->Close();
     }
   }
 
   // Notify streams about the suspended status changes.
   for (uint32_t i = 0; i < mSuspendedStatusToNotify.Length(); ++i) {
     MediaCache::ResourceStreamIterator iter(this, mSuspendedStatusToNotify[i]);
     while (MediaCacheStream* stream = iter.Next()) {
       stream->mClient->CacheClientNotifySuspendedStatusChanged();
@@ -1875,21 +1874,18 @@ MediaCacheStream::UpdatePrincipal(nsIPri
 {
   return nsContentUtils::CombineResourcePrincipals(&mPrincipal, aPrincipal);
 }
 
 void
 MediaCacheStream::NotifyDataReceived(int64_t aSize, const char* aData,
     nsIPrincipal* aPrincipal)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
-
-  if (mClosed) {
-    return;
-  }
+  // This might happen off the main thread.
+  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.
   {
     MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
--- a/dom/media/MediaResource.cpp
+++ b/dom/media/MediaResource.cpp
@@ -627,18 +627,18 @@ nsresult ChannelMediaResource::SetupChan
   }
   return NS_OK;
 }
 
 nsresult ChannelMediaResource::Close()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
+  CloseChannel();
   mCacheStream.Close();
-  CloseChannel();
   return NS_OK;
 }
 
 already_AddRefed<nsIPrincipal> ChannelMediaResource::GetCurrentPrincipal()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   nsCOMPtr<nsIPrincipal> principal = mCacheStream.GetCurrentPrincipal();