From 4c3bf72271ee323113a93f62fa370be385afa059 Mon Sep 17 00:00:00 2001 From: Andreas Pehrson Date: Wed, 13 Nov 2019 08:58:34 +0000 Subject: [PATCH] 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 | 20 +++++++++++++++--- dom/media/ChannelMediaDecoder.h | 5 +++++ dom/media/ChannelMediaResource.cpp | 6 +++--- dom/media/ChannelMediaResource.h | 2 +- dom/media/CloneableWithRangeMediaResource.cpp | 4 +++- dom/media/CloneableWithRangeMediaResource.h | 2 +- dom/media/FileMediaResource.cpp | 4 ++-- dom/media/FileMediaResource.h | 2 +- dom/media/MediaCache.cpp | 21 ++++++++++--------- dom/media/MediaCache.h | 6 +++--- dom/media/MediaDecoder.cpp | 9 ++++++-- dom/media/MediaDecoder.h | 5 +++++ dom/media/MediaResource.h | 7 +++++-- .../mediasource/SourceBufferResource.cpp | 4 ++-- dom/media/mediasource/SourceBufferResource.h | 2 +- 15 files changed, 67 insertions(+), 32 deletions(-) diff --git a/dom/media/ChannelMediaDecoder.cpp b/dom/media/ChannelMediaDecoder.cpp index e79b37913a70..90450d28828e 100644 --- a/dom/media/ChannelMediaDecoder.cpp +++ b/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(this)] { + MediaShutdownManager::Instance().Unregister(self); + }); + return; +} + nsresult ChannelMediaDecoder::Load(nsIChannel* aChannel, bool aIsPrivateBrowsing, nsIStreamListener** aStreamListener) { diff --git a/dom/media/ChannelMediaDecoder.h b/dom/media/ChannelMediaDecoder.h index 3a500890c3cd..ed53496d3be4 100644 --- a/dom/media/ChannelMediaDecoder.h +++ b/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 aInfo, UniquePtr 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 mResourceClosePromise; }; } // namespace mozilla diff --git a/dom/media/ChannelMediaResource.cpp b/dom/media/ChannelMediaResource.cpp index 06ab2c0d5011..8653269f36b0 100644 --- a/dom/media/ChannelMediaResource.cpp +++ b/dom/media/ChannelMediaResource.cpp @@ -589,15 +589,15 @@ nsresult ChannelMediaResource::SetupChannelHeaders(int64_t aOffset) { return NS_OK; } -nsresult ChannelMediaResource::Close() { +RefPtr 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 ChannelMediaResource::GetCurrentPrincipal() { diff --git a/dom/media/ChannelMediaResource.h b/dom/media/ChannelMediaResource.h index a9e07e80691c..a6e6459b6a8a 100644 --- a/dom/media/ChannelMediaResource.h +++ b/dom/media/ChannelMediaResource.h @@ -117,7 +117,7 @@ class ChannelMediaResource // Main thread nsresult Open(nsIStreamListener** aStreamListener) override; - nsresult Close() override; + RefPtr Close() override; void Suspend(bool aCloseImmediately) override; void Resume() override; already_AddRefed GetCurrentPrincipal() override; diff --git a/dom/media/CloneableWithRangeMediaResource.cpp b/dom/media/CloneableWithRangeMediaResource.cpp index bf2839ba876e..74d9cb4ce5b1 100644 --- a/dom/media/CloneableWithRangeMediaResource.cpp +++ b/dom/media/CloneableWithRangeMediaResource.cpp @@ -148,7 +148,9 @@ nsresult CloneableWithRangeMediaResource::Open( return NS_OK; } -nsresult CloneableWithRangeMediaResource::Close() { return NS_OK; } +RefPtr CloneableWithRangeMediaResource::Close() { + return GenericPromise::CreateAndResolve(true, __func__); +} already_AddRefed CloneableWithRangeMediaResource::GetCurrentPrincipal() { diff --git a/dom/media/CloneableWithRangeMediaResource.h b/dom/media/CloneableWithRangeMediaResource.h index 4f78f798ab44..d3aafce731f2 100644 --- a/dom/media/CloneableWithRangeMediaResource.h +++ b/dom/media/CloneableWithRangeMediaResource.h @@ -27,7 +27,7 @@ class CloneableWithRangeMediaResource : public BaseMediaResource { // Main thread nsresult Open(nsIStreamListener** aStreamListener) override; - nsresult Close() override; + RefPtr Close() override; void Suspend(bool aCloseImmediately) override {} void Resume() override {} already_AddRefed GetCurrentPrincipal() override; diff --git a/dom/media/FileMediaResource.cpp b/dom/media/FileMediaResource.cpp index 94671d502178..09ee243bf01f 100644 --- a/dom/media/FileMediaResource.cpp +++ b/dom/media/FileMediaResource.cpp @@ -95,7 +95,7 @@ nsresult FileMediaResource::Open(nsIStreamListener** aStreamListener) { return NS_OK; } -nsresult FileMediaResource::Close() { +RefPtr 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 FileMediaResource::GetCurrentPrincipal() { diff --git a/dom/media/FileMediaResource.h b/dom/media/FileMediaResource.h index a1a61b56ca1a..18b1828b13d2 100644 --- a/dom/media/FileMediaResource.h +++ b/dom/media/FileMediaResource.h @@ -23,7 +23,7 @@ class FileMediaResource : public BaseMediaResource { // Main thread nsresult Open(nsIStreamListener** aStreamListener) override; - nsresult Close() override; + RefPtr Close() override; void Suspend(bool aCloseImmediately) override {} void Resume() override {} already_AddRefed GetCurrentPrincipal() override; diff --git a/dom/media/MediaCache.cpp b/dom/media/MediaCache.cpp index 6cb0f34763c3..91fe23889bf8 100644 --- a/dom/media/MediaCache.cpp +++ b/dom/media/MediaCache.cpp @@ -161,7 +161,7 @@ class MediaCache { // file backing will be provided. static RefPtr 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 MediaCacheStream::Close() { MOZ_ASSERT(NS_IsMainThread()); if (!mMediaCache) { - return; + return GenericPromise::CreateAndResolve(true, __func__); } - OwnerThread()->Dispatch(NS_NewRunnableFunction( - "MediaCacheStream::Close", - [this, client = RefPtr(mClient)]() { - AutoLock lock(mMediaCache->Monitor()); - CloseInternal(lock); - })); + + return InvokeAsync(OwnerThread(), "MediaCacheStream::Close", + [this, client = RefPtr(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(); } diff --git a/dom/media/MediaCache.h b/dom/media/MediaCache.h index 5e639275e0b2..c19669de8a70 100644 --- a/dom/media/MediaCache.h +++ b/dom/media/MediaCache.h @@ -217,12 +217,12 @@ class MediaCacheStream : public DecoderDoctorLifeLogger { // 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 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. diff --git a/dom/media/MediaDecoder.cpp b/dom/media/MediaDecoder.cpp index 6a7941014d4a..2e544410a511 100644 --- a/dom/media/MediaDecoder.cpp +++ b/dom/media/MediaDecoder.cpp @@ -390,7 +390,7 @@ void MediaDecoder::Shutdown() { nsCOMPtr 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() { diff --git a/dom/media/MediaDecoder.h b/dom/media/MediaDecoder.h index cd9a474d54fa..01a4d6735fd3 100644 --- a/dom/media/MediaDecoder.h +++ b/dom/media/MediaDecoder.h @@ -403,6 +403,11 @@ class MediaDecoder : public DecoderDoctorLifeLogger { 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. diff --git a/dom/media/MediaResource.h b/dom/media/MediaResource.h index e80632bbfc79..280953e679d8 100644 --- a/dom/media/MediaResource.h +++ b/dom/media/MediaResource.h @@ -60,8 +60,11 @@ class MediaResource : public DecoderDoctorLifeLogger { // 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 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 diff --git a/dom/media/mediasource/SourceBufferResource.cpp b/dom/media/mediasource/SourceBufferResource.cpp index f0dedf390786..49447be01532 100644 --- a/dom/media/mediasource/SourceBufferResource.cpp +++ b/dom/media/mediasource/SourceBufferResource.cpp @@ -24,11 +24,11 @@ mozilla::LogModule* GetSourceBufferResourceLog() { namespace mozilla { -nsresult SourceBufferResource::Close() { +RefPtr 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, diff --git a/dom/media/mediasource/SourceBufferResource.h b/dom/media/mediasource/SourceBufferResource.h index 8649a1de9fbf..5810412ea390 100644 --- a/dom/media/mediasource/SourceBufferResource.h +++ b/dom/media/mediasource/SourceBufferResource.h @@ -36,7 +36,7 @@ class SourceBufferResource final public DecoderDoctorLifeLogger { public: SourceBufferResource(); - nsresult Close() override; + RefPtr Close() override; nsresult ReadAt(int64_t aOffset, char* aBuffer, uint32_t aCount, uint32_t* aBytes) override; // Memory-based and no locks, caching discouraged.