Bug 1395017. P3 - always dispatch a task to run UpdatePrincipal() even when CacheClientUpdatePrincipal() already runs in the main thread. draft
authorJW Wang <jwwang@mozilla.com>
Wed, 30 Aug 2017 11:42:25 +0800
changeset 659704 8d0be483ab3f5c8a53d86728810396ab7b6f6301
parent 659451 77d28757d694deb23c20c82a6b52afac4586e48e
child 659705 22b362b56f218acb70cc7ce3c0fd0ea113563681
push id78176
push userjwwang@mozilla.com
push dateWed, 06 Sep 2017 07:18:39 +0000
bugs1395017
milestone57.0a1
Bug 1395017. P3 - always dispatch a task to run UpdatePrincipal() even when CacheClientUpdatePrincipal() already runs in the main thread. When MediaCacheStream::NotifyDataReceived() runs off the main thread, there is no guarantee that the principal will be updated before the new data is observable to the consumer because the principal can only be updated on the main thread while the consumer can access the data off the main thread. To make the code simpler, we always dispatch a task to run UpdatePrincipal() even when CacheClientUpdatePrincipal() already runs in the main thread. This also avoid the deadlock because ChannelMediaResource::UpdatePrincipal() will never run with the cache monitor held. MozReview-Commit-ID: 9CdrJnaV0hl
dom/media/MediaCache.cpp
dom/media/MediaResource.cpp
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -1879,29 +1879,26 @@ MediaCacheStream::UpdatePrincipal(nsIPri
 }
 
 void
 MediaCacheStream::NotifyDataReceived(int64_t aSize, const char* aData)
 {
   // 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.
+  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   {
+    // Need to acquire the monitor because this code might run
+    // off the main thread.
     MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
     while (MediaCacheStream* stream = iter.Next()) {
       stream->mClient->CacheClientUpdatePrincipal();
     }
   }
 
-  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   int64_t size = aSize;
   const char* data = aData;
 
   LOG("Stream %p DataReceived at %" PRId64 " count=%" PRId64,
       this, mChannelOffset, aSize);
 
   // We process the data one block (or part of a block) at a time
   while (size > 0) {
--- a/dom/media/MediaResource.cpp
+++ b/dom/media/MediaResource.cpp
@@ -884,22 +884,16 @@ ChannelMediaResource::UpdatePrincipal()
   if (mCacheStream.UpdatePrincipal(principal)) {
     mCallback->NotifyPrincipalChanged();
   }
 }
 
 void
 ChannelMediaResource::CacheClientUpdatePrincipal()
 {
-  // This might happen off the main thread.
-  if (NS_IsMainThread()) {
-    UpdatePrincipal();
-    return;
-  }
-
   SystemGroup::Dispatch(
     TaskCategory::Other,
     NewRunnableMethod("ChannelMediaResource::UpdatePrincipal",
                       this,
                       &ChannelMediaResource::UpdatePrincipal));
 }
 
 void