Bug 834383: Ensure PeerConnectionImpl destructor doesn't use globals after they're gone r=jesup,bsmith
☠☠ backed out by 9545909e8283 ☠ ☠
authorAdam Roach [:abr] <adam@nostrum.com>
Wed, 30 Jan 2013 15:31:22 -0600
changeset 130369 70872c02094446ad2045c2eb5f5633bcaffbc94e
parent 130368 eefb9fb2af7860d23519eb8c66e42c3f1f018070
child 130370 c0dc5d403329c8757542e29ac7eaeb7f6e4b1eb1
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, bsmith
bugs834383
milestone21.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 834383: Ensure PeerConnectionImpl destructor doesn't use globals after they're gone r=jesup,bsmith
media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
--- a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
+++ b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ -985,19 +985,24 @@ static short vcmGetDtlsIdentity_m(const 
   digestp[0]='\0';
 
   sipcc::PeerConnectionWrapper pc(peerconnection);
   ENSURE_PC(pc, VCM_ERROR);
 
   unsigned char digest[TransportLayerDtls::kMaxDigestLength];
   size_t digest_len;
 
-  nsresult res = pc.impl()->GetIdentity()->ComputeFingerprint("sha-256", digest,
-                                                               sizeof(digest),
-                                                               &digest_len);
+  mozilla::RefPtr<DtlsIdentity> id = pc.impl()->GetIdentity();
+
+  if (!id) {
+    return VCM_ERROR;
+  }
+
+  nsresult res = id->ComputeFingerprint("sha-256", digest, sizeof(digest),
+                                        &digest_len);
   if (!NS_SUCCEEDED(res)) {
     CSFLogError( logTag, "%s: Could not compute identity fingerprint", __FUNCTION__);
     return VCM_ERROR;
   }
 
   // digest_len should be 32 for SHA-256
   PR_ASSERT(digest_len == 32);
   std::string fingerprint_txt = DtlsIdentity::FormatFingerprint(digest, digest_len);
@@ -2635,17 +2640,21 @@ vcmCreateTransportFlow(sipcc::PeerConnec
     //
     // Currently we just hardwire the roles to be that the offerer is the
     // server, which is what you would expect from the "recommended"
     // behavior above.
     //
     // TODO(ekr@rtfm.com): implement the actpass logic above.
     dtls->SetRole(pc->GetRole() == sipcc::PeerConnectionImpl::kRoleOfferer ?
                   TransportLayerDtls::SERVER : TransportLayerDtls::CLIENT);
-    dtls->SetIdentity(pc->GetIdentity());
+    mozilla::RefPtr<DtlsIdentity> pcid = pc->GetIdentity();
+    if (!pcid) {
+      return nullptr;
+    }
+    dtls->SetIdentity(pcid);
 
     unsigned char remote_digest[TransportLayerDtls::kMaxDigestLength];
     size_t digest_len;
 
     nsresult res = DtlsIdentity::ParseFingerprint(fingerprint,
                                                   remote_digest,
                                                   sizeof(remote_digest),
                                                   &digest_len);
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ -133,16 +133,20 @@ nsresult PeerConnectionCtx::InitializeGl
   return NS_OK;
 }
 
 PeerConnectionCtx* PeerConnectionCtx::GetInstance() {
   MOZ_ASSERT(gInstance);
   return gInstance;
 }
 
+bool PeerConnectionCtx::isActive() {
+  return gInstance;
+}
+
 void PeerConnectionCtx::Destroy() {
   CSFLogDebug(logTag, "%s", __FUNCTION__);
 
   if (gInstance) {
     gInstance->Cleanup();
     delete gInstance;
     gInstance = NULL;
   }
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h
@@ -26,16 +26,17 @@ namespace sipcc {
 // * The global PeerConnectionImpl table and its associated lock.
 // * Currently SIPCC only allows a single stack instance to exist in a process
 //   at once. This class implements a singleton object that wraps that.
 // * The observer class that demuxes events onto individual PCs.
 class PeerConnectionCtx : public CSF::CC_Observer {
  public:
   static nsresult InitializeGlobal(nsIThread *mainThread);
   static PeerConnectionCtx* GetInstance();
+  static bool isActive();
   static void Destroy();
 
   // Implementations of CC_Observer methods
   virtual void onDeviceEvent(ccapi_device_event_e deviceEvent, CSF::CC_DevicePtr device, CSF::CC_DeviceInfoPtr info);
   virtual void onFeatureEvent(ccapi_device_event_e deviceEvent, CSF::CC_DevicePtr device, CSF::CC_FeatureInfoPtr feature_info) {}
   virtual void onLineEvent(ccapi_line_event_e lineEvent, CSF::CC_LinePtr line, CSF::CC_LineInfoPtr info) {}
   virtual void onCallEvent(ccapi_call_event_e callEvent, CSF::CC_CallPtr call, CSF::CC_CallInfoPtr info);
 
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -237,19 +237,29 @@ PeerConnectionImpl::PeerConnectionImpl()
 #ifdef MOZILLA_INTERNAL_API
   MOZ_ASSERT(NS_IsMainThread());
 #endif
 }
 
 PeerConnectionImpl::~PeerConnectionImpl()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
-  PeerConnectionCtx::GetInstance()->mPeerConnections.erase(mHandle);
+  if (PeerConnectionCtx::isActive()) {
+    PeerConnectionCtx::GetInstance()->mPeerConnections.erase(mHandle);
+  } else {
+    CSFLogErrorS(logTag, "PeerConnectionCtx is already gone. Ignoring...");
+  }
+
   CloseInt(false);
 
+#ifdef MOZILLA_INTERNAL_API
+  // Deregister as an NSS Shutdown Object
+  shutdown(calledFromObject);
+#endif
+
   // Since this and Initialize() occur on MainThread, they can't both be
   // running at once
 
   // Right now, we delete PeerConnectionCtx at XPCOM shutdown only, but we
   // probably want to shut it down more aggressively to save memory.  We
   // could shut down here when there are no uses.  It might be more optimal
   // to release off a timer (and XPCOM Shutdown) to avoid churn
 
@@ -1130,16 +1140,30 @@ PeerConnectionImpl::ShutdownMedia(bool a
 
   // Forget the reference so that we can transfer it to
   // SelfDestruct().
   RUN_ON_THREAD(mThread, WrapRunnable(mMedia.forget().get(),
                                       &PeerConnectionMedia::SelfDestruct),
                 aIsSynchronous ? NS_DISPATCH_SYNC : NS_DISPATCH_NORMAL);
 }
 
+#ifdef MOZILLA_INTERNAL_API
+// If NSS is shutting down, then we need to get rid of the DTLS
+// identity right now; otherwise, we'll cause wreckage when we do
+// finally deallocate it in our destructor.
+void
+PeerConnectionImpl::virtualDestroyNSSReference()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  CSFLogDebugS(logTag, __FUNCTION__ << ": "
+               << "NSS shutting down; freeing our DtlsIdentity.");
+  mIdentity = nullptr;
+}
+#endif
+
 void
 PeerConnectionImpl::onCallEvent(ccapi_call_event_e aCallEvent,
                                 CSF::CC_CallPtr aCall, CSF::CC_CallInfoPtr aInfo)
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
   MOZ_ASSERT(aCall.get());
   MOZ_ASSERT(aInfo.get());
 
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -29,16 +29,17 @@
 #include "PeerConnectionMedia.h"
 
 #ifdef MOZILLA_INTERNAL_API
 #include "mozilla/net/DataChannel.h"
 #include "Layers.h"
 #include "VideoUtils.h"
 #include "ImageLayers.h"
 #include "VideoSegment.h"
+#include "nsNSSShutDown.h"
 #else
 namespace mozilla {
   class DataChannel;
 }
 #endif
 
 using namespace mozilla;
 
@@ -93,16 +94,17 @@ class PeerConnectionWrapper;
       if (NS_FAILED(res)) return res; \
     } while(0)
 #define PC_AUTO_ENTER_API_CALL_NO_CHECK() CheckThread()
 
 
 class PeerConnectionImpl MOZ_FINAL : public IPeerConnection,
 #ifdef MOZILLA_INTERNAL_API
                                      public mozilla::DataChannelConnection::DataConnectionListener,
+                                     public nsNSSShutDownObject,
 #endif
                                      public sigslot::has_slots<>
 {
 public:
   PeerConnectionImpl();
   ~PeerConnectionImpl();
 
   enum ReadyState {
@@ -244,16 +246,20 @@ private:
     // TODO(ekr@rtfm.com): Fix the unit tests so they don't do that.
     bool on;
     NS_ENSURE_SUCCESS(mThread->IsOnCurrentThread(&on), false);
     NS_ENSURE_TRUE(on, false);
 #endif
     return true;
   }
 
+#ifdef MOZILLA_INTERNAL_API
+  void virtualDestroyNSSReference() MOZ_FINAL;
+#endif
+
   // Shut down media. Called on any thread.
   void ShutdownMedia(bool isSynchronous);
 
   // ICE callbacks run on the right thread.
   nsresult IceGatheringCompleted_m(NrIceCtx *aCtx);
   nsresult IceCompleted_m(NrIceCtx *aCtx);
 
   // The role we are adopting