This is an automated email from the git hooks/post-receive script.
richard pushed a commit to branch geckoview-102.3.0esr-12.0-1 in repository tor-browser.
commit d03f74a543ed4bfd7e1d97735720b1477096df10 Author: David Parks daparks@mozilla.com AuthorDate: Fri Jul 29 19:32:14 2022 +0000
Bug 1755700: Simplify ownership of AudioSession r=cmartin, a=RyanVM
Concurrent operations in the MTA make the required lifetime of the AudioSession complex. In particular, it cannot be known when/if any methods are currently queued or being executed (in particular, Start). In order to make this safe, we keep the AudioSession at least as long as XPCOM is running background threads.
This patch also removes a long-defunct state machine and does some basic code cleanup.
Differential Revision: https://phabricator.services.mozilla.com/D152300 --- widget/windows/AudioSession.cpp | 212 +++++++++++++++------------------------- 1 file changed, 78 insertions(+), 134 deletions(-)
diff --git a/widget/windows/AudioSession.cpp b/widget/windows/AudioSession.cpp index cd04834c9f5da..c696f2f50af21 100644 --- a/widget/windows/AudioSession.cpp +++ b/widget/windows/AudioSession.cpp @@ -9,7 +9,10 @@ #include <windows.h> #include <mmdeviceapi.h>
+#include "mozilla/ClearOnShutdown.h" #include "mozilla/RefPtr.h" +#include "mozilla/ScopeExit.h" +#include "mozilla/StaticPtr.h" #include "nsIStringBundle.h"
#include "nsCOMPtr.h" @@ -33,6 +36,8 @@ namespace widget { */ class AudioSession final : public IAudioSessionEvents { public: + AudioSession(); + static AudioSession* GetSingleton();
// COM IUnknown @@ -54,29 +59,16 @@ class AudioSession final : public IAudioSessionEvents {
void Start(); void Stop(); - void StopInternal(); - void InitializeAudioSession();
nsresult GetSessionData(nsID& aID, nsString& aSessionName, nsString& aIconPath); nsresult SetSessionData(const nsID& aID, const nsString& aSessionName, const nsString& aIconPath);
- enum SessionState { - UNINITIALIZED, // Has not been initialized yet - STARTED, // Started - CLONED, // SetSessionInfoCalled, Start not called - FAILED, // The audio session failed to start - STOPPED, // Stop called - AUDIO_SESSION_DISCONNECTED // Audio session disconnected - }; - - SessionState mState; - private: - AudioSession(); - ~AudioSession(); - nsresult CommitAudioSessionData(); + ~AudioSession() = default; + + void StopInternal(const MutexAutoLock& aProofOfLock);
protected: RefPtr<IAudioSessionControl> mAudioSessionControl; @@ -90,44 +82,38 @@ class AudioSession final : public IAudioSessionEvents { NS_DECL_OWNINGTHREAD };
-static std::atomic<AudioSession*> sService = nullptr; +StaticRefPtr<AudioSession> sService;
void StartAudioSession() { - AudioSession::GetSingleton()->InitializeAudioSession(); - NS_DispatchBackgroundTask(NS_NewRunnableFunction( - "StartAudioSession", - []() -> void { AudioSession::GetSingleton()->Start(); })); + MOZ_ASSERT(NS_IsMainThread()); + MOZ_ASSERT(!sService); + sService = new AudioSession(); + + // Destroy AudioSession only after any background task threads have been + // stopped or abandoned. + ClearOnShutdown(&sService, ShutdownPhase::XPCOMShutdownFinal); + + NS_DispatchBackgroundTask( + NS_NewCancelableRunnableFunction("StartAudioSession", []() -> void { + MOZ_ASSERT(AudioSession::GetSingleton(), + "AudioSession should outlive background threads"); + AudioSession::GetSingleton()->Start(); + })); }
void StopAudioSession() { - RefPtr<AudioSession> audioSession; - AudioSession* temp = sService; - audioSession.swap(temp); - sService = nullptr; - - if (audioSession) { - NS_DispatchBackgroundTask(NS_NewRunnableFunction( - "StopAudioSession", - [audioSession]() -> void { audioSession->Stop(); })); - } -} - -AudioSession::AudioSession() : mMutex("AudioSessionControl") { - mState = UNINITIALIZED; + MOZ_ASSERT(NS_IsMainThread()); + MOZ_ASSERT(sService); + NS_DispatchBackgroundTask( + NS_NewRunnableFunction("StopAudioSession", []() -> void { + MOZ_ASSERT(AudioSession::GetSingleton(), + "AudioSession should outlive background threads"); + AudioSession::GetSingleton()->Stop(); + })); }
-AudioSession::~AudioSession() {} - AudioSession* AudioSession::GetSingleton() { - if (!sService) { - RefPtr<AudioSession> service = new AudioSession(); - AudioSession* temp = nullptr; - service.swap(temp); - sService = temp; - } - - // We don't refcount AudioSession on the Gecko side, we hold one single ref - // as long as the appshell is running. + MOZ_ASSERT(mscom::IsCurrentThreadMTA()); return sService; }
@@ -147,7 +133,7 @@ AudioSession::QueryInterface(REFIID iid, void** ppv) { return E_NOINTERFACE; }
-void AudioSession::InitializeAudioSession() { +AudioSession::AudioSession() : mMutex("AudioSessionControl") { // This func must be run on the main thread as // nsStringBundle is not thread safe otherwise MOZ_ASSERT(NS_IsMainThread()); @@ -155,8 +141,6 @@ void AudioSession::InitializeAudioSession() { MOZ_ASSERT(XRE_IsParentProcess(), "Should only get here in a chrome process!");
- if (mState != UNINITIALIZED) return; - nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID); MOZ_ASSERT(bundleService); @@ -181,25 +165,25 @@ void AudioSession::InitializeAudioSession() { // calls Stop. void AudioSession::Start() { MOZ_ASSERT(mscom::IsCurrentThreadMTA()); - MOZ_ASSERT(mState == UNINITIALIZED || mState == CLONED || - mState == AUDIO_SESSION_DISCONNECTED, - "State invariants violated");
const CLSID CLSID_MMDeviceEnumerator = __uuidof(MMDeviceEnumerator); const IID IID_IMMDeviceEnumerator = __uuidof(IMMDeviceEnumerator); const IID IID_IAudioSessionManager = __uuidof(IAudioSessionManager);
- HRESULT hr; - - mState = FAILED; - + MutexAutoLock lock(mMutex); + MOZ_ASSERT(!mAudioSessionControl); MOZ_ASSERT(!mDisplayName.IsEmpty() || !mIconPath.IsEmpty(), "Should never happen ...");
+ auto scopeExit = MakeScopeExit([&] { StopInternal(lock); }); + RefPtr<IMMDeviceEnumerator> enumerator; - hr = ::CoCreateInstance(CLSID_MMDeviceEnumerator, nullptr, CLSCTX_ALL, - IID_IMMDeviceEnumerator, getter_AddRefs(enumerator)); - if (FAILED(hr)) return; + HRESULT hr = + ::CoCreateInstance(CLSID_MMDeviceEnumerator, nullptr, CLSCTX_ALL, + IID_IMMDeviceEnumerator, getter_AddRefs(enumerator)); + if (FAILED(hr)) { + return; + }
RefPtr<IMMDevice> device; hr = enumerator->GetDefaultAudioEndpoint( @@ -215,50 +199,59 @@ void AudioSession::Start() { return; }
- MutexAutoLock lock(mMutex); hr = manager->GetAudioSessionControl(&GUID_NULL, 0, getter_AddRefs(mAudioSessionControl));
- if (FAILED(hr)) { + if (FAILED(hr) || !mAudioSessionControl) { return; }
// Increments refcount of 'this'. hr = mAudioSessionControl->RegisterAudioSessionNotification(this); if (FAILED(hr)) { - StopInternal(); return; }
- CommitAudioSessionData(); - mState = STARTED; -} + hr = mAudioSessionControl->SetGroupingParam( + (LPGUID) & (mSessionGroupingParameter), nullptr); + if (FAILED(hr)) { + return; + }
-void AudioSession::StopInternal() { - mMutex.AssertCurrentThreadOwns(); - - if (mAudioSessionControl && (mState == STARTED || mState == STOPPED)) { - // Decrement refcount of 'this' - mAudioSessionControl->UnregisterAudioSessionNotification(this); - // Deleting this COM object seems to require the STA / main thread. - // Audio code may concurrently be running on the main thread and it may - // block waiting for this to complete, creating deadlock. So we destroy the - // object on the main thread instead. - NS_DispatchToMainThread(NS_NewRunnableFunction( - "ShutdownAudioSession", - [asc = std::move(mAudioSessionControl)] { /* */ })); + hr = mAudioSessionControl->SetDisplayName(mDisplayName.get(), nullptr); + if (FAILED(hr)) { + return; } + + hr = mAudioSessionControl->SetIconPath(mIconPath.get(), nullptr); + if (FAILED(hr)) { + return; + } + + scopeExit.release(); }
void AudioSession::Stop() { - MOZ_ASSERT(mState == STARTED || mState == UNINITIALIZED || // XXXremove this - mState == FAILED, - "State invariants violated"); MOZ_ASSERT(mscom::IsCurrentThreadMTA());
MutexAutoLock lock(mMutex); - mState = STOPPED; - StopInternal(); + StopInternal(lock); +} + +void AudioSession::StopInternal(const MutexAutoLock& aProofOfLock) { + if (!mAudioSessionControl) { + return; + } + + // Decrement refcount of 'this' + mAudioSessionControl->UnregisterAudioSessionNotification(this); + + // Deleting the IAudioSessionControl COM object requires the STA/main thread. + // Audio code may concurrently be running on the main thread and it may + // block waiting for this to complete, creating deadlock. So we destroy the + // IAudioSessionControl on the main thread instead. + NS_DispatchToMainThread(NS_NewRunnableFunction( + "FreeAudioSession", [asc = std::move(mAudioSessionControl)] { /* */ })); }
void CopynsID(nsID& lhs, const nsID& rhs) { @@ -272,62 +265,24 @@ void CopynsID(nsID& lhs, const nsID& rhs) {
nsresult AudioSession::GetSessionData(nsID& aID, nsString& aSessionName, nsString& aIconPath) { - MOZ_ASSERT(mState == FAILED || mState == STARTED || mState == CLONED, - "State invariants violated"); - CopynsID(aID, mSessionGroupingParameter); aSessionName = mDisplayName; aIconPath = mIconPath;
- if (mState == FAILED) return NS_ERROR_FAILURE; - return NS_OK; }
nsresult AudioSession::SetSessionData(const nsID& aID, const nsString& aSessionName, const nsString& aIconPath) { - MOZ_ASSERT(mState == UNINITIALIZED, "State invariants violated"); MOZ_ASSERT(!XRE_IsParentProcess(), "Should never get here in a chrome process!"); - mState = CLONED; - CopynsID(mSessionGroupingParameter, aID); mDisplayName = aSessionName; mIconPath = aIconPath; return NS_OK; }
-nsresult AudioSession::CommitAudioSessionData() { - mMutex.AssertCurrentThreadOwns(); - - if (!mAudioSessionControl) { - // Stop() was called before we had a chance to do this. - return NS_OK; - } - - HRESULT hr = mAudioSessionControl->SetGroupingParam( - (LPGUID) & (mSessionGroupingParameter), nullptr); - if (FAILED(hr)) { - StopInternal(); - return NS_ERROR_FAILURE; - } - - hr = mAudioSessionControl->SetDisplayName(mDisplayName.get(), nullptr); - if (FAILED(hr)) { - StopInternal(); - return NS_ERROR_FAILURE; - } - - hr = mAudioSessionControl->SetIconPath(mIconPath.get(), nullptr); - if (FAILED(hr)) { - StopInternal(); - return NS_ERROR_FAILURE; - } - - return NS_OK; -} - STDMETHODIMP AudioSession::OnChannelVolumeChanged(DWORD aChannelCount, float aChannelVolumeArray[], @@ -352,19 +307,8 @@ AudioSession::OnIconPathChanged(LPCWSTR aIconPath, LPCGUID aContext) {
STDMETHODIMP AudioSession::OnSessionDisconnected(AudioSessionDisconnectReason aReason) { - { - MutexAutoLock lock(mMutex); - if (!mAudioSessionControl) return S_OK; - mAudioSessionControl->UnregisterAudioSessionNotification(this); - // Deleting this COM object seems to require the STA / main thread. - // Audio code may concurrently be running on the main thread and it may - // block waiting for this to complete, creating deadlock. So we destroy the - // object on the main thread instead. - NS_DispatchToMainThread(NS_NewRunnableFunction( - "FreeAudioSession", [asc = std::move(mAudioSessionControl)] { /* */ })); - mState = AUDIO_SESSION_DISCONNECTED; - } - Start(); // If it fails there's not much we can do. + Stop(); + Start(); return S_OK; }