Bug 1626278: Use NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DESTROY to ensure that MediaTransportHandler stays around long enough to finish pending stats queries. r=mjf

Differential Revision: https://phabricator.services.mozilla.com/D87180

dom/media/webrtc/jsapi/MediaTransportHandler.cpp

@@ -33,6 +33,7 @@
 
 #include "mozilla/Algorithm.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/ClearOnShutdown.h"
 
 #include "mozilla/dom/RTCStatsReportBinding.h"
 
@@ -64,7 +65,6 @@ class MediaTransportHandlerSTS : public MediaTransportHandler,
   nsresult CreateIceCtx(const std::string& aName,
                         const nsTArray<dom::RTCIceServer>& aIceServers,
                         dom::RTCIceTransportPolicy aIcePolicy) override;
-  void Destroy() override;
 
   // We will probably be able to move the proxy lookup stuff into
   // this class once we move mtransport to its own process.
@@ -114,7 +114,13 @@ class MediaTransportHandlerSTS : public MediaTransportHandler,
   RefPtr<dom::RTCStatsPromise> GetIceStats(const std::string& aTransportId,
                                            DOMHighResTimeStamp aNow) override;
 
+  void Shutdown();
+
  private:
+  void Destroy() override;
+  void Destroy_s();
+  void DestroyFinal();
+  void Shutdown_s();
   RefPtr<TransportFlow> CreateTransportFlow(const std::string& aTransportId,
                                             bool aIsRtcp,
                                             RefPtr<DtlsIdentity> aDtlsIdentity,
@@ -183,6 +189,41 @@ already_AddRefed<MediaTransportHandler> MediaTransportHandler::Create(
   return result.forget();
 }
 
+class STSShutdownHandler {
+ public:
+  ~STSShutdownHandler() {
+    MOZ_ASSERT(NS_IsMainThread());
+    for (const auto& handler : mHandlers) {
+      handler->Shutdown();
+    }
+  }
+
+  void Register(MediaTransportHandlerSTS* aHandler) {
+    MOZ_ASSERT(NS_IsMainThread());
+    mHandlers.insert(aHandler);
+  }
+
+  void Deregister(MediaTransportHandlerSTS* aHandler) {
+    MOZ_ASSERT(NS_IsMainThread());
+    mHandlers.erase(aHandler);
+  }
+
+ private:
+  // Raw ptrs, registered on init, deregistered on destruction, all on main
+  std::set<MediaTransportHandlerSTS*> mHandlers;
+};
+
+static STSShutdownHandler* GetShutdownHandler() {
+  MOZ_ASSERT(NS_IsMainThread());
+  static UniquePtr<STSShutdownHandler> sHandler(new STSShutdownHandler);
+  static bool initted = false;
+  if (!initted) {
+    initted = true;
+    ClearOnShutdown(&sHandler, ShutdownPhase::WillShutdown);
+  }
+  return sHandler.get();
+}
+
 MediaTransportHandlerSTS::MediaTransportHandlerSTS(
     nsISerialEventTarget* aCallbackThread)
     : MediaTransportHandler(aCallbackThread) {
@@ -194,7 +235,7 @@ MediaTransportHandlerSTS::MediaTransportHandlerSTS(
 
   RLogConnector::CreateInstance();
 
-  CSFLogDebug(LOGTAG, "%s done", __func__);
+  CSFLogDebug(LOGTAG, "%s done %p", __func__, this);
 
   // We do not set up mDNSService here, because we are not running on main (we
   // use PBackground), and the DNS service asserts.
@@ -443,6 +484,8 @@ nsresult MediaTransportHandlerSTS::CreateIceCtx(
         config.mPolicy = toNrIcePolicy(aIcePolicy);
         config.mNatSimulatorConfig = GetNatConfig();
 
+        GetShutdownHandler()->Register(this);
+
         return InvokeAsync(
             mStsThread, __func__,
             [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
@@ -499,49 +542,96 @@ nsresult MediaTransportHandlerSTS::CreateIceCtx(
   return NS_OK;
 }
 
-void MediaTransportHandlerSTS::Destroy() {
+void MediaTransportHandlerSTS::Shutdown() {
-  if (!mInitPromise) {
+  CSFLogDebug(LOGTAG, "%s", __func__);
+  if (!mStsThread->IsOnCurrentThread()) {
+    mStsThread->Dispatch(NewNonOwningRunnableMethod(
+        __func__, this, &MediaTransportHandlerSTS::Shutdown_s));
     return;
   }
 
-  mInitPromise->Then(
+  Shutdown_s();
-      mStsThread, __func__,
-      [this, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
-        disconnect_all();
-        // Clear the transports before destroying the ice ctx so that
-        // the close_notify alerts have a chance to be sent as the
-        // TransportFlow destructors execute.
-        mTransports.clear();
-        if (mIceCtx) {
-          // We're already on the STS thread, but the TransportFlow
-          // destructors executed when mTransports.clear() is called
-          // above dispatch calls to DestroyFinal to the STS thread. If
-          // we don't also dispatch the call to destory the NrIceCtx to
-          // the STS thread, it will tear down the NrIceMediaStreams
-          // before the TransportFlows are destroyed.  Without a valid
-          // NrIceMediaStreams the close_notify alert cannot be sent.
-          mStsThread->Dispatch(NS_NewRunnableFunction(
-              __func__, [iceCtx = RefPtr<NrIceCtx>(mIceCtx)] {
-                NrIceStats stats = iceCtx->Destroy();
-                CSFLogDebug(LOGTAG,
-                            "Ice Telemetry: stun (retransmits: %d)"
-                            "   turn (401s: %d   403s: %d   438s: %d)",
-                            stats.stun_retransmits, stats.turn_401s,
-                            stats.turn_403s, stats.turn_438s);
-              }));
-
-          mIceCtx = nullptr;
-        }
-      },
-      [](const std::string& aError) {});
 }
 
+void MediaTransportHandlerSTS::Shutdown_s() {
+  CSFLogDebug(LOGTAG, "%s", __func__);
+  disconnect_all();
+  // Clear the transports before destroying the ice ctx so that
+  // the close_notify alerts have a chance to be sent as the
+  // TransportFlow destructors execute.
+  mTransports.clear();
+  if (mIceCtx) {
+    // We're already on the STS thread, but the TransportFlow
+    // destructors executed when mTransports.clear() is called
+    // above dispatch calls to DestroyFinal to the STS thread. If
+    // we don't also dispatch the call to destory the NrIceCtx to
+    // the STS thread, it will tear down the NrIceMediaStreams
+    // before the TransportFlows are destroyed.  Without a valid
+    // NrIceMediaStreams the close_notify alert cannot be sent.
+    mStsThread->Dispatch(
+        NS_NewRunnableFunction(__func__, [iceCtx = RefPtr<NrIceCtx>(mIceCtx)] {
+          NrIceStats stats = iceCtx->Destroy();
+          CSFLogDebug(LOGTAG,
+                      "Ice Telemetry: stun (retransmits: %d)"
+                      "   turn (401s: %d   403s: %d   438s: %d)",
+                      stats.stun_retransmits, stats.turn_401s, stats.turn_403s,
+                      stats.turn_438s);
+        }));
+  }
+  mIceCtx = nullptr;
+}
+
+void MediaTransportHandlerSTS::Destroy() {
+  CSFLogDebug(LOGTAG, "%s %p", __func__, this);
+  // Our "destruction tour" starts on main, because we need to deregister.
+  if (!NS_IsMainThread()) {
+    GetMainThreadEventTarget()->Dispatch(NewNonOwningRunnableMethod(
+        __func__, this, &MediaTransportHandlerSTS::Destroy));
+    return;
+  }
+
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!GetShutdownHandler()) {
+    // Already shut down. Nothing else to do.
+    delete this;
+    return;
+  }
+
+  GetShutdownHandler()->Deregister(this);
+
+  // mIceCtx still has a reference to us via sigslot! We must dispach to STS,
+  // and clean up there. However, by the time _that_ happens, we may have
+  // dispatched a signal callback to mCallbackThread, so we have to dispatch
+  // the final destruction to mCallbackThread.
+  mStsThread->Dispatch(NewNonOwningRunnableMethod(
+      __func__, this, &MediaTransportHandlerSTS::Destroy_s));
+}
+
+void MediaTransportHandlerSTS::Destroy_s() {
+  Shutdown_s();
+  if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
+    nsresult rv = mCallbackThread->Dispatch(NewNonOwningRunnableMethod(
+        __func__, this, &MediaTransportHandlerSTS::DestroyFinal));
+    if (NS_SUCCEEDED(rv)) {
+      return;
+    }
+  }
+
+  DestroyFinal();
+}
+
+void MediaTransportHandlerSTS::DestroyFinal() { delete this; }
+
 void MediaTransportHandlerSTS::SetProxyConfig(
     NrSocketProxyConfig&& aProxyConfig) {
   mInitPromise->Then(
       mStsThread, __func__,
       [this, self = RefPtr<MediaTransportHandlerSTS>(this),
        aProxyConfig = std::move(aProxyConfig)]() mutable {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         mIceCtx->SetProxyConfig(std::move(aProxyConfig));
       },
       [](const std::string& aError) {});
@@ -553,6 +643,10 @@ void MediaTransportHandlerSTS::EnsureProvisionalTransport(
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         RefPtr<NrIceMediaStream> stream(mIceCtx->GetStream(aTransportId));
         if (!stream) {
           CSFLogDebug(LOGTAG, "%s: Creating ICE media stream=%s components=%u",
@@ -593,6 +687,10 @@ void MediaTransportHandlerSTS::ActivateTransport(
       mStsThread, __func__,
       [=, keyDer = aKeyDer.Clone(), certDer = aCertDer.Clone(),
        self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         MOZ_ASSERT(aComponentCount);
         RefPtr<DtlsIdentity> dtlsIdentity(
             DtlsIdentity::Deserialize(keyDer, certDer, aAuthType));
@@ -673,6 +771,10 @@ void MediaTransportHandlerSTS::SetTargetForDefaultLocalAddressLookup(
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         mIceCtx->SetTargetForDefaultLocalAddressLookup(aTargetIp, aTargetPort);
       },
       [](const std::string& aError) {});
@@ -685,6 +787,10 @@ void MediaTransportHandlerSTS::StartIceGathering(
       mStsThread, __func__,
       [=, stunAddrs = aStunAddrs.Clone(),
        self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         mObfuscateHostAddresses = aObfuscateHostAddresses;
 
         // Belt and suspenders - in e10s mode, the call below to SetStunAddrs
@@ -722,6 +828,10 @@ void MediaTransportHandlerSTS::StartIceChecks(
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         nsresult rv = mIceCtx->ParseGlobalAttributes(aIceOptions);
         if (NS_FAILED(rv)) {
           CSFLogError(LOGTAG, "%s: couldn't parse global parameters",
@@ -763,6 +873,10 @@ void MediaTransportHandlerSTS::AddIceCandidate(
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         std::vector<std::string> tokens;
         TokenizeCandidate(aCandidate, tokens);
 
@@ -798,6 +912,10 @@ void MediaTransportHandlerSTS::UpdateNetworkState(bool aOnline) {
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         mIceCtx->UpdateNetworkState(aOnline);
       },
       [](const std::string& aError) {});
@@ -808,6 +926,10 @@ void MediaTransportHandlerSTS::RemoveTransportsExcept(
   mInitPromise->Then(
       mStsThread, __func__,
       [=, self = RefPtr<MediaTransportHandlerSTS>(this)]() {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         for (auto it = mTransports.begin(); it != mTransports.end();) {
           const std::string transportId(it->first);
           if (!aTransportIds.count(transportId)) {
@@ -845,6 +967,10 @@ void MediaTransportHandlerSTS::SendPacket(const std::string& aTransportId,
       mStsThread, __func__,
       [this, self = RefPtr<MediaTransportHandlerSTS>(this), aTransportId,
        aPacket = std::move(aPacket)]() mutable {
+        if (!mIceCtx) {
+          return;  // Probably due to XPCOM shutdown
+        }
+
         MOZ_ASSERT(aPacket.type() != MediaPacket::UNCLASSIFIED);
         RefPtr<TransportFlow> flow =
             GetTransportFlow(aTransportId, aPacket.type() == MediaPacket::RTCP);
@@ -907,10 +1033,11 @@ TransportLayer::State MediaTransportHandler::GetState(
 void MediaTransportHandler::OnCandidate(const std::string& aTransportId,
                                         const CandidateInfo& aCandidateInfo) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
-    mCallbackThread->Dispatch(WrapRunnable(RefPtr<MediaTransportHandler>(this),
+    mCallbackThread->Dispatch(
-                                           &MediaTransportHandler::OnCandidate,
+        // This is being called from sigslot, which does not hold a strong ref.
-                                           aTransportId, aCandidateInfo),
+        WrapRunnable(this, &MediaTransportHandler::OnCandidate, aTransportId,
-                              NS_DISPATCH_NORMAL);
+                     aCandidateInfo),
+        NS_DISPATCH_NORMAL);
     return;
   }
 
@@ -920,8 +1047,8 @@ void MediaTransportHandler::OnCandidate(const std::string& aTransportId,
 void MediaTransportHandler::OnAlpnNegotiated(const std::string& aAlpn) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnAlpnNegotiated, aAlpn),
+        WrapRunnable(this, &MediaTransportHandler::OnAlpnNegotiated, aAlpn),
         NS_DISPATCH_NORMAL);
     return;
   }
@@ -934,8 +1061,9 @@ void MediaTransportHandler::OnGatheringStateChange(
     dom::RTCIceGatheringState aState) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnGatheringStateChange, aState),
+        WrapRunnable(this, &MediaTransportHandler::OnGatheringStateChange,
+                     aState),
         NS_DISPATCH_NORMAL);
     return;
   }
@@ -947,8 +1075,9 @@ void MediaTransportHandler::OnConnectionStateChange(
     dom::RTCIceConnectionState aState) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnConnectionStateChange, aState),
+        WrapRunnable(this, &MediaTransportHandler::OnConnectionStateChange,
+                     aState),
         NS_DISPATCH_NORMAL);
     return;
   }
@@ -960,9 +1089,9 @@ void MediaTransportHandler::OnPacketReceived(const std::string& aTransportId,
                                              const MediaPacket& aPacket) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnPacketReceived, aTransportId,
+        WrapRunnable(this, &MediaTransportHandler::OnPacketReceived,
-                     const_cast<MediaPacket&>(aPacket)),
+                     aTransportId, const_cast<MediaPacket&>(aPacket)),
         NS_DISPATCH_NORMAL);
     return;
   }
@@ -974,9 +1103,9 @@ void MediaTransportHandler::OnEncryptedSending(const std::string& aTransportId,
                                                const MediaPacket& aPacket) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnEncryptedSending, aTransportId,
+        WrapRunnable(this, &MediaTransportHandler::OnEncryptedSending,
-                     const_cast<MediaPacket&>(aPacket)),
+                     aTransportId, const_cast<MediaPacket&>(aPacket)),
         NS_DISPATCH_NORMAL);
     return;
   }
@@ -988,8 +1117,8 @@ void MediaTransportHandler::OnStateChange(const std::string& aTransportId,
                                           TransportLayer::State aState) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnStateChange, aTransportId,
+        WrapRunnable(this, &MediaTransportHandler::OnStateChange, aTransportId,
                      aState),
         NS_DISPATCH_NORMAL);
     return;
@@ -1007,9 +1136,9 @@ void MediaTransportHandler::OnRtcpStateChange(const std::string& aTransportId,
                                               TransportLayer::State aState) {
   if (mCallbackThread && !mCallbackThread->IsOnCurrentThread()) {
     mCallbackThread->Dispatch(
-        WrapRunnable(RefPtr<MediaTransportHandler>(this),
+        // This is being called from sigslot, which does not hold a strong ref.
-                     &MediaTransportHandler::OnRtcpStateChange, aTransportId,
+        WrapRunnable(this, &MediaTransportHandler::OnRtcpStateChange,
-                     aState),
+                     aTransportId, aState),
         NS_DISPATCH_NORMAL);
     return;
   }

dom/media/webrtc/jsapi/MediaTransportHandler.h

@@ -63,8 +63,6 @@ class MediaTransportHandler {
   virtual void EnterPrivateMode() = 0;
   virtual void ExitPrivateMode() = 0;
 
-  virtual void Destroy() = 0;
-
   virtual nsresult CreateIceCtx(const std::string& aName,
                                 const nsTArray<dom::RTCIceServer>& aIceServers,
                                 dom::RTCIceTransportPolicy aIcePolicy) = 0;
@@ -129,7 +127,9 @@ class MediaTransportHandler {
   sigslot::signal2<const std::string&, TransportLayer::State> SignalStateChange;
   sigslot::signal2<const std::string&, TransportLayer::State>
       SignalRtcpStateChange;
-  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaTransportHandler)
+
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DESTROY(MediaTransportHandler,
+                                                     Destroy())
 
   TransportLayer::State GetState(const std::string& aTransportId,
                                  bool aRtcp) const;
@@ -148,6 +148,7 @@ class MediaTransportHandler {
                      TransportLayer::State aState);
   void OnRtcpStateChange(const std::string& aTransportId,
                          TransportLayer::State aState);
+  virtual void Destroy() = 0;
   virtual ~MediaTransportHandler() = default;
   std::map<std::string, TransportLayer::State> mStateCache;
   std::map<std::string, TransportLayer::State> mRtcpStateCache;

dom/media/webrtc/jsapi/MediaTransportHandlerIPC.cpp

@@ -152,15 +152,11 @@ nsresult MediaTransportHandlerIPC::CreateIceCtx(
 }
 
 void MediaTransportHandlerIPC::Destroy() {
-  mInitPromise->Then(
+  if (mChild) {
-      mCallbackThread, __func__,
+    MediaTransportChild::Send__delete__(mChild);
-      [=, self = RefPtr<MediaTransportHandlerIPC>(this)](bool /*dummy*/) {
+    mChild = nullptr;
-        if (mChild) {
+  }
-          MediaTransportChild::Send__delete__(mChild);
+  delete this;
-          mChild = nullptr;
-        }
-      },
-      [](const nsCString& aError) {});
 }
 
 // We will probably be able to move the proxy lookup stuff into

dom/media/webrtc/jsapi/MediaTransportHandlerIPC.h

@@ -25,7 +25,6 @@ class MediaTransportHandlerIPC : public MediaTransportHandler {
   nsresult CreateIceCtx(const std::string& aName,
                         const nsTArray<dom::RTCIceServer>& aIceServers,
                         dom::RTCIceTransportPolicy aIcePolicy) override;
-  void Destroy() override;
 
   // We will probably be able to move the proxy lookup stuff into
   // this class once we move mtransport to its own process.
@@ -76,6 +75,7 @@ class MediaTransportHandlerIPC : public MediaTransportHandler {
 
  private:
   friend class MediaTransportChild;
+  void Destroy() override;
 
   // We do not own this; it will tell us when it is going away.
   dom::PMediaTransportChild* mChild = nullptr;

dom/media/webrtc/jsapi/MediaTransportParent.cpp

@@ -37,7 +37,6 @@ class MediaTransportParent::Impl : public sigslot::has_slots<> {
 
   virtual ~Impl() {
     disconnect_all();
-    mHandler->Destroy();
     mHandler = nullptr;
   }
 

dom/media/webrtc/jsapi/PeerConnectionCtx.cpp

@@ -354,7 +354,7 @@ nsresult PeerConnectionCtx::Cleanup() {
 
   mQueuedJSEPOperations.Clear();
   mGMPService = nullptr;
-  mTransportHandler->Destroy();
+  mTransportHandler = nullptr;
   return NS_OK;
 }
 

dom/media/webrtc/jsapi/PeerConnectionMedia.cpp

@@ -641,7 +641,7 @@ void PeerConnectionMedia::ShutdownMediaTransport_s() {
 
   disconnect_all();
 
-  mTransportHandler->Destroy();
+  mTransportHandler->RemoveTransportsExcept(std::set<std::string>());
   mTransportHandler = nullptr;
 
   // we're holding a ref to 'this' that's released by SelfDestruct_m