Bug 1411808. P2 - don't call mClient->IsSuspended() off the main thread in Update(). r=gerald
authorJW Wang <jwwang@mozilla.com>
Tue, 24 Oct 2017 11:25:41 +0800
changeset 443224 875d91b29c311400cc3108f959da53d09bab66b3
parent 443223 f26682779a1841d5ff52797e9693dee02d8fbade
child 443225 efbfeacfdb9621ab2c67b2400cb89d92669b6c53
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald
bugs1411808
milestone58.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 1411808. P2 - don't call mClient->IsSuspended() off the main thread in Update(). r=gerald 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
@@ -1425,17 +1425,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",
@@ -2127,16 +2127,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);
@@ -2654,16 +2672,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