From 6146db617bef9e185c9e22f4aed7c49d0765f5aa Mon Sep 17 00:00:00 2001 From: Narcis Beleuzu Date: Wed, 16 Feb 2022 18:14:02 +0200 Subject: [PATCH] Backed out changeset e1b946a41694 (bug 1617283) for audio related crashes and hangs (bug 1755700, bug 1755717). a=backout --- build/win32/orderfile.txt | 1 + build/win64/orderfile.txt | 1 + dom/ipc/ContentChild.cpp | 5 + toolkit/xre/nsAppRunner.cpp | 8 +- tools/lint/mscom-init.yml | 1 + widget/windows/AudioSession.cpp | 229 ++++++++++++++++++++------------ widget/windows/AudioSession.h | 13 +- widget/windows/nsAppShell.cpp | 5 + 8 files changed, 172 insertions(+), 91 deletions(-) diff --git a/build/win32/orderfile.txt b/build/win32/orderfile.txt index 2c420bb1bc2e..dca462f833e0 100644 --- a/build/win32/orderfile.txt +++ b/build/win32/orderfile.txt @@ -6180,6 +6180,7 @@ XPCOMService_GetSocketTransportService ?BroadcastStringBundle@ContentParent@dom@mozilla@@SAXABVStringBundleDescriptor@23@@Z ?Get@SharedStringMap@ipc@dom@mozilla@@QAE_NABV?$nsTString@D@@AAV?$nsTSubstring@_S@@@Z ?ReinitForContent@ImageBridgeChild@layers@mozilla@@SA_N$$QAV?$Endpoint@VPImageBridgeChild@layers@mozilla@@@ipc@3@I@Z +?RecvAudioSessionData@widget@mozilla@@YA?AW4nsresult@@ABUnsID@@ABV?$nsTString@_S@@1@Z ?Release@WakeLock@dom@mozilla@@UAGKXZ ?GetInstance@PowerManagerService@power@dom@mozilla@@SA?AU?$already_AddRefed@VPowerManagerService@power@dom@mozilla@@@@XZ ?RegisterWakeLockObserver@hal@mozilla@@YAXPAV?$Observer@VWakeLockInformation@hal@mozilla@@@2@@Z diff --git a/build/win64/orderfile.txt b/build/win64/orderfile.txt index e3b35dbe6496..87f0af561f60 100644 --- a/build/win64/orderfile.txt +++ b/build/win64/orderfile.txt @@ -6464,6 +6464,7 @@ ZN11encoding_rs3mem17utf16_valid_up_to17h89dc8b8f218f8fa1E ?BroadcastStringBundle@ContentParent@dom@mozilla@@SAXAEBVStringBundleDescriptor@23@@Z ?Get@SharedStringMap@ipc@dom@mozilla@@QEAA_NAEBV?$nsTString@D@@AEAV?$nsTSubstring@_S@@@Z ?GetAndResetReleaseFence@RenderCompositor@wr@mozilla@@UEAA?AVFileDescriptor@ipc@3@XZ +?RecvAudioSessionData@widget@mozilla@@YA?AW4nsresult@@AEBUnsID@@AEBV?$nsTString@_S@@1@Z ?Release@WakeLock@dom@mozilla@@UEAAKXZ ?GetInstance@PowerManagerService@power@dom@mozilla@@SA?AU?$already_AddRefed@VPowerManagerService@power@dom@mozilla@@@@XZ ?RegisterWakeLockObserver@hal@mozilla@@YAXPEAV?$Observer@VWakeLockInformation@hal@mozilla@@@2@@Z diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp index 50c02a6fef91..6fea572c25ac 100644 --- a/dom/ipc/ContentChild.cpp +++ b/dom/ipc/ContentChild.cpp @@ -228,6 +228,7 @@ # include # define getpid _getpid # include "mozilla/WinDllServices.h" +# include "mozilla/widget/AudioSession.h" # include "mozilla/widget/WinContentSystemParameters.h" #endif @@ -3059,6 +3060,10 @@ void ContentChild::ShutdownInternal() { os->NotifyObservers(ToSupports(this), "content-child-shutdown", nullptr); } +#if defined(XP_WIN) + mozilla::widget::StopAudioSession(); +#endif + GetIPCChannel()->SetAbortOnError(false); if (mProfilerController) { diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp index f3ff1e2edb92..eeef21445a59 100644 --- a/toolkit/xre/nsAppRunner.cpp +++ b/toolkit/xre/nsAppRunner.cpp @@ -5903,10 +5903,6 @@ int XREMain::XRE_main(int argc, char* argv[], const BootstrapConfig& aConfig) { // run! rv = XRE_mainRun(); -#if defined(XP_WIN) - mozilla::widget::StopAudioSession(); -#endif - #ifdef MOZ_INSTRUMENT_EVENT_LOOP mozilla::ShutdownEventTracing(); #endif @@ -5924,6 +5920,10 @@ int XREMain::XRE_main(int argc, char* argv[], const BootstrapConfig& aConfig) { mScopedXPCOM = nullptr; +#if defined(XP_WIN) + mozilla::widget::StopAudioSession(); +#endif + // unlock the profile after ScopedXPCOMStartup object (xpcom) // has gone out of scope. see bug #386739 for more details mProfileLock->Unlock(); diff --git a/tools/lint/mscom-init.yml b/tools/lint/mscom-init.yml index 58c012359d96..b3a05e617e6f 100644 --- a/tools/lint/mscom-init.yml +++ b/tools/lint/mscom-init.yml @@ -37,6 +37,7 @@ forbid-mscom-init: - toolkit/crashreporter/google-breakpad/src/common/windows/pdb_source_line_writer.cc - toolkit/mozapps/defaultagent/main.cpp - uriloader/exthandler/win/nsOSHelperAppService.cpp + - widget/windows/AudioSession.cpp - widget/windows/InkCollector.cpp - widget/windows/TaskbarPreview.cpp - widget/windows/WinTaskbar.cpp diff --git a/widget/windows/AudioSession.cpp b/widget/windows/AudioSession.cpp index 672422205142..fce78d96e99b 100644 --- a/widget/windows/AudioSession.cpp +++ b/widget/windows/AudioSession.cpp @@ -11,6 +11,7 @@ #include "mozilla/RefPtr.h" #include "nsIStringBundle.h" +//#include "AudioSession.h" #include "nsCOMPtr.h" #include "nsID.h" #include "nsServiceManagerUtils.h" @@ -18,10 +19,11 @@ #include "nsThreadUtils.h" #include "nsXULAppAPI.h" #include "mozilla/Attributes.h" -#include "mozilla/mscom/Utils.h" #include "mozilla/Mutex.h" #include "mozilla/WindowsVersion.h" +#include + namespace mozilla { namespace widget { @@ -31,6 +33,10 @@ namespace widget { * and implements IAudioSessionEvents (for callbacks from Windows) */ class AudioSession final : public IAudioSessionEvents { + private: + AudioSession(); + ~AudioSession(); + public: static AudioSession* GetSingleton(); @@ -47,17 +53,23 @@ class AudioSession final : public IAudioSessionEvents { STDMETHODIMP OnGroupingParamChanged(LPCGUID aGroupingParam, LPCGUID aContext); STDMETHODIMP OnIconPathChanged(LPCWSTR aIconPath, LPCGUID aContext); STDMETHODIMP OnSessionDisconnected(AudioSessionDisconnectReason aReason); + + private: + nsresult OnSessionDisconnectedInternal(); + nsresult CommitAudioSessionData(); + + public: STDMETHODIMP OnSimpleVolumeChanged(float aVolume, BOOL aMute, LPCGUID aContext); STDMETHODIMP OnStateChanged(AudioSessionState aState); - void Start(); - void Stop(); + nsresult Start(); + nsresult Stop(); void StopInternal(); - void InitializeAudioSession(); nsresult GetSessionData(nsID& aID, nsString& aSessionName, nsString& aIconPath); + nsresult SetSessionData(const nsID& aID, const nsString& aSessionName, const nsString& aIconPath); @@ -70,44 +82,39 @@ class AudioSession final : public IAudioSessionEvents { AUDIO_SESSION_DISCONNECTED // Audio session disconnected }; - SessionState mState; - - private: - AudioSession(); - ~AudioSession(); - nsresult CommitAudioSessionData(); - protected: RefPtr mAudioSessionControl; nsString mDisplayName; nsString mIconPath; nsID mSessionGroupingParameter; + SessionState mState; // Guards the IAudioSessionControl mozilla::Mutex mMutex; ThreadSafeAutoRefCnt mRefCnt; NS_DECL_OWNINGTHREAD + + static AudioSession* sService; }; -static AudioSession* sService = nullptr; +nsresult StartAudioSession() { return AudioSession::GetSingleton()->Start(); } -void StartAudioSession() { - AudioSession::GetSingleton()->InitializeAudioSession(); - NS_DispatchBackgroundTask(NS_NewRunnableFunction( - "StartAudioSession", - []() -> void { AudioSession::GetSingleton()->Start(); })); +nsresult StopAudioSession() { return AudioSession::GetSingleton()->Stop(); } + +nsresult GetAudioSessionData(nsID& aID, nsString& aSessionName, + nsString& aIconPath) { + return AudioSession::GetSingleton()->GetSessionData(aID, aSessionName, + aIconPath); } -void StopAudioSession() { - RefPtr audioSession; - audioSession.swap(sService); - if (audioSession) { - NS_DispatchBackgroundTask(NS_NewRunnableFunction( - "StopAudioSession", - [audioSession]() -> void { audioSession->Stop(); })); - } +nsresult RecvAudioSessionData(const nsID& aID, const nsString& aSessionName, + const nsString& aIconPath) { + return AudioSession::GetSingleton()->SetSessionData(aID, aSessionName, + aIconPath); } +AudioSession* AudioSession::sService = nullptr; + AudioSession::AudioSession() : mMutex("AudioSessionControl") { mState = UNINITIALIZED; } @@ -115,14 +122,14 @@ AudioSession::AudioSession() : mMutex("AudioSessionControl") { AudioSession::~AudioSession() {} AudioSession* AudioSession::GetSingleton() { - if (!sService) { + if (!(AudioSession::sService)) { RefPtr service = new AudioSession(); - service.forget(&sService); + service.forget(&AudioSession::sService); } // We don't refcount AudioSession on the Gecko side, we hold one single ref // as long as the appshell is running. - return sService; + return AudioSession::sService; } // It appears Windows will use us on a background thread ... @@ -141,40 +148,10 @@ AudioSession::QueryInterface(REFIID iid, void** ppv) { return E_NOINTERFACE; } -void AudioSession::InitializeAudioSession() { - // This func must be run on the main thread as - // nsStringBundle is not thread safe otherwise - MOZ_ASSERT(NS_IsMainThread()); - - MOZ_ASSERT(XRE_IsParentProcess(), - "Should only get here in a chrome process!"); - - if (mState != UNINITIALIZED) return; - - nsCOMPtr bundleService = - do_GetService(NS_STRINGBUNDLE_CONTRACTID); - MOZ_ASSERT(bundleService); - - nsCOMPtr bundle; - bundleService->CreateBundle("chrome://branding/locale/brand.properties", - getter_AddRefs(bundle)); - MOZ_ASSERT(bundle); - bundle->GetStringFromName("brandFullName", mDisplayName); - - wchar_t* buffer; - mIconPath.GetMutableData(&buffer, MAX_PATH); - ::GetModuleFileNameW(nullptr, buffer, MAX_PATH); - - [[maybe_unused]] nsresult rv = - nsID::GenerateUUIDInPlace(mSessionGroupingParameter); - MOZ_ASSERT(rv == NS_OK); -} - // Once we are started Windows will hold a reference to us through our // IAudioSessionEvents interface that will keep us alive until the appshell // calls Stop. -void AudioSession::Start() { - MOZ_ASSERT(mscom::IsCurrentThreadMTA()); +nsresult AudioSession::Start() { MOZ_ASSERT(mState == UNINITIALIZED || mState == CLONED || mState == AUDIO_SESSION_DISCONNECTED, "State invariants violated"); @@ -185,6 +162,41 @@ void AudioSession::Start() { HRESULT hr; + // There's a matching CoUninit in Stop() for this tied to a state of + // UNINITIALIZED. + hr = CoInitialize(nullptr); + MOZ_ASSERT(SUCCEEDED(hr), + "CoInitialize failure in audio session control, unexpected"); + + if (mState == UNINITIALIZED) { + mState = FAILED; + + // Content processes should be CLONED + if (XRE_IsContentProcess()) { + return NS_ERROR_FAILURE; + } + + MOZ_ASSERT(XRE_IsParentProcess(), + "Should only get here in a chrome process!"); + + nsCOMPtr bundleService = + do_GetService(NS_STRINGBUNDLE_CONTRACTID); + NS_ENSURE_TRUE(bundleService, NS_ERROR_FAILURE); + nsCOMPtr bundle; + bundleService->CreateBundle("chrome://branding/locale/brand.properties", + getter_AddRefs(bundle)); + NS_ENSURE_TRUE(bundle, NS_ERROR_FAILURE); + + bundle->GetStringFromName("brandFullName", mDisplayName); + + wchar_t* buffer; + mIconPath.GetMutableData(&buffer, MAX_PATH); + ::GetModuleFileNameW(nullptr, buffer, MAX_PATH); + + nsresult rv = nsID::GenerateUUIDInPlace(mSessionGroupingParameter); + NS_ENSURE_SUCCESS(rv, rv); + } + mState = FAILED; MOZ_ASSERT(!mDisplayName.IsEmpty() || !mIconPath.IsEmpty(), @@ -193,21 +205,21 @@ void AudioSession::Start() { RefPtr enumerator; hr = ::CoCreateInstance(CLSID_MMDeviceEnumerator, nullptr, CLSCTX_ALL, IID_IMMDeviceEnumerator, getter_AddRefs(enumerator)); - if (FAILED(hr)) return; + if (FAILED(hr)) return NS_ERROR_NOT_AVAILABLE; RefPtr device; hr = enumerator->GetDefaultAudioEndpoint( EDataFlow::eRender, ERole::eMultimedia, getter_AddRefs(device)); if (FAILED(hr)) { - if (hr == E_NOTFOUND) return; - return; + if (hr == E_NOTFOUND) return NS_ERROR_NOT_AVAILABLE; + return NS_ERROR_FAILURE; } RefPtr manager; hr = device->Activate(IID_IAudioSessionManager, CLSCTX_ALL, nullptr, getter_AddRefs(manager)); if (FAILED(hr)) { - return; + return NS_ERROR_FAILURE; } MutexAutoLock lock(mMutex); @@ -215,18 +227,44 @@ void AudioSession::Start() { getter_AddRefs(mAudioSessionControl)); if (FAILED(hr)) { - return; + return NS_ERROR_FAILURE; } // Increments refcount of 'this'. hr = mAudioSessionControl->RegisterAudioSessionNotification(this); if (FAILED(hr)) { StopInternal(); - return; + return NS_ERROR_FAILURE; } - CommitAudioSessionData(); + nsCOMPtr runnable = + NewRunnableMethod("AudioSession::CommitAudioSessionData", this, + &AudioSession::CommitAudioSessionData); + NS_DispatchToMainThread(runnable); + mState = STARTED; + + return NS_OK; +} + +void SpawnASCReleaseThread(RefPtr&& aASC) { + // Fake moving to the other thread by circumventing the ref count. + // (RefPtrs don't play well with C++11 lambdas and we don't want to use + // XPCOM here.) + IAudioSessionControl* rawPtr = nullptr; + aASC.forget(&rawPtr); + MOZ_ASSERT(rawPtr); + PRThread* thread = PR_CreateThread( + PR_USER_THREAD, + [](void* aRawPtr) { + NS_SetCurrentThreadName("AudioASCReleaser"); + static_cast(aRawPtr)->Release(); + }, + rawPtr, PR_PRIORITY_NORMAL, PR_LOCAL_THREAD, PR_UNJOINABLE_THREAD, 0); + if (!thread) { + // We can't make a thread so just destroy the IAudioSessionControl here. + rawPtr->Release(); + } } void AudioSession::StopInternal() { @@ -238,25 +276,32 @@ void AudioSession::StopInternal() { } if (mAudioSessionControl) { - // Release cannot be called on mAudioSessionControl directly because - // RefPtr::operator-> is marked MOZ_NO_ADDREF_RELEASE_ON_RETURN - // See Bug 1156084 - IAudioSessionControl* rawPtr = nullptr; - mAudioSessionControl.forget(&rawPtr); - MOZ_ASSERT(rawPtr); - rawPtr->Release(); + // Avoid hanging when destroying AudioSessionControl. We do that by + // moving the AudioSessionControl to a worker thread (that we never + // 'join') for destruction. + SpawnASCReleaseThread(std::move(mAudioSessionControl)); } } -void AudioSession::Stop() { +nsresult AudioSession::Stop() { MOZ_ASSERT(mState == STARTED || mState == UNINITIALIZED || // XXXremove this mState == FAILED, "State invariants violated"); - MOZ_ASSERT(mscom::IsCurrentThreadMTA()); - - MutexAutoLock lock(mMutex); + SessionState state = mState; mState = STOPPED; - StopInternal(); + + { + RefPtr kungFuDeathGrip; + kungFuDeathGrip.swap(sService); + + MutexAutoLock lock(mMutex); + StopInternal(); + } + + if (state != UNINITIALIZED) { + ::CoUninitialize(); + } + return NS_OK; } void CopynsID(nsID& lhs, const nsID& rhs) { @@ -297,7 +342,7 @@ nsresult AudioSession::SetSessionData(const nsID& aID, } nsresult AudioSession::CommitAudioSessionData() { - mMutex.AssertCurrentThreadOwns(); + MutexAutoLock lock(mMutex); if (!mAudioSessionControl) { // Stop() was called before we had a chance to do this. @@ -305,7 +350,7 @@ nsresult AudioSession::CommitAudioSessionData() { } HRESULT hr = mAudioSessionControl->SetGroupingParam( - (LPGUID) & (mSessionGroupingParameter), nullptr); + (LPGUID)&mSessionGroupingParameter, nullptr); if (FAILED(hr)) { StopInternal(); return NS_ERROR_FAILURE; @@ -350,21 +395,35 @@ AudioSession::OnIconPathChanged(LPCWSTR aIconPath, LPCGUID aContext) { STDMETHODIMP AudioSession::OnSessionDisconnected(AudioSessionDisconnectReason aReason) { + // Run our code asynchronously. Per MSDN we can't do anything interesting + // in this callback. + nsCOMPtr runnable = + NewRunnableMethod("widget::AudioSession::OnSessionDisconnectedInternal", + this, &AudioSession::OnSessionDisconnectedInternal); + NS_DispatchToMainThread(runnable); + return S_OK; +} + +nsresult AudioSession::OnSessionDisconnectedInternal() { // When successful, UnregisterAudioSessionNotification will decrement the // refcount of 'this'. Start will re-increment it. In the interim, // we'll need to reference ourselves. + RefPtr kungFuDeathGrip(this); - // We need to release the mutex before we call Start(). { + // We need to release the mutex before we call Start(). MutexAutoLock lock(mMutex); - if (!mAudioSessionControl) return S_OK; + + if (!mAudioSessionControl) return NS_OK; + mAudioSessionControl->UnregisterAudioSessionNotification(this); mAudioSessionControl = nullptr; - mState = AUDIO_SESSION_DISCONNECTED; } - Start(); // If it fails there's not much we can do. - return S_OK; + mState = AUDIO_SESSION_DISCONNECTED; + CoUninitialize(); + Start(); // If it fails there's not much we can do. + return NS_OK; } STDMETHODIMP diff --git a/widget/windows/AudioSession.h b/widget/windows/AudioSession.h index 612a5e209b58..f715ec5458ec 100644 --- a/widget/windows/AudioSession.h +++ b/widget/windows/AudioSession.h @@ -10,10 +10,19 @@ namespace mozilla { namespace widget { // Start the audio session in the current process -void StartAudioSession(); +nsresult StartAudioSession(); + +// Pass the information necessary to start an audio session in another process +nsresult GetAudioSessionData(nsID& aID, nsString& aSessionName, + nsString& aIconPath); + +// Receive the information necessary to start an audio session in a non-chrome +// process +nsresult RecvAudioSessionData(const nsID& aID, const nsString& aSessionName, + const nsString& aIconPath); // Stop the audio session in the current process -void StopAudioSession(); +nsresult StopAudioSession(); } // namespace widget } // namespace mozilla diff --git a/widget/windows/nsAppShell.cpp b/widget/windows/nsAppShell.cpp index 22b070e9f53c..15d0d149c48e 100644 --- a/widget/windows/nsAppShell.cpp +++ b/widget/windows/nsAppShell.cpp @@ -585,6 +585,11 @@ nsresult nsAppShell::Init() { NS_IMETHODIMP nsAppShell::Run(void) { + // Content processes initialize audio later through PContent using audio + // tray id information pulled from the browser process AudioSession. This + // way the two share a single volume control. + // Note StopAudioSession() is called from nsAppRunner.cpp after xpcom is torn + // down to insure the browser shuts down after child processes. if (XRE_IsParentProcess()) { mozilla::widget::StartAudioSession(); }