Bug 1500049 - Wait for MediaCacheStreams to close properly before finishing MediaDecoder shutdown. r=bryce
☠☠ backed out by 7272d77d4e80 ☠ ☠
authorAndreas Pehrson <apehrson@mozilla.com>
Wed, 13 Nov 2019 08:58:34 +0000
changeset 501797 355f090421a6b38618b3f2e2e0c08d9166a78e72
parent 501796 306341d0b5869bafb98f11492f64f3aa0e4e51a7
child 501798 9cdc93b402a207bd714de113f4bcfd7086062cf2
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbryce
bugs1500049
milestone72.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 1500049 - Wait for MediaCacheStreams to close properly before finishing MediaDecoder shutdown. r=bryce Differential Revision: https://phabricator.services.mozilla.com/D52052
dom/media/ChannelMediaDecoder.cpp
dom/media/ChannelMediaDecoder.h
dom/media/ChannelMediaResource.cpp
dom/media/ChannelMediaResource.h
dom/media/CloneableWithRangeMediaResource.cpp
dom/media/CloneableWithRangeMediaResource.h
dom/media/FileMediaResource.cpp
dom/media/FileMediaResource.h
dom/media/MediaCache.cpp
dom/media/MediaCache.h
dom/media/MediaDecoder.cpp
dom/media/MediaDecoder.h
dom/media/MediaResource.h
dom/media/mediasource/SourceBufferResource.cpp
dom/media/mediasource/SourceBufferResource.h
--- a/dom/media/ChannelMediaDecoder.cpp
+++ b/dom/media/ChannelMediaDecoder.cpp
@@ -216,23 +216,37 @@ MediaDecoderStateMachine* ChannelMediaDe
   mReader = DecoderTraits::CreateReader(ContainerType(), init);
   return new MediaDecoderStateMachine(this, mReader);
 }
 
 void ChannelMediaDecoder::Shutdown() {
   mResourceCallback->Disconnect();
   MediaDecoder::Shutdown();
 
-  // Force any outstanding seek and byterange requests to complete
-  // to prevent shutdown from deadlocking.
   if (mResource) {
-    mResource->Close();
+    // Force any outstanding seek and byterange requests to complete
+    // to prevent shutdown from deadlocking.
+    mResourceClosePromise = mResource->Close();
   }
 }
 
+void ChannelMediaDecoder::ShutdownInternal() {
+  if (!mResourceClosePromise) {
+    MediaShutdownManager::Instance().Unregister(this);
+    return;
+  }
+
+  mResourceClosePromise->Then(
+      AbstractMainThread(), __func__,
+      [self = RefPtr<ChannelMediaDecoder>(this)] {
+        MediaShutdownManager::Instance().Unregister(self);
+      });
+  return;
+}
+
 nsresult ChannelMediaDecoder::Load(nsIChannel* aChannel,
                                    bool aIsPrivateBrowsing,
                                    nsIStreamListener** aStreamListener) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!mResource);
   MOZ_ASSERT(aStreamListener);
   AbstractThread::AutoEnter context(AbstractMainThread());
 
--- a/dom/media/ChannelMediaDecoder.h
+++ b/dom/media/ChannelMediaDecoder.h
@@ -54,16 +54,17 @@ class ChannelMediaDecoder
     // The decoder to send notifications. Main-thread only.
     ChannelMediaDecoder* mDecoder = nullptr;
     nsCOMPtr<nsITimer> mTimer;
     bool mTimerArmed = false;
     const RefPtr<AbstractThread> mAbstractMainThread;
   };
 
  protected:
+  void ShutdownInternal() override;
   void OnPlaybackEvent(MediaPlaybackEvent&& aEvent) override;
   void DurationChanged() override;
   void MetadataLoaded(UniquePtr<MediaInfo> aInfo, UniquePtr<MetadataTags> aTags,
                       MediaDecoderEventVisibility aEventVisibility) override;
   void NotifyPrincipalChanged() override;
 
   RefPtr<ResourceCallback> mResourceCallback;
   RefPtr<BaseMediaResource> mResource;
@@ -151,13 +152,17 @@ class ChannelMediaDecoder
   // start playing back again.
   int64_t mPlaybackPosition = 0;
 
   bool mCanPlayThrough = false;
 
   // True if we've been notified that the ChannelMediaResource has
   // a principal.
   bool mInitialChannelPrincipalKnown = false;
+
+  // Set in Shutdown() when we start closing mResource, if mResource is set.
+  // Must resolve before we unregister the shutdown blocker.
+  RefPtr<GenericPromise> mResourceClosePromise;
 };
 
 }  // namespace mozilla
 
 #endif  // ChannelMediaDecoder_h_
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -584,25 +584,25 @@ nsresult ChannelMediaResource::SetupChan
     element->SetRequestHeaders(hc);
   } else {
     NS_ASSERTION(aOffset == 0, "Don't know how to seek on this channel type");
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
-nsresult ChannelMediaResource::Close() {
+RefPtr<GenericPromise> ChannelMediaResource::Close() {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   if (!mClosed) {
     CloseChannel();
-    mCacheStream.Close();
     mClosed = true;
+    return mCacheStream.Close();
   }
-  return NS_OK;
+  return GenericPromise::CreateAndResolve(true, __func__);
 }
 
 already_AddRefed<nsIPrincipal> ChannelMediaResource::GetCurrentPrincipal() {
   MOZ_ASSERT(NS_IsMainThread());
   return do_AddRef(mSharedInfo->mPrincipal);
 }
 
 bool ChannelMediaResource::HadCrossOriginRedirects() {
--- a/dom/media/ChannelMediaResource.h
+++ b/dom/media/ChannelMediaResource.h
@@ -112,17 +112,17 @@ class ChannelMediaResource
   void CacheClientResume();
 
   bool IsSuspended();
 
   void ThrottleReadahead(bool bThrottle) override;
 
   // Main thread
   nsresult Open(nsIStreamListener** aStreamListener) override;
-  nsresult Close() override;
+  RefPtr<GenericPromise> Close() override;
   void Suspend(bool aCloseImmediately) override;
   void Resume() override;
   already_AddRefed<nsIPrincipal> GetCurrentPrincipal() override;
   bool HadCrossOriginRedirects() override;
   bool CanClone() override;
   already_AddRefed<BaseMediaResource> CloneData(
       MediaResourceCallback* aDecoder) override;
   nsresult ReadFromCache(char* aBuffer, int64_t aOffset,
--- a/dom/media/CloneableWithRangeMediaResource.cpp
+++ b/dom/media/CloneableWithRangeMediaResource.cpp
@@ -143,17 +143,19 @@ nsresult CloneableWithRangeMediaResource
     nsIStreamListener** aStreamListener) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aStreamListener);
 
   *aStreamListener = nullptr;
   return NS_OK;
 }
 
-nsresult CloneableWithRangeMediaResource::Close() { return NS_OK; }
+RefPtr<GenericPromise> CloneableWithRangeMediaResource::Close() {
+  return GenericPromise::CreateAndResolve(true, __func__);
+}
 
 already_AddRefed<nsIPrincipal>
 CloneableWithRangeMediaResource::GetCurrentPrincipal() {
   MOZ_ASSERT(NS_IsMainThread());
 
   nsCOMPtr<nsIPrincipal> principal;
   nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
   if (!secMan || !mChannel) {
--- a/dom/media/CloneableWithRangeMediaResource.h
+++ b/dom/media/CloneableWithRangeMediaResource.h
@@ -22,17 +22,17 @@ class CloneableWithRangeMediaResource : 
         mInitialized(false) {
     MOZ_ASSERT(mStream);
   }
 
   ~CloneableWithRangeMediaResource() {}
 
   // Main thread
   nsresult Open(nsIStreamListener** aStreamListener) override;
-  nsresult Close() override;
+  RefPtr<GenericPromise> Close() override;
   void Suspend(bool aCloseImmediately) override {}
   void Resume() override {}
   already_AddRefed<nsIPrincipal> GetCurrentPrincipal() override;
   bool HadCrossOriginRedirects() override;
   nsresult ReadFromCache(char* aBuffer, int64_t aOffset,
                          uint32_t aCount) override;
 
   // These methods are called off the main thread.
--- a/dom/media/FileMediaResource.cpp
+++ b/dom/media/FileMediaResource.cpp
@@ -90,27 +90,27 @@ nsresult FileMediaResource::Open(nsIStre
     // doing an async open and waiting until we locate the real resource,
     // then using that (if it's still a file!).
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
 }
 
-nsresult FileMediaResource::Close() {
+RefPtr<GenericPromise> FileMediaResource::Close() {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   // Since mChennel is only accessed by main thread, there is no necessary to
   // take the lock.
   if (mChannel) {
     mChannel->Cancel(NS_ERROR_PARSED_DATA_CACHED);
     mChannel = nullptr;
   }
 
-  return NS_OK;
+  return GenericPromise::CreateAndResolve(true, __func__);
 }
 
 already_AddRefed<nsIPrincipal> FileMediaResource::GetCurrentPrincipal() {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   nsCOMPtr<nsIPrincipal> principal;
   nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
   if (!secMan || !mChannel) return nullptr;
--- a/dom/media/FileMediaResource.h
+++ b/dom/media/FileMediaResource.h
@@ -18,17 +18,17 @@ class FileMediaResource : public BaseMed
       : BaseMediaResource(aCallback, aChannel, aURI),
         mSize(aSize),
         mLock("FileMediaResource.mLock"),
         mSizeInitialized(aSize != -1) {}
   ~FileMediaResource() {}
 
   // Main thread
   nsresult Open(nsIStreamListener** aStreamListener) override;
-  nsresult Close() override;
+  RefPtr<GenericPromise> Close() override;
   void Suspend(bool aCloseImmediately) override {}
   void Resume() override {}
   already_AddRefed<nsIPrincipal> GetCurrentPrincipal() override;
   bool HadCrossOriginRedirects() override;
   nsresult ReadFromCache(char* aBuffer, int64_t aOffset,
                          uint32_t aCount) override;
 
   // These methods are called off the main thread.
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -156,17 +156,17 @@ class MediaCache {
 
   // Get an instance of a MediaCache (or nullptr if initialization failed).
   // aContentLength is the content length if known already, otherwise -1.
   // If the length is known and considered small enough, a discrete MediaCache
   // with memory backing will be given. Otherwise the one MediaCache with
   // file backing will be provided.
   static RefPtr<MediaCache> GetMediaCache(int64_t aContentLength);
 
-  nsIEventTarget* OwnerThread() const { return sThread; }
+  nsISerialEventTarget* OwnerThread() const { return sThread; }
 
   // Brutally flush the cache contents. Main thread only.
   void Flush();
 
   // Close all streams associated with private browsing windows. This will
   // also remove the blocks from the cache since we don't want to leave any
   // traces when PB is done.
   void CloseStreamsForPrivateBrowsing();
@@ -2191,27 +2191,28 @@ bool MediaCacheStream::AreAllStreamsForR
       continue;
     }
     return false;
   }
 
   return true;
 }
 
-void MediaCacheStream::Close() {
+RefPtr<GenericPromise> MediaCacheStream::Close() {
   MOZ_ASSERT(NS_IsMainThread());
   if (!mMediaCache) {
-    return;
+    return GenericPromise::CreateAndResolve(true, __func__);
   }
-  OwnerThread()->Dispatch(NS_NewRunnableFunction(
-      "MediaCacheStream::Close",
-      [this, client = RefPtr<ChannelMediaResource>(mClient)]() {
-        AutoLock lock(mMediaCache->Monitor());
-        CloseInternal(lock);
-      }));
+
+  return InvokeAsync(OwnerThread(), "MediaCacheStream::Close",
+                     [this, client = RefPtr<ChannelMediaResource>(mClient)] {
+                       AutoLock lock(mMediaCache->Monitor());
+                       CloseInternal(lock);
+                       return GenericPromise::CreateAndResolve(true, __func__);
+                     });
 }
 
 void MediaCacheStream::CloseInternal(AutoLock& aLock) {
   MOZ_ASSERT(OwnerThread()->IsOnCurrentThread());
 
   if (mClosed) {
     return;
   }
@@ -2729,17 +2730,17 @@ void MediaCacheStream::InitAsCloneIntern
   mClient->CacheClientSuspend();
 
   // Step 5: add the stream to be managed by the cache.
   mMediaCache->OpenStream(lock, this, true /* aIsClone */);
   // Wake up the reader which is waiting for the cloned data.
   lock.NotifyAll();
 }
 
-nsIEventTarget* MediaCacheStream::OwnerThread() const {
+nsISerialEventTarget* MediaCacheStream::OwnerThread() const {
   return mMediaCache->OwnerThread();
 }
 
 nsresult MediaCacheStream::GetCachedRanges(MediaByteRangeSet& aRanges) {
   MOZ_ASSERT(!NS_IsMainThread());
   // Take the monitor, so that the cached data ranges can't grow while we're
   // trying to loop over them.
   AutoLock lock(mMediaCache->Monitor());
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -212,22 +212,22 @@ class MediaCacheStream : public DecoderD
   nsresult Init(int64_t aContentLength);
 
   // Set up this stream with the cache, assuming it's for the same data
   // as the aOriginal stream.
   // Exactly one of InitAsClone or Init must be called before any other method
   // on this class.
   void InitAsClone(MediaCacheStream* aOriginal);
 
-  nsIEventTarget* OwnerThread() const;
+  nsISerialEventTarget* OwnerThread() const;
 
   // These are called on the main thread.
-  // This must be called (and return) before the ChannelMediaResource
+  // This must be called (and resolve) before the ChannelMediaResource
   // used to create this MediaCacheStream is deleted.
-  void Close();
+  RefPtr<GenericPromise> Close();
   // This returns true when the stream has been closed.
   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 !mIsPrivateBrowsing; }
 
   // These callbacks are called on the main thread by the client
   // when data has been received via the channel.
--- a/dom/media/MediaDecoder.cpp
+++ b/dom/media/MediaDecoder.cpp
@@ -385,17 +385,17 @@ void MediaDecoder::Shutdown() {
         &MediaDecoder::FinishShutdown);
   } else {
     // Ensure we always unregister asynchronously in order not to disrupt
     // the hashtable iterating in MediaShutdownManager::Shutdown().
     RefPtr<MediaDecoder> self = this;
     nsCOMPtr<nsIRunnable> r =
         NS_NewRunnableFunction("MediaDecoder::Shutdown", [self]() {
           self->mVideoFrameContainer = nullptr;
-          MediaShutdownManager::Instance().Unregister(self);
+          self->ShutdownInternal();
         });
     mAbstractMainThread->Dispatch(r.forget());
   }
 
   ChangeState(PLAY_STATE_SHUTDOWN);
   mVideoDecodingOberver->UnregisterEvent();
   mVideoDecodingOberver = nullptr;
   mOwner = nullptr;
@@ -526,21 +526,26 @@ void MediaDecoder::OnStoreDecoderBenchma
         "type = %s\n",
         benchmarkInfo.mWidth, benchmarkInfo.mHeight, benchmarkInfo.mFrameRate,
         benchmarkInfo.mContentType.BeginReading());
 
     mDecoderBenchmark->Store(benchmarkInfo, mFrameStats);
   }
 }
 
+void MediaDecoder::ShutdownInternal() {
+  MOZ_ASSERT(NS_IsMainThread());
+  MediaShutdownManager::Instance().Unregister(this);
+}
+
 void MediaDecoder::FinishShutdown() {
   MOZ_ASSERT(NS_IsMainThread());
   SetStateMachine(nullptr);
   mVideoFrameContainer = nullptr;
-  MediaShutdownManager::Instance().Unregister(this);
+  ShutdownInternal();
 }
 
 nsresult MediaDecoder::InitializeStateMachine() {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ASSERTION(mDecoderStateMachine, "Cannot initialize null state machine!");
   AbstractThread::AutoEnter context(AbstractMainThread());
 
   nsresult rv = mDecoderStateMachine->Init(this);
--- a/dom/media/MediaDecoder.h
+++ b/dom/media/MediaDecoder.h
@@ -398,16 +398,21 @@ class MediaDecoder : public DecoderDocto
 
   // Called when the first audio and/or video from the media file has been
   // loaded by the state machine. Call on the main thread only.
   virtual void FirstFrameLoaded(nsAutoPtr<MediaInfo> aInfo,
                                 MediaDecoderEventVisibility aEventVisibility);
 
   void SetStateMachineParameters();
 
+  // Called when MediaDecoder shutdown is finished. Subclasses use this to clean
+  // up internal structures, and unregister potential shutdown blockers when
+  // they're done.
+  virtual void ShutdownInternal();
+
   bool IsShutdown() const;
 
   // Called to notify the decoder that the duration has changed.
   virtual void DurationChanged();
 
   // State-watching manager.
   WatchManager<MediaDecoder> mWatchManager;
 
--- a/dom/media/MediaResource.h
+++ b/dom/media/MediaResource.h
@@ -55,18 +55,21 @@ class MediaResource : public DecoderDoct
   // Note that this means it's safe for references to this object to be
   // released on a non main thread, but the destructor will always run on
   // the main thread.
   NS_METHOD_(MozExternalRefCountType) AddRef(void);
   NS_METHOD_(MozExternalRefCountType) Release(void);
 
   // Close the resource, stop any listeners, channels, etc.
   // Cancels any currently blocking Read request and forces that request to
-  // return an error.
-  virtual nsresult Close() { return NS_OK; }
+  // return an error. This must be called (and resolve) before the MediaResource
+  // is deleted.
+  virtual RefPtr<GenericPromise> Close() {
+    return GenericPromise::CreateAndResolve(true, __func__);
+  }
 
   // These methods are called off the main thread.
   // Read up to aCount bytes from the stream. The read starts at
   // aOffset in the stream, seeking to that location initially if
   // it is not the current stream offset. The remaining arguments,
   // results and requirements are the same as per the Read method.
   virtual nsresult ReadAt(int64_t aOffset, char* aBuffer, uint32_t aCount,
                           uint32_t* aBytes) = 0;
--- a/dom/media/mediasource/SourceBufferResource.cpp
+++ b/dom/media/mediasource/SourceBufferResource.cpp
@@ -19,21 +19,21 @@ mozilla::LogModule* GetSourceBufferResou
   DDMOZ_LOG(GetSourceBufferResourceLog(), mozilla::LogLevel::Debug, \
             "::%s: " arg, __func__, ##__VA_ARGS__)
 #define SBR_DEBUGV(arg, ...)                                          \
   DDMOZ_LOG(GetSourceBufferResourceLog(), mozilla::LogLevel::Verbose, \
             "::%s: " arg, __func__, ##__VA_ARGS__)
 
 namespace mozilla {
 
-nsresult SourceBufferResource::Close() {
+RefPtr<GenericPromise> SourceBufferResource::Close() {
   MOZ_ASSERT(OnThread());
   SBR_DEBUG("Close");
   mClosed = true;
-  return NS_OK;
+  return GenericPromise::CreateAndResolve(true, __func__);
 }
 
 nsresult SourceBufferResource::ReadAt(int64_t aOffset, char* aBuffer,
                                       uint32_t aCount, uint32_t* aBytes) {
   SBR_DEBUG("ReadAt(aOffset=%" PRId64 ", aBuffer=%p, aCount=%u, aBytes=%p)",
             aOffset, aBytes, aCount, aBytes);
   return ReadAtInternal(aOffset, aBuffer, aCount, aBytes);
 }
--- a/dom/media/mediasource/SourceBufferResource.h
+++ b/dom/media/mediasource/SourceBufferResource.h
@@ -31,17 +31,17 @@ class SourceBuffer;
 DDLoggedTypeDeclNameAndBase(SourceBufferResource, MediaResource);
 
 // SourceBufferResource is not thread safe.
 class SourceBufferResource final
     : public MediaResource,
       public DecoderDoctorLifeLogger<SourceBufferResource> {
  public:
   SourceBufferResource();
-  nsresult Close() override;
+  RefPtr<GenericPromise> Close() override;
   nsresult ReadAt(int64_t aOffset, char* aBuffer, uint32_t aCount,
                   uint32_t* aBytes) override;
   // Memory-based and no locks, caching discouraged.
   bool ShouldCacheReads() override { return false; }
   void Pin() override { UNIMPLEMENTED(); }
   void Unpin() override { UNIMPLEMENTED(); }
   int64_t GetLength() override { return mInputBuffer.GetLength(); }
   int64_t GetNextCachedData(int64_t aOffset) override {