From 96caacdf427ad10be5c60a50ffdfd97912bbbf77 Mon Sep 17 00:00:00 2001 From: Randell Jesup Date: Fri, 29 May 2015 10:10:02 -0400 Subject: [PATCH] Bug 1164463: clean up MediaManager shutdown to be reliable and avoid holding locks while Joining a thread r=jib --- dom/media/MediaManager.cpp | 107 ++++++++++++------ dom/media/MediaManager.h | 3 +- dom/media/webrtc/MediaEngine.h | 5 + .../webrtc/MediaEngineCameraVideoSource.h | 2 + dom/media/webrtc/MediaEngineDefault.h | 18 ++- dom/media/webrtc/MediaEngineTabVideoSource.h | 1 + dom/media/webrtc/MediaEngineWebRTC.cpp | 35 +++++- dom/media/webrtc/MediaEngineWebRTC.h | 18 +-- dom/media/webrtc/MediaEngineWebRTCAudio.cpp | 4 +- dom/media/webrtc/MediaEngineWebRTCVideo.cpp | 9 +- 10 files changed, 148 insertions(+), 54 deletions(-) diff --git a/dom/media/MediaManager.cpp b/dom/media/MediaManager.cpp index e7aa3f06d52..cdff28dc401 100644 --- a/dom/media/MediaManager.cpp +++ b/dom/media/MediaManager.cpp @@ -124,6 +124,8 @@ using dom::OwningBooleanOrMediaTrackConstraints; using dom::SupportedAudioConstraints; using dom::SupportedVideoConstraints; +static Atomic sInShutdown; + static bool HostInDomain(const nsCString &aHost, const nsCString &aPattern) { @@ -946,7 +948,7 @@ public: nsRefPtr trackunion = nsDOMUserMediaStream::CreateTrackUnionStream(window, mListener, mAudioSource, mVideoSource); - if (!trackunion) { + if (!trackunion || sInShutdown) { nsCOMPtr onFailure = mOnFailure.forget(); LOG(("Returning error for getUserMedia() - no stream")); @@ -954,7 +956,8 @@ public: if (window) { nsRefPtr error = new MediaStreamError(window, NS_LITERAL_STRING("InternalError"), - NS_LITERAL_STRING("No stream.")); + sInShutdown ? NS_LITERAL_STRING("In shutdown") : + NS_LITERAL_STRING("No stream.")); onFailure->OnError(error); } return NS_OK; @@ -1009,7 +1012,7 @@ public: // because that can take a while. // Pass ownership of trackunion to the MediaOperationTask // to ensure it's kept alive until the MediaOperationTask runs (at least). - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new MediaOperationTask(MEDIA_START, mListener, trackunion, tracksAvailableCallback, mAudioSource, mVideoSource, false, mWindowID, @@ -1612,7 +1615,7 @@ MediaManager::Get() { nsCOMPtr obs = services::GetObserverService(); if (obs) { - obs->AddObserver(sSingleton, "xpcom-shutdown", false); + obs->AddObserver(sSingleton, "xpcom-will-shutdown", false); obs->AddObserver(sSingleton, "getUserMedia:response:allow", false); obs->AddObserver(sSingleton, "getUserMedia:response:deny", false); obs->AddObserver(sSingleton, "getUserMedia:revoke", false); @@ -1644,12 +1647,17 @@ MediaManager::GetInstance() } /* static */ -MessageLoop* -MediaManager::GetMessageLoop() +void +MediaManager::PostTask(const tracked_objects::Location& from_here, Task* task) { + if (sInShutdown) { + // Can't safely delete task here since it may have items with specific + // thread-release requirements. + return; + } NS_ASSERTION(Get(), "MediaManager singleton?"); NS_ASSERTION(Get()->mMediaThread, "No thread yet"); - return Get()->mMediaThread->message_loop(); + Get()->mMediaThread->message_loop()->PostTask(from_here, task); } /* static */ nsresult @@ -1783,6 +1791,9 @@ MediaManager::GetUserMedia( task = new GetUserMediaTask(c, onSuccess.forget(), onFailure.forget(), windowID, listener, mPrefs); } + if (sInShutdown) { + return task->Denied(NS_LITERAL_STRING("In shutdown")); + } nsIURI* docURI = aWindow->GetDocumentURI(); @@ -1889,7 +1900,7 @@ MediaManager::GetUserMedia( // XXX No full support for picture in Desktop yet (needs proper UI) if (privileged || (fake && !Preferences::GetBool("media.navigator.permission.fake"))) { - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, task.forget()); + MediaManager::PostTask(FROM_HERE, task.forget()); } else { bool isHTTPS = false; if (docURI) { @@ -1974,6 +1985,7 @@ MediaManager::GetUserMediaDevices(nsPIDOMWindow* aWindow, NS_ENSURE_TRUE(aOnFailure, NS_ERROR_NULL_POINTER); NS_ENSURE_TRUE(aOnSuccess, NS_ERROR_NULL_POINTER); + NS_ENSURE_TRUE(!sInShutdown, NS_ERROR_FAILURE); nsCOMPtr onSuccess(aOnSuccess); nsCOMPtr onFailure(aOnFailure); @@ -1994,7 +2006,7 @@ MediaManager::GetUserMediaDevices(nsPIDOMWindow* aWindow, nsCOMPtr loadContext = doc->GetLoadContext(); inPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing(); } - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new GetUserMediaDevicesTask( aConstraints, onSuccess.forget(), onFailure.forget(), (aInnerWindowID ? aInnerWindowID : aWindow->WindowID()), @@ -2027,6 +2039,7 @@ MediaManager::GetBackend(uint64_t aWindowId) // This IS called off main-thread. MutexAutoLock lock(mMutex); if (!mBackend) { + MOZ_RELEASE_ASSERT(!sInShutdown); // we should never create a new backend in shutdown #if defined(MOZ_WEBRTC) mBackend = new MediaEngineWebRTC(mPrefs); #else @@ -2202,8 +2215,10 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic, LOG(("%s: %dx%d @%dfps (min %d)", __FUNCTION__, mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS)); } - } else if (!strcmp(aTopic, "xpcom-shutdown")) { - obs->RemoveObserver(this, "xpcom-shutdown"); + } else if (!strcmp(aTopic, "xpcom-will-shutdown")) { + sInShutdown = true; + + obs->RemoveObserver(this, "xpcom-will-shutdown"); obs->RemoveObserver(this, "getUserMedia:response:allow"); obs->RemoveObserver(this, "getUserMedia:response:deny"); obs->RemoveObserver(this, "getUserMedia:revoke"); @@ -2216,22 +2231,43 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic, prefs->RemoveObserver("media.navigator.video.default_minfps", this); } + // Close off any remaining active windows. + GetActiveWindows()->Clear(); + mActiveCallbacks.Clear(); + mCallIds.Clear(); + { + MutexAutoLock lock(mMutex); + if (mBackend) { + mBackend->Shutdown(); // ok to invoke multiple times + } + } + // Because mMediaThread is not an nsThread, we must dispatch to it so it can // clean up BackgroundChild. Continue stopping thread once this is done. class ShutdownTask : public Task { public: - explicit ShutdownTask(nsRunnable* aReply) : mReply(aReply) {} + ShutdownTask(TemporaryRef aBackend, + nsRunnable* aReply) + : mReply(aReply) + , mBackend(aBackend) {} private: virtual void Run() { + LOG(("MediaManager Thread Shutdown")); MOZ_ASSERT(MediaManager::IsInMediaThread()); mozilla::ipc::BackgroundChild::CloseForCurrentThread(); - NS_DispatchToMainThread(mReply); + // must explicitly do this before dispatching the reply, since the reply may kill us with Stop() + mBackend = nullptr; // last reference, will invoke Shutdown() again + + if (NS_FAILED(NS_DispatchToMainThread(mReply))) { + LOG(("Will leak thread: DispatchToMainthread of reply runnable failed in MediaManager shutdown")); + } } nsRefPtr mReply; + RefPtr mBackend; }; // Post ShutdownTask to execute on mMediaThread and pass in a lambda @@ -2241,20 +2277,25 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic, // This is safe since this is guaranteed to be here since sSingleton isn't // cleared until the lambda function clears it. - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, new ShutdownTask( - media::NewRunnableFrom([this]() mutable { - // Close off any remaining active windows. + // note that this == sSingleton + nsRefPtr that(sSingleton); + // Release the backend (and call Shutdown()) from within the MediaManager thread + RefPtr temp; + { MutexAutoLock lock(mMutex); - GetActiveWindows()->Clear(); - mActiveCallbacks.Clear(); - mCallIds.Clear(); - LOG(("Releasing MediaManager singleton and thread")); - // Note: won't be released immediately as the Observer has a ref to us - sSingleton = nullptr; + temp = mBackend.forget(); + } + // Don't use MediaManager::PostTask() because we're sInShutdown=true here! + mMediaThread->message_loop()->PostTask(FROM_HERE, new ShutdownTask( + temp.forget(), + media::NewRunnableFrom([this, that]() mutable { + LOG(("MediaManager shutdown lambda running, releasing MediaManager singleton and thread")); if (mMediaThread) { mMediaThread->Stop(); } - mBackend = nullptr; + // we hold a ref to 'that' which is the same as sSingleton + sSingleton = nullptr; + return NS_OK; }))); return NS_OK; @@ -2299,8 +2340,11 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic, } } + if (sInShutdown) { + return task->Denied(NS_LITERAL_STRING("In shutdown")); + } // Reuse the same thread to save memory. - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, task.forget()); + MediaManager::PostTask(FROM_HERE, task.forget()); return NS_OK; } else if (!strcmp(aTopic, "getUserMedia:response:deny")) { @@ -2498,8 +2542,7 @@ MediaManager::SanitizeDeviceIds(int64_t aSinceWhen) NS_ASSERTION(NS_IsMainThread(), "Only call on main thread"); LOG(("%s: sinceWhen = %llu", __FUNCTION__, aSinceWhen)); - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, - new SanitizeDeviceIdsTask(aSinceWhen)); + MediaManager::PostTask(FROM_HERE, new SanitizeDeviceIdsTask(aSinceWhen)); return NS_OK; } @@ -2616,12 +2659,10 @@ GetUserMediaCallbackMediaStreamListener::AudioConfig(bool aEchoOn, { if (mAudioSource) { #ifdef MOZ_WEBRTC - mMediaThread->message_loop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, NewRunnableMethod(mAudioSource.get(), &MediaEngineSource::Config, aEchoOn, aEcho, aAgcOn, aAGC, aNoiseOn, aNoise, aPlayoutDelay)); -#else - unused << mMediaThread; #endif } } @@ -2634,7 +2675,7 @@ GetUserMediaCallbackMediaStreamListener::Invalidate() // thread. // Pass a ref to us (which is threadsafe) so it can query us for the // source stream info. - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new MediaOperationTask(MEDIA_STOP, this, nullptr, nullptr, mAudioSource, mVideoSource, @@ -2652,7 +2693,7 @@ GetUserMediaCallbackMediaStreamListener::StopScreenWindowSharing() mVideoSource->GetMediaSource() == dom::MediaSourceEnum::Application || mVideoSource->GetMediaSource() == dom::MediaSourceEnum::Window)) { // Stop the whole stream if there's no audio; just the video track if we have both - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new MediaOperationTask(mAudioSource ? MEDIA_STOP_TRACK : MEDIA_STOP, this, nullptr, nullptr, nullptr, mVideoSource, @@ -2670,7 +2711,7 @@ GetUserMediaCallbackMediaStreamListener::StopTrack(TrackID aID, bool aIsAudio) { // XXX to support multiple tracks of a type in a stream, this should key off // the TrackID and not just the type - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new MediaOperationTask(MEDIA_STOP_TRACK, this, nullptr, nullptr, aIsAudio ? mAudioSource : nullptr, @@ -2696,7 +2737,7 @@ void GetUserMediaCallbackMediaStreamListener::NotifyDirectListeners(MediaStreamGraph* aGraph, bool aHasListeners) { - MediaManager::GetMessageLoop()->PostTask(FROM_HERE, + MediaManager::PostTask(FROM_HERE, new MediaOperationTask(MEDIA_DIRECT_LISTENERS, this, nullptr, nullptr, mAudioSource, mVideoSource, diff --git a/dom/media/MediaManager.h b/dom/media/MediaManager.h index 1333d0fc324..97276598df6 100644 --- a/dom/media/MediaManager.h +++ b/dom/media/MediaManager.h @@ -71,6 +71,7 @@ public: ~GetUserMediaCallbackMediaStreamListener() { + unused << mMediaThread; // It's OK to release mStream on any thread; they have thread-safe // refcounts. } @@ -514,7 +515,7 @@ public: // from MediaManager thread. static MediaManager* Get(); static MediaManager* GetIfExists(); - static MessageLoop* GetMessageLoop(); + static void PostTask(const tracked_objects::Location& from_here, Task* task); #ifdef DEBUG static bool IsInMediaThread(); #endif diff --git a/dom/media/webrtc/MediaEngine.h b/dom/media/webrtc/MediaEngine.h index 041c4e7c8f6..6d9cc4910ae 100644 --- a/dom/media/webrtc/MediaEngine.h +++ b/dom/media/webrtc/MediaEngine.h @@ -64,6 +64,8 @@ public: virtual void EnumerateAudioDevices(dom::MediaSourceEnum, nsTArray >*) = 0; + virtual void Shutdown() = 0; + protected: virtual ~MediaEngine() {} }; @@ -81,6 +83,8 @@ public: virtual ~MediaEngineSource() {} + virtual void Shutdown() = 0; + /* Populate the human readable name of this device in the nsAString */ virtual void GetName(nsAString&) = 0; @@ -245,6 +249,7 @@ public: /* This call reserves but does not start the device. */ virtual nsresult Allocate(const dom::MediaTrackConstraints &aConstraints, const MediaEnginePrefs &aPrefs) = 0; + protected: explicit MediaEngineAudioSource(MediaEngineState aState) : MediaEngineSource(aState) {} diff --git a/dom/media/webrtc/MediaEngineCameraVideoSource.h b/dom/media/webrtc/MediaEngineCameraVideoSource.h index 3079e828df6..3ea9ec039f5 100644 --- a/dom/media/webrtc/MediaEngineCameraVideoSource.h +++ b/dom/media/webrtc/MediaEngineCameraVideoSource.h @@ -61,6 +61,8 @@ public: uint32_t GetBestFitnessDistance( const nsTArray& aConstraintSets) override; + virtual void Shutdown() override {}; + protected: struct CapabilityCandidate { explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0) diff --git a/dom/media/webrtc/MediaEngineDefault.h b/dom/media/webrtc/MediaEngineDefault.h index 88a3a119844..e0accf09687 100644 --- a/dom/media/webrtc/MediaEngineDefault.h +++ b/dom/media/webrtc/MediaEngineDefault.h @@ -36,6 +36,8 @@ class MediaEngineDefaultVideoSource : public nsITimerCallback, public: MediaEngineDefaultVideoSource(); + virtual void Shutdown() override {}; + virtual void GetName(nsAString&) override; virtual void GetUUID(nsAString&) override; @@ -104,6 +106,8 @@ class MediaEngineDefaultAudioSource : public nsITimerCallback, public: MediaEngineDefaultAudioSource(); + virtual void Shutdown() override {}; + virtual void GetName(nsAString&) override; virtual void GetUUID(nsAString&) override; @@ -158,15 +162,23 @@ public: {} virtual void EnumerateVideoDevices(dom::MediaSourceEnum, - nsTArray >*); + nsTArray >*) override; virtual void EnumerateAudioDevices(dom::MediaSourceEnum, - nsTArray >*); + nsTArray >*) override; + virtual void Shutdown() { + MutexAutoLock lock(mMutex); + + mVSources.Clear(); + mASources.Clear(); + }; protected: bool mHasFakeTracks; private: - ~MediaEngineDefault() {} + ~MediaEngineDefault() { + Shutdown(); + } Mutex mMutex; // protected with mMutex: diff --git a/dom/media/webrtc/MediaEngineTabVideoSource.h b/dom/media/webrtc/MediaEngineTabVideoSource.h index e01cfb8635a..a02c4a0fae9 100644 --- a/dom/media/webrtc/MediaEngineTabVideoSource.h +++ b/dom/media/webrtc/MediaEngineTabVideoSource.h @@ -18,6 +18,7 @@ class MediaEngineTabVideoSource : public MediaEngineVideoSource, nsIDOMEventList NS_DECL_NSITIMERCALLBACK MediaEngineTabVideoSource(); + virtual void Shutdown() override {}; virtual void GetName(nsAString_internal&) override; virtual void GetUUID(nsAString_internal&) override; virtual nsresult Allocate(const dom::MediaTrackConstraints &, diff --git a/dom/media/webrtc/MediaEngineWebRTC.cpp b/dom/media/webrtc/MediaEngineWebRTC.cpp index 69036caa3b0..5a1cab054ee 100644 --- a/dom/media/webrtc/MediaEngineWebRTC.cpp +++ b/dom/media/webrtc/MediaEngineWebRTC.cpp @@ -274,10 +274,9 @@ MediaEngineWebRTC::EnumerateVideoDevices(dom::MediaSourceEnum aMediaSource, } } - if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource) + if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource) { aVSources->AppendElement(new MediaEngineTabVideoSource()); - - return; + } #endif } @@ -372,15 +371,43 @@ MediaEngineWebRTC::EnumerateAudioDevices(dom::MediaSourceEnum aMediaSource, } } +static PLDHashOperator +ClearVideoSource (const nsAString&, // unused + MediaEngineVideoSource* aData, + void *userArg) +{ + if (aData) { + aData->Shutdown(); + } + return PL_DHASH_NEXT; +} + +static PLDHashOperator +ClearAudioSource (const nsAString&, // unused + MediaEngineWebRTCAudioSource* aData, + void *userArg) +{ + if (aData) { + aData->Shutdown(); + } + return PL_DHASH_NEXT; +} + void MediaEngineWebRTC::Shutdown() { // This is likely paranoia MutexAutoLock lock(mMutex); - // Clear callbacks before we go away since the engines may outlive us + LOG(("%s", __FUNCTION__)); + // Shutdown all the sources, since we may have dangling references to the + // sources in nsDOMUserMediaStreams waiting for GC/CC + mVideoSources.EnumerateRead(ClearVideoSource, nullptr); + mAudioSources.EnumerateRead(ClearAudioSource, nullptr); mVideoSources.Clear(); mAudioSources.Clear(); + + // Clear callbacks before we go away since the engines may outlive us if (mVideoEngine) { mVideoEngine->SetTraceCallback(nullptr); webrtc::VideoEngine::Delete(mVideoEngine); diff --git a/dom/media/webrtc/MediaEngineWebRTC.h b/dom/media/webrtc/MediaEngineWebRTC.h index 7a8cac647fa..5d6f8cbebdb 100644 --- a/dom/media/webrtc/MediaEngineWebRTC.h +++ b/dom/media/webrtc/MediaEngineWebRTC.h @@ -109,19 +109,20 @@ public: void Refresh(int aIndex); + virtual void Shutdown() override; + protected: ~MediaEngineWebRTCVideoSource() { Shutdown(); } private: // Initialize the needed Video engine interfaces. void Init(); - void Shutdown(); // Engine variables. webrtc::VideoEngine* mVideoEngine; // Weak reference, don't free. - webrtc::ViEBase* mViEBase; - webrtc::ViECapture* mViECapture; - webrtc::ViERender* mViERender; + ScopedCustomReleasePtr mViEBase; + ScopedCustomReleasePtr mViECapture; + ScopedCustomReleasePtr mViERender; int mMinFps; // Min rate we want to accept dom::MediaSourceEnum mMediaSource; // source of media (camera | application | screen) @@ -195,12 +196,13 @@ public: NS_DECL_THREADSAFE_ISUPPORTS + virtual void Shutdown() override; + protected: ~MediaEngineWebRTCAudioSource() { Shutdown(); } private: void Init(); - void Shutdown(); webrtc::VoiceEngine* mVoiceEngine; ScopedCustomReleasePtr mVoEBase; @@ -239,12 +241,12 @@ public: // Clients should ensure to clean-up sources video/audio sources // before invoking Shutdown on this class. - void Shutdown(); + void Shutdown() override; virtual void EnumerateVideoDevices(dom::MediaSourceEnum, - nsTArray >*); + nsTArray>*) override; virtual void EnumerateAudioDevices(dom::MediaSourceEnum, - nsTArray >*); + nsTArray>*) override; private: ~MediaEngineWebRTC() { Shutdown(); diff --git a/dom/media/webrtc/MediaEngineWebRTCAudio.cpp b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp index f84b9ef2011..5b3cc1a993a 100644 --- a/dom/media/webrtc/MediaEngineWebRTCAudio.cpp +++ b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp @@ -478,11 +478,12 @@ MediaEngineWebRTCAudioSource::Shutdown() { if (!mInitDone) { // duplicate these here in case we failed during Init() - if (mChannel != -1) { + if (mChannel != -1 && mVoENetwork) { mVoENetwork->DeRegisterExternalTransport(mChannel); } delete mNullTransport; + mNullTransport = nullptr; return; } @@ -514,6 +515,7 @@ MediaEngineWebRTCAudioSource::Shutdown() } delete mNullTransport; + mNullTransport = nullptr; mVoEProcessing = nullptr; mVoENetwork = nullptr; diff --git a/dom/media/webrtc/MediaEngineWebRTCVideo.cpp b/dom/media/webrtc/MediaEngineWebRTCVideo.cpp index 5c19e8e09c4..1d2d1b6150d 100644 --- a/dom/media/webrtc/MediaEngineWebRTCVideo.cpp +++ b/dom/media/webrtc/MediaEngineWebRTCVideo.cpp @@ -266,7 +266,7 @@ MediaEngineWebRTCVideoSource::Deallocate() // another thread anywhere else, b) ViEInputManager::DestroyCaptureDevice() grabs // an exclusive object lock and deletes it in a critical section, so all in all // this should be safe threadwise. - NS_DispatchToMainThread(WrapRunnable(mViECapture, + NS_DispatchToMainThread(WrapRunnable(mViECapture.get(), &webrtc::ViECapture::ReleaseCaptureDevice, mCaptureIndex), NS_DISPATCH_SYNC); @@ -422,9 +422,10 @@ MediaEngineWebRTCVideoSource::Shutdown() if (mState == kAllocated || mState == kStopped) { Deallocate(); } - mViECapture->Release(); - mViERender->Release(); - mViEBase->Release(); + mViECapture = nullptr; + mViERender = nullptr; + mViEBase = nullptr; + mState = kReleased; mInitDone = false; }