Bug 1500049 - Wait for MediaCacheStreams to close properly before finishing MediaDecoder shutdown. r=bryce

Differential Revision: https://phabricator.services.mozilla.com/D52052

--HG--
extra : moz-landing-system : lando

dom/media/ChannelMediaDecoder.cpp

@@ -221,13 +221,27 @@ 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) {

dom/media/ChannelMediaDecoder.h

@@ -59,6 +59,7 @@ class ChannelMediaDecoder
   };
 
  protected:
+  void ShutdownInternal() override;
   void OnPlaybackEvent(MediaPlaybackEvent&& aEvent) override;
   void DurationChanged() override;
   void MetadataLoaded(UniquePtr<MediaInfo> aInfo, UniquePtr<MetadataTags> aTags,
@@ -156,6 +157,10 @@ class ChannelMediaDecoder
   // 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

dom/media/ChannelMediaResource.cpp

@@ -589,15 +589,15 @@ nsresult ChannelMediaResource::SetupChannelHeaders(int64_t aOffset) {
   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() {

dom/media/ChannelMediaResource.h

@@ -117,7 +117,7 @@ class ChannelMediaResource
 
   // 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;

dom/media/CloneableWithRangeMediaResource.cpp

@@ -148,7 +148,9 @@ nsresult CloneableWithRangeMediaResource::Open(
   return NS_OK;
 }
 
-nsresult CloneableWithRangeMediaResource::Close() { return NS_OK; }
+RefPtr<GenericPromise> CloneableWithRangeMediaResource::Close() {
+  return GenericPromise::CreateAndResolve(true, __func__);
+}
 
 already_AddRefed<nsIPrincipal>
 CloneableWithRangeMediaResource::GetCurrentPrincipal() {

dom/media/CloneableWithRangeMediaResource.h

@@ -27,7 +27,7 @@ class CloneableWithRangeMediaResource : public BaseMediaResource {
 
   // 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;

dom/media/FileMediaResource.cpp

@@ -95,7 +95,7 @@ nsresult FileMediaResource::Open(nsIStreamListener** aStreamListener) {
   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
@@ -105,7 +105,7 @@ nsresult FileMediaResource::Close() {
     mChannel = nullptr;
   }
 
-  return NS_OK;
+  return GenericPromise::CreateAndResolve(true, __func__);
 }
 
 already_AddRefed<nsIPrincipal> FileMediaResource::GetCurrentPrincipal() {

dom/media/FileMediaResource.h

@@ -23,7 +23,7 @@ class FileMediaResource : public BaseMediaResource {
 
   // 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;

dom/media/MediaCache.cpp

@@ -161,7 +161,7 @@ class MediaCache {
   // 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();
@@ -2196,17 +2196,18 @@ bool MediaCacheStream::AreAllStreamsForResourceSuspended(AutoLock& aLock) {
   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) {
@@ -2734,7 +2735,7 @@ void MediaCacheStream::InitAsCloneInternal(MediaCacheStream* aOriginal) {
   lock.NotifyAll();
 }
 
-nsIEventTarget* MediaCacheStream::OwnerThread() const {
+nsISerialEventTarget* MediaCacheStream::OwnerThread() const {
   return mMediaCache->OwnerThread();
 }
 

dom/media/MediaCache.h

@@ -217,12 +217,12 @@ class MediaCacheStream : public DecoderDoctorLifeLogger<MediaCacheStream> {
   // 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.

dom/media/MediaDecoder.cpp

@@ -390,7 +390,7 @@ void MediaDecoder::Shutdown() {
     nsCOMPtr<nsIRunnable> r =
         NS_NewRunnableFunction("MediaDecoder::Shutdown", [self]() {
           self->mVideoFrameContainer = nullptr;
-          MediaShutdownManager::Instance().Unregister(self);
+          self->ShutdownInternal();
         });
     mAbstractMainThread->Dispatch(r.forget());
   }
@@ -531,11 +531,16 @@ void MediaDecoder::OnStoreDecoderBenchmark(const VideoInfo& aInfo) {
   }
 }
 
+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() {

dom/media/MediaDecoder.h

@@ -403,6 +403,11 @@ class MediaDecoder : public DecoderDoctorLifeLogger<MediaDecoder> {
 
   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.

dom/media/MediaResource.h

@@ -60,8 +60,11 @@ class MediaResource : public DecoderDoctorLifeLogger<MediaResource> {
 
   // 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

dom/media/mediasource/SourceBufferResource.cpp

@@ -24,11 +24,11 @@ mozilla::LogModule* GetSourceBufferResourceLog() {
 
 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,

dom/media/mediasource/SourceBufferResource.h

@@ -36,7 +36,7 @@ class SourceBufferResource final
       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.