Bug 1426061. P1 - always access MediaCacheStream::mClosed while holding the lock. r=bechen,gerald
authorJW Wang <jwwang@mozilla.com>
Thu, 14 Dec 2017 16:22:53 +0800
changeset 448737 26dcfbdd90803904986d82eebb70ec85d09d0ff9
parent 448736 c7a72db9069bbce81ff1bcc7a3b719efad255829
child 448738 f58dca05f26f69116bf5a736d384e97963beec61
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbechen, gerald
bugs1426061
milestone59.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 1426061. P1 - always access MediaCacheStream::mClosed while holding the lock. r=bechen,gerald We will offload MediaCacheStream::Close() to another thread and need to access mClosed off the main thread. MozReview-Commit-ID: 891rzC8dOON
dom/media/ChannelMediaResource.cpp
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -552,24 +552,24 @@ already_AddRefed<nsIPrincipal>
 ChannelMediaResource::GetCurrentPrincipal()
 {
   MOZ_ASSERT(NS_IsMainThread());
   return do_AddRef(mSharedInfo->mPrincipal);
 }
 
 bool ChannelMediaResource::CanClone()
 {
-  return mCacheStream.IsAvailableForSharing();
+  return !mClosed && mCacheStream.IsAvailableForSharing();
 }
 
 already_AddRefed<BaseMediaResource>
 ChannelMediaResource::CloneData(MediaResourceCallback* aCallback)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
-  NS_ASSERTION(mCacheStream.IsAvailableForSharing(), "Stream can't be cloned");
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(CanClone(), "Stream can't be cloned");
 
   RefPtr<ChannelMediaResource> resource =
     new ChannelMediaResource(aCallback, nullptr, mURI);
 
   resource->mIsLiveStream = mIsLiveStream;
   resource->mIsTransportSeekable = mIsTransportSeekable;
   resource->mSharedInfo = mSharedInfo;
   mSharedInfo->mResources.AppendElement(resource.get());
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -259,22 +259,22 @@ public:
   {
   public:
     ResourceStreamIterator(MediaCache* aMediaCache, int64_t aResourceID)
       : mMediaCache(aMediaCache)
       , mResourceID(aResourceID)
       , mNext(0)
     {
     }
-    MediaCacheStream* Next()
+    MediaCacheStream* Next(AutoLock& aLock)
     {
       while (mNext < mMediaCache->mStreams.Length()) {
         MediaCacheStream* stream = mMediaCache->mStreams[mNext];
         ++mNext;
-        if (stream->GetResourceID() == mResourceID && !stream->IsClosed())
+        if (stream->GetResourceID() == mResourceID && !stream->IsClosed(aLock))
           return stream;
       }
       return nullptr;
     }
   private:
     MediaCache* mMediaCache;
     int64_t  mResourceID;
     uint32_t mNext;
@@ -1573,17 +1573,17 @@ MediaCache::Update()
       default:
         break;
     }
   }
 
   // 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()) {
+    while (MediaCacheStream* stream = iter.Next(lock)) {
       stream->mClient->CacheClientNotifySuspendedStatusChanged(
         stream->AreAllStreamsForResourceSuspended(lock));
     }
   }
   mSuspendedStatusToNotify.Clear();
 }
 
 class UpdateEvent : public Runnable
@@ -1696,17 +1696,17 @@ MediaCache::AllocateAndWriteBlock(AutoLo
                                   MediaCacheStream::ReadMode aMode,
                                   Span<const uint8_t> aData1,
                                   Span<const uint8_t> aData2)
 {
   MOZ_ASSERT(sThread->IsOnCurrentThread());
 
   // Remove all cached copies of this block
   ResourceStreamIterator iter(this, aStream->mResourceID);
-  while (MediaCacheStream* stream = iter.Next()) {
+  while (MediaCacheStream* stream = iter.Next(aLock)) {
     while (aStreamBlockIndex >= int32_t(stream->mBlocks.Length())) {
       stream->mBlocks.AppendElement(-1);
     }
     if (stream->mBlocks[aStreamBlockIndex] >= 0) {
       // We no longer want to own this block
       int32_t globalBlockIndex = stream->mBlocks[aStreamBlockIndex];
       LOG("Released block %d from stream %p block %d(%" PRId64 ")",
           globalBlockIndex,
@@ -1728,17 +1728,17 @@ MediaCache::AllocateAndWriteBlock(AutoLo
     Block* block = &mIndex[blockIndex];
     LOG("Allocated block %d to stream %p block %d(%" PRId64 ")",
         blockIndex,
         aStream,
         aStreamBlockIndex,
         aStreamBlockIndex * BLOCK_SIZE);
 
     ResourceStreamIterator iter(this, aStream->mResourceID);
-    while (MediaCacheStream* stream = iter.Next()) {
+    while (MediaCacheStream* stream = iter.Next(aLock)) {
       BlockOwner* bo = block->mOwners.AppendElement();
       if (!bo) {
         // Roll back mOwners if any allocation fails.
         block->mOwners.Clear();
         return;
       }
       mBlockOwnersWatermark =
         std::max(mBlockOwnersWatermark, uint32_t(block->mOwners.Length()));
@@ -2116,17 +2116,17 @@ MediaCacheStream::NotifyDataReceived(uin
                                    remaining);
       memcpy(buf.Elements(), source.Elements(), source.Length());
       mChannelOffset += source.Length();
       break;
     }
   }
 
   MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
-  while (MediaCacheStream* stream = iter.Next()) {
+  while (MediaCacheStream* stream = iter.Next(lock)) {
     if (stream->mStreamLength >= 0) {
       // The stream is at least as long as what we've read
       stream->mStreamLength = std::max(stream->mStreamLength, mChannelOffset);
     }
     stream->mClient->CacheClientNotifyDataReceived();
   }
 
   // Notify in case there's a waiting reader
@@ -2248,17 +2248,17 @@ MediaCacheStream::NotifyDataEndedInterna
     return;
   }
 
   // Note we don't flush the partial block when download ends abnormally for
   // the padding zeros will give wrong data to other streams.
   FlushPartialBlockInternal(lock, true);
 
   MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
-  while (MediaCacheStream* stream = iter.Next()) {
+  while (MediaCacheStream* stream = iter.Next(lock)) {
     // We read the whole stream, so remember the true length
     stream->mStreamLength = mChannelOffset;
     if (!stream->mDidNotifyDataEnded) {
       stream->mDidNotifyDataEnded = true;
       stream->mNotifyDataEndedStatus = aStatus;
       stream->mClient->CacheClientNotifyDataEnded(aStatus);
     }
   }
@@ -2348,17 +2348,17 @@ MediaCacheStream::~MediaCacheStream()
 bool
 MediaCacheStream::AreAllStreamsForResourceSuspended(AutoLock& aLock)
 {
   MOZ_ASSERT(!NS_IsMainThread());
 
   MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
   // Look for a stream that's able to read the data we need
   int64_t dataOffset = -1;
-  while (MediaCacheStream* stream = iter.Next()) {
+  while (MediaCacheStream* stream = iter.Next(aLock)) {
     if (stream->mCacheSuspended || stream->mChannelEnded || stream->mClosed) {
       continue;
     }
     if (dataOffset < 0) {
       dataOffset = GetCachedDataEndInternal(aLock, mStreamOffset);
     }
     // Ignore streams that are reading beyond the data we need
     if (stream->mChannelOffset > dataOffset) {
@@ -2368,23 +2368,25 @@ MediaCacheStream::AreAllStreamsForResour
   }
 
   return true;
 }
 
 void
 MediaCacheStream::Close()
 {
-  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
-
-  if (!mMediaCache || mClosed) {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!mMediaCache) {
     return;
   }
 
   AutoLock lock(mMediaCache->Monitor());
+  if (mClosed) {
+    return;
+  }
 
   // Closing a stream will change the return value of
   // MediaCacheStream::AreAllStreamsForResourceSuspended as well as
   // ChannelMediaResource::IsSuspendedByCache. Let's notify it.
   mMediaCache->QueueSuspendedStatusUpdate(lock, mResourceID);
 
   mClosed = true;
   mMediaCache->ReleaseStreamBlocks(lock, this);
@@ -2711,17 +2713,17 @@ MediaCacheStream::Read(char* aBuffer, ui
       continue;
     }
 
     // See if we can use the data in the partial block of any stream reading
     // this resource. Note we use the partial block only when it is completed,
     // that is reaching EOS.
     bool foundDataInPartialBlock = false;
     MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
-    while (MediaCacheStream* stream = iter.Next()) {
+    while (MediaCacheStream* stream = iter.Next(lock)) {
       if (OffsetToBlockIndexUnchecked(stream->mChannelOffset) ==
             OffsetToBlockIndexUnchecked(streamOffset) &&
           stream->mChannelOffset == stream->mStreamLength) {
         uint32_t bytes = stream->ReadPartialBlock(lock, streamOffset, buffer);
         streamOffset += bytes;
         buffer = buffer.From(bytes);
         foundDataInPartialBlock = true;
         break;
@@ -2857,17 +2859,16 @@ MediaCacheStream::Init(int64_t aContentL
   AutoLock lock(mMediaCache->Monitor());
   mMediaCache->OpenStream(lock, this);
   return NS_OK;
 }
 
 nsresult
 MediaCacheStream::InitAsClone(MediaCacheStream* aOriginal)
 {
-  MOZ_ASSERT(aOriginal->IsAvailableForSharing());
   MOZ_ASSERT(!mMediaCache, "Has been initialized.");
   MOZ_ASSERT(aOriginal->mMediaCache, "Don't clone an uninitialized stream.");
 
   AutoLock lock(aOriginal->mMediaCache->Monitor());
 
   if (aOriginal->mDidNotifyDataEnded &&
       NS_FAILED(aOriginal->mNotifyDataEndedStatus)) {
     // Streams that ended abnormally are ineligible for cloning.
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -222,21 +222,20 @@ public:
 
   nsIEventTarget* OwnerThread() const;
 
   // These are called on the main thread.
   // This must be called (and return) before the ChannelMediaResource
   // used to create this MediaCacheStream is deleted.
   void Close();
   // This returns true when the stream has been closed.
-  // Must be used on the main thread or while holding the cache lock.
-  bool IsClosed() const { return mClosed; }
+  bool IsClosed(AutoLock&) const { return mClosed; }
   // Returns true when this stream is can be shared by a new resource load.
   // Called on the main thread only.
-  bool IsAvailableForSharing() const { return !mClosed && !mIsPrivateBrowsing; }
+  bool IsAvailableForSharing() const { return !mIsPrivateBrowsing; }
 
   // These callbacks are called on the main thread by the client
   // when data has been received via the channel.
 
   // Notifies the cache that a load has begun. We pass the offset
   // because in some cases the offset might not be what the cache
   // requested. In particular we might unexpectedly start providing
   // data at offset 0. This need not be called if the offset is the