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
This commit is contained in:
Byron Campen 2022-03-01 22:16:49 +00:00
parent 3c1761ab3b
commit 269cf7fcc0
2 changed files with 23 additions and 61 deletions

View File

@ -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() {

View File

@ -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<DataChannelConnection> 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<nsIObserverService> 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<nsIObserverService> 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<DataChannelRegistry>& Instance() {
static RefPtr<DataChannelRegistry> 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<DataChannelRegistry> 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<uintptr_t, RefPtr<DataChannelConnection>> mConnections;
UniquePtr<media::ShutdownBlockingTicket> 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) {