Bug 1426578. P1 - tweak the constructor and init functions of ChannelMediaResource/MediaCacheStream. draft
authorJW Wang <jwwang@mozilla.com>
Thu, 14 Dec 2017 16:08:17 +0800
changeset 713856 13667c7100ae1404c207fbed43f1e573bb8ac96d
parent 713743 62dd5404cf55e29412d5fff8fe9105076b1ca437
child 713857 1a2469069657433d4752c6f88a32d5df74675d03
push id93781
push userjwwang@mozilla.com
push dateThu, 21 Dec 2017 03:28:43 +0000
bugs1426578
milestone59.0a1
Bug 1426578. P1 - tweak the constructor and init functions of ChannelMediaResource/MediaCacheStream. We want to init as many members as possible before taking the cache monitor. This makes it easier to move part of the init functions to another thread. MozReview-Commit-ID: 6mmO356nCyQ
dom/media/ChannelMediaResource.cpp
dom/media/ChannelMediaResource.h
dom/media/MediaCache.cpp
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -25,17 +25,17 @@ mozilla::LazyLogModule gMediaResourceLog
 namespace mozilla {
 
 ChannelMediaResource::ChannelMediaResource(MediaResourceCallback* aCallback,
                                            nsIChannel* aChannel,
                                            nsIURI* aURI,
                                            bool aIsPrivateBrowsing)
   : BaseMediaResource(aCallback, aChannel, aURI)
   , mCacheStream(this, aIsPrivateBrowsing)
-  , mSuspendAgent(mCacheStream)
+  , mSuspendAgent(mCacheStream, !aChannel /*aSuspended*/)
 {
 }
 
 ChannelMediaResource::~ChannelMediaResource()
 {
   MOZ_ASSERT(mClosed);
   MOZ_ASSERT(!mChannel);
   MOZ_ASSERT(!mListener);
@@ -580,20 +580,16 @@ ChannelMediaResource::CloneData(MediaRes
   // 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.
   nsresult rv = resource->mCacheStream.InitAsClone(&mCacheStream);
   if (NS_FAILED(rv)) {
     resource->Close();
     return nullptr;
   }
-  // mSuspendAgent.Suspend() accesses mCacheStream which is not ready
-  // until InitAsClone() is done.
-  resource->mSuspendAgent.Suspend();
-
   return resource.forget();
 }
 
 void ChannelMediaResource::CloseChannel()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   if (mChannel) {
--- a/dom/media/ChannelMediaResource.h
+++ b/dom/media/ChannelMediaResource.h
@@ -18,18 +18,19 @@ namespace mozilla {
 
 /**
  * This class is responsible for managing the suspend count and report suspend
  * status of channel.
  **/
 class ChannelSuspendAgent
 {
 public:
-  explicit ChannelSuspendAgent(MediaCacheStream& aCacheStream)
+  ChannelSuspendAgent(MediaCacheStream& aCacheStream, bool aSuspended)
     : mCacheStream(aCacheStream)
+    , mSuspendCount(aSuspended ? 1 : 0)
   {
   }
 
   // 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.
@@ -44,17 +45,17 @@ public:
   void Revoke();
 
 private:
   // Only suspends channel but not changes the suspend count.
   void SuspendInternal();
 
   nsIChannel* mChannel = nullptr;
   MediaCacheStream& mCacheStream;
-  uint32_t mSuspendCount = 0;
+  uint32_t mSuspendCount;
   bool mIsChannelSuspended = false;
 };
 
 DDLoggedTypeDeclNameAndBase(ChannelMediaResource, BaseMediaResource);
 
 /**
  * This is the MediaResource implementation that wraps Necko channels.
  * Much of its functionality is actually delegated to MediaCache via
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -2871,43 +2871,41 @@ MediaCacheStream::Init(int64_t aContentL
 }
 
 nsresult
 MediaCacheStream::InitAsClone(MediaCacheStream* aOriginal)
 {
   MOZ_ASSERT(!mMediaCache, "Has been initialized.");
   MOZ_ASSERT(aOriginal->mMediaCache, "Don't clone an uninitialized stream.");
 
+  // Use the same MediaCache as our clone.
+  mMediaCache = aOriginal->mMediaCache;
+  // This needs to be done before OpenStream() to avoid data race.
+  mClientSuspended = true;
+  // Cloned streams are initially suspended, since there is no channel open
+  // initially for a clone.
+  mCacheSuspended = true;
+  mChannelEnded = true;
+
   AutoLock lock(aOriginal->mMediaCache->Monitor());
 
   if (aOriginal->mDidNotifyDataEnded &&
       NS_FAILED(aOriginal->mNotifyDataEndedStatus)) {
     // Streams that ended abnormally are ineligible for cloning.
     return NS_ERROR_FAILURE;
   }
 
-  // This needs to be done before OpenStream() to avoid data race.
-  mClientSuspended = true;
-
-  // Use the same MediaCache as our clone.
-  mMediaCache = aOriginal->mMediaCache;
-
   mResourceID = aOriginal->mResourceID;
 
   // Grab cache blocks from aOriginal as readahead blocks for our stream
   mStreamLength = aOriginal->mStreamLength;
   mIsTransportSeekable = aOriginal->mIsTransportSeekable;
   mDownloadStatistics = aOriginal->mDownloadStatistics;
   mDownloadStatistics.Stop();
 
-  // Cloned streams are initially suspended, since there is no channel open
-  // initially for a clone.
-  mCacheSuspended = true;
-  mChannelEnded = true;
-
   if (aOriginal->mDidNotifyDataEnded) {
     mNotifyDataEndedStatus = aOriginal->mNotifyDataEndedStatus;
     mDidNotifyDataEnded = true;
     mClient->CacheClientNotifyDataEnded(mNotifyDataEndedStatus);
   }
 
   for (uint32_t i = 0; i < aOriginal->mBlocks.Length(); ++i) {
     int32_t cacheBlockIndex = aOriginal->mBlocks[i];