From 269cf7fcc0d1fb7fe5695045fc595ed6ebb44ff7 Mon Sep 17 00:00:00 2001 From: Byron Campen Date: Tue, 1 Mar 2022 22:16:49 +0000 Subject: [PATCH] Bug 1755318: Fix issue where the order of PeerConnectionCtx shutdown and DataChannel shutdown was unpredictable. r=mjf When the pc.close() calls in PeerConnection.jsm were removed, tests that did not explicitly call pc.close() would result in situations where DataChannel shutdown ran first, which resulted in DataChannels hanging around after DataChannel ran its global shutdown code. So, we stop using xpcom-shutdown to prompt DataChannel to shut down, and instead use ShutdownBlockingTicket to ensure that shutdown does not proceed until DataChannel is cleaned up, which naturally results from the PeerConnectionCtx shutdown code. However, ShutdownBlockingTicket blocks xpcom-will-shutdown, not xpcom-shutdown, which resulted in PeerConnectionCtx not getting an opportunity to run its shutdown code, which then led to shutdown hangs. So, PeerConnectionCtx runs its shutdown code at xpcom-will-shutdown. Differential Revision: https://phabricator.services.mozilla.com/D139210 --- dom/media/webrtc/jsapi/PeerConnectionCtx.cpp | 11 +-- netwerk/sctp/datachannel/DataChannel.cpp | 73 +++++--------------- 2 files changed, 23 insertions(+), 61 deletions(-) diff --git a/dom/media/webrtc/jsapi/PeerConnectionCtx.cpp b/dom/media/webrtc/jsapi/PeerConnectionCtx.cpp index 24bf227d6701..9d7db8322693 100644 --- a/dom/media/webrtc/jsapi/PeerConnectionCtx.cpp +++ b/dom/media/webrtc/jsapi/PeerConnectionCtx.cpp @@ -207,7 +207,7 @@ class PeerConnectionCtxObserver : public nsIObserver { nsresult rv = NS_OK; - rv = observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, + rv = observerService->AddObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID, false); MOZ_ALWAYS_SUCCEEDS(rv); rv = observerService->AddObserver(this, NS_IOSERVICE_OFFLINE_STATUS_TOPIC, @@ -218,7 +218,7 @@ class PeerConnectionCtxObserver : public nsIObserver { NS_IMETHOD Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData) override { - if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) { + if (strcmp(aTopic, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID) == 0) { CSFLogDebug(LOGTAG, "Shutting down PeerConnectionCtx"); PeerConnectionCtx::Destroy(); @@ -229,7 +229,8 @@ class PeerConnectionCtxObserver : public nsIObserver { nsresult rv = observerService->RemoveObserver( this, NS_IOSERVICE_OFFLINE_STATUS_TOPIC); MOZ_ALWAYS_SUCCEEDS(rv); - rv = observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID); + rv = observerService->RemoveObserver(this, + NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID); MOZ_ALWAYS_SUCCEEDS(rv); // Make sure we're not deleted while still inside ::Observe() @@ -257,7 +258,7 @@ class PeerConnectionCtxObserver : public nsIObserver { services::GetObserverService(); if (observerService) { observerService->RemoveObserver(this, NS_IOSERVICE_OFFLINE_STATUS_TOPIC); - observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID); + observerService->RemoveObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID); } } }; @@ -537,7 +538,7 @@ static void GMPReady_m() { static void GMPReady() { GetMainThreadEventTarget()->Dispatch(WrapRunnableNM(&GMPReady_m), - NS_DISPATCH_NORMAL); + NS_DISPATCH_NORMAL); }; void PeerConnectionCtx::initGMP() { diff --git a/netwerk/sctp/datachannel/DataChannel.cpp b/netwerk/sctp/datachannel/DataChannel.cpp index cca63ee5374e..5747f9bc494c 100644 --- a/netwerk/sctp/datachannel/DataChannel.cpp +++ b/netwerk/sctp/datachannel/DataChannel.cpp @@ -32,8 +32,6 @@ #include "nsServiceManagerUtils.h" #include "nsIInputStream.h" -#include "nsIObserverService.h" -#include "nsIObserver.h" #include "nsIPrefBranch.h" #include "nsIPrefService.h" #include "mozilla/Services.h" @@ -49,6 +47,7 @@ #include "mozilla/Unused.h" #include "mozilla/dom/RTCDataChannelBinding.h" #include "mozilla/dom/RTCStatsReportBinding.h" +#include "mozilla/media/MediaUtils.h" #ifdef MOZ_PEERCONNECTION # include "transport/runnable_utils.h" # include "jsapi/MediaTransportHandler.h" @@ -95,9 +94,9 @@ static void debug_printf(const char* format, ...) { } } -class DataChannelRegistry : public nsIObserver { +class DataChannelRegistry { public: - NS_DECL_THREADSAFE_ISUPPORTS + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataChannelRegistry) static uintptr_t Register(DataChannelConnection* aConnection) { StaticMutexAutoLock lock(sInstanceMutex); @@ -124,10 +123,6 @@ class DataChannelRegistry : public nsIObserver { maybeTrash = Instance().forget(); } } - - if (maybeTrash) { - maybeTrash->Shutdown(); - } } static RefPtr Lookup(uintptr_t aId) { @@ -142,30 +137,12 @@ class DataChannelRegistry : public nsIObserver { // This is a singleton class, so don't let just anyone create one of these DataChannelRegistry() { ASSERT_WEBRTC(NS_IsMainThread()); - nsCOMPtr observerService = - mozilla::services::GetObserverService(); - if (!observerService) return; - - nsresult rv = - observerService->AddObserver(this, "xpcom-will-shutdown", false); - MOZ_ASSERT(rv == NS_OK); - (void)rv; + mShutdownBlocker = media::ShutdownBlockingTicket::Create( + u"DataChannelRegistry::mShutdownBlocker"_ns, + NS_LITERAL_STRING_FROM_CSTRING(__FILE__), __LINE__); InitUsrSctp(); } - nsresult Shutdown() { - DeinitUsrSctp(); - nsCOMPtr observerService = - mozilla::services::GetObserverService(); - if (NS_WARN_IF(!observerService)) { - return NS_ERROR_FAILURE; - } - - nsresult rv = observerService->RemoveObserver(this, "xpcom-will-shutdown"); - MOZ_ASSERT(rv == NS_OK); - return rv; - } - static RefPtr& Instance() { static RefPtr sRegistry; return sRegistry; @@ -179,30 +156,6 @@ class DataChannelRegistry : public nsIObserver { return Instance(); } - NS_IMETHOD Observe(nsISupports* aSubject, const char* aTopic, - const char16_t* aData) override { - ASSERT_WEBRTC(NS_IsMainThread()); - if (strcmp(aTopic, "xpcom-will-shutdown") == 0) { - RefPtr self = this; - { - StaticMutexAutoLock lock(sInstanceMutex); - Instance() = nullptr; - } - - // |self| and the reference being held onto by the observer service are - // the only ones left now. - - if (NS_WARN_IF(!mConnections.empty())) { - MOZ_ASSERT(false); - mConnections.clear(); - } - - return Shutdown(); - } - - return NS_OK; - } - uintptr_t RegisterImpl(DataChannelConnection* aConnection) { ASSERT_WEBRTC(NS_IsMainThread()); mConnections.emplace(mNextId, aConnection); @@ -225,7 +178,16 @@ class DataChannelRegistry : public nsIObserver { return it->second; } - virtual ~DataChannelRegistry() = default; + virtual ~DataChannelRegistry() { + ASSERT_WEBRTC(NS_IsMainThread()); + + if (NS_WARN_IF(!mConnections.empty())) { + MOZ_ASSERT(false); + mConnections.clear(); + } + + DeinitUsrSctp(); + } #ifdef SCTP_DTLS_SUPPORTED static int SctpDtlsOutput(void* addr, void* buffer, size_t length, @@ -279,13 +241,12 @@ class DataChannelRegistry : public nsIObserver { uintptr_t mNextId = 1; std::map> mConnections; + UniquePtr mShutdownBlocker; static StaticMutex sInstanceMutex; }; StaticMutex DataChannelRegistry::sInstanceMutex; -NS_IMPL_ISUPPORTS(DataChannelRegistry, nsIObserver); - OutgoingMsg::OutgoingMsg(struct sctp_sendv_spa& info, const uint8_t* data, size_t length) : mLength(length), mData(data) {