Bug 1411808. P2 - don't call mClient->IsSuspended() off the main thread in Update(). draft
authorJW Wang <jwwang@mozilla.com>
Tue, 24 Oct 2017 11:25:41 +0800
changeset 692421 6511455588e27909cde4469483c14ade4b50e693
parent 692420 44695a45657cbf55732fbd4054b3f45ed254f0d9
child 692422 c7ed0e632108b76f5a915ceb620036013d94ed56
push id87497
push userjwwang@mozilla.com
push dateFri, 03 Nov 2017 03:36:40 +0000
bugs1411808
milestone58.0a1
Bug 1411808. P2 - don't call mClient->IsSuspended() off the main thread in Update(). By mirroring the suspend status of the client, Update() is able to make decisions on reading streams without calling mClient->IsSuspended(). MozReview-Commit-ID: G4gS2VGiMjj
dom/media/ChannelMediaResource.cpp
dom/media/ChannelMediaResource.h
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -26,30 +26,30 @@ namespace mozilla {
 
 ChannelMediaResource::ChannelMediaResource(MediaResourceCallback* aCallback,
                                            nsIChannel* aChannel,
                                            nsIURI* aURI,
                                            bool aIsPrivateBrowsing)
   : BaseMediaResource(aCallback, aChannel, aURI)
   , mReopenOnError(false)
   , mCacheStream(this, aIsPrivateBrowsing)
-  , mSuspendAgent(mChannel)
+  , mSuspendAgent(mChannel, mCacheStream)
 {
 }
 
 ChannelMediaResource::ChannelMediaResource(
   MediaResourceCallback* aCallback,
   nsIChannel* aChannel,
   nsIURI* aURI,
   const MediaChannelStatistics& aStatistics)
   : BaseMediaResource(aCallback, aChannel, aURI)
   , mReopenOnError(false)
   , mCacheStream(this, /* aIsPrivateBrowsing = */ false)
   , mChannelStatistics(aStatistics)
-  , mSuspendAgent(mChannel)
+  , mSuspendAgent(mChannel, mCacheStream)
 {
 }
 
 ChannelMediaResource::~ChannelMediaResource()
 {
   MOZ_ASSERT(mClosed);
   MOZ_ASSERT(!mChannel);
   MOZ_ASSERT(!mListener);
@@ -599,18 +599,20 @@ ChannelMediaResource::CloneData(MediaRes
     new ChannelMediaResource(aCallback, nullptr, mURI, mChannelStatistics);
 
   // Initially the clone is treated as suspended by the cache, because
   // we don't have a channel. If the cache needs to read data from the clone
   // it will call CacheClientResume (or CacheClientSeek with aResume true)
   // which will recreate the channel. This way, if all of the media data
   // is already in the cache we don't create an unnecessary HTTP channel
   // and perform a useless HTTP transaction.
+  resource->mCacheStream.InitAsClone(&mCacheStream);
+  // mSuspendAgent.Suspend() accesses mCacheStream which is not ready
+  // until InitAsClone() is done.
   resource->mSuspendAgent.Suspend();
-  resource->mCacheStream.InitAsClone(&mCacheStream);
   resource->mChannelStatistics.Stop();
 
   return resource.forget();
 }
 
 void ChannelMediaResource::CloseChannel()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
@@ -1015,17 +1017,21 @@ ChannelMediaResource::GetDebugInfo()
 
 // ChannelSuspendAgent
 
 bool
 ChannelSuspendAgent::Suspend()
 {
   MOZ_ASSERT(NS_IsMainThread());
   SuspendInternal();
-  return (++mSuspendCount == 1);
+  if (++mSuspendCount == 1) {
+    mCacheStream.NotifyClientSuspended(true);
+    return true;
+  }
+  return false;
 }
 
 void
 ChannelSuspendAgent::SuspendInternal()
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (mChannel) {
     bool isPending = false;
@@ -1037,23 +1043,23 @@ ChannelSuspendAgent::SuspendInternal()
   }
 }
 
 bool
 ChannelSuspendAgent::Resume()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(IsSuspended(), "Resume without suspend!");
-  --mSuspendCount;
 
-  if (mSuspendCount == 0) {
+  if (--mSuspendCount == 0) {
     if (mChannel && mIsChannelSuspended) {
       mChannel->Resume();
       mIsChannelSuspended = false;
     }
+    mCacheStream.NotifyClientSuspended(false);
     return true;
   }
   return false;
 }
 
 void
 ChannelSuspendAgent::UpdateSuspendedStatusIfNeeded()
 {
--- a/dom/media/ChannelMediaResource.h
+++ b/dom/media/ChannelMediaResource.h
@@ -19,19 +19,19 @@ namespace mozilla {
 
 /**
  * This class is responsible for managing the suspend count and report suspend
  * status of channel.
  **/
 class ChannelSuspendAgent
 {
 public:
-  explicit ChannelSuspendAgent(nsIChannel* aChannel)
+  ChannelSuspendAgent(nsIChannel* aChannel, MediaCacheStream& aCacheStream)
     : mChannel(aChannel)
-    , mIsChannelSuspended(false)
+    , mCacheStream(aCacheStream)
   {
   }
 
   // True when the channel has been suspended or needs to be suspended.
   bool IsSuspended();
 
   // Return true when the channel is logically suspended, i.e. the suspend
   // count goes from 0 to 1.
@@ -50,18 +50,19 @@ public:
   // Check whether we need to suspend the channel.
   void UpdateSuspendedStatusIfNeeded();
 
 private:
   // Only suspends channel but not changes the suspend count.
   void SuspendInternal();
 
   nsIChannel* mChannel;
+  MediaCacheStream& mCacheStream;
   uint32_t mSuspendCount = 0;
-  bool mIsChannelSuspended;
+  bool mIsChannelSuspended = false;
 };
 
 /**
  * This is the MediaResource implementation that wraps Necko channels.
  * Much of its functionality is actually delegated to MediaCache via
  * an underlying MediaCacheStream.
  *
  * All synchronization is performed by MediaCacheStream; all off-main-
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -1442,17 +1442,17 @@ MediaCache::Update()
         enableReading = predictedNewDataUse < latestNextUse;
       }
     }
 
     if (enableReading) {
       for (uint32_t j = 0; j < i; ++j) {
         MediaCacheStream* other = mStreams[j];
         if (other->mResourceID == stream->mResourceID && !other->mClosed &&
-            !other->mClient->IsSuspended() &&
+            !other->mClientSuspended &&
             OffsetToBlockIndexUnchecked(other->mSeekTarget != -1
                                           ? other->mSeekTarget
                                           : other->mChannelOffset) ==
               OffsetToBlockIndexUnchecked(desiredOffset)) {
           // This block is already going to be read by the other stream.
           // So don't try to read it from this stream as well.
           enableReading = false;
           LOG("Stream %p waiting on same block (%" PRId32 ") from stream %p",
@@ -2144,16 +2144,34 @@ void
 MediaCacheStream::NotifyChannelRecreated()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   mChannelEnded = false;
   mDidNotifyDataEnded = false;
 }
 
+void
+MediaCacheStream::NotifyClientSuspended(bool aSuspended)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  RefPtr<ChannelMediaResource> client = mClient;
+  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+    "MediaCacheStream::NotifyClientSuspended", [client, this, aSuspended]() {
+      if (mClientSuspended != aSuspended) {
+        mClientSuspended = aSuspended;
+        // mClientSuspended changes the decision of reading streams.
+        ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
+        mMediaCache->QueueUpdate();
+      }
+    });
+  OwnerThread()->Dispatch(r.forget());
+}
+
 MediaCacheStream::~MediaCacheStream()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   NS_ASSERTION(!mPinCount, "Unbalanced Pin");
 
   if (mMediaCache) {
     NS_ASSERTION(mClosed, "Stream was not closed");
     mMediaCache->ReleaseStream(this);
@@ -2671,16 +2689,19 @@ MediaCacheStream::Init(int64_t aContentL
 
 void
 MediaCacheStream::InitAsClone(MediaCacheStream* aOriginal)
 {
   MOZ_ASSERT(aOriginal->IsAvailableForSharing());
   MOZ_ASSERT(!mMediaCache, "Has been initialized.");
   MOZ_ASSERT(aOriginal->mMediaCache, "Don't clone an uninitialized stream.");
 
+  // This needs to be done before OpenStream() to avoid data race.
+  mClientSuspended = true;
+
   // Use the same MediaCache as our clone.
   mMediaCache = aOriginal->mMediaCache;
 
   mMediaCache->OpenStream(this);
 
   mResourceID = aOriginal->mResourceID;
 
   // Grab cache blocks from aOriginal as readahead blocks for our stream
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -270,16 +270,20 @@ public:
   void FlushPartialBlock();
   // Notifies the cache that the channel has closed with the given status.
   void NotifyDataEnded(nsresult aStatus);
 
   // Notifies the stream that the channel is reopened. The stream should
   // reset variables such as |mDidNotifyDataEnded|.
   void NotifyChannelRecreated();
 
+  // Notifies the stream that the suspend status of the client has changed.
+  // Main thread only.
+  void NotifyClientSuspended(bool aSuspended);
+
   // These methods can be called on any thread.
   // Cached blocks associated with this stream will not be evicted
   // while the stream is pinned.
   void Pin();
   void Unpin();
   // See comments above for NotifyDataLength about how the length
   // can vary over time. Returns -1 if no length is known. Returns the
   // reported length if we haven't got any better information. If
@@ -513,13 +517,16 @@ private:
   // Heap allocate this buffer since the exact power-of-2 will cause allocation
   // slop when combined with the rest of the object members.
   // This partial buffer should always be read/write within the cache's monitor.
   const UniquePtr<uint8_t[]> mPartialBlockBuffer =
     MakeUnique<uint8_t[]>(BLOCK_SIZE);
 
   // True if associated with a private browsing window.
   const bool mIsPrivateBrowsing;
+
+  // True if the client is suspended. Accessed on the owner thread only.
+  bool mClientSuspended = false;
 };
 
 } // namespace mozilla
 
 #endif