Bug 824919: Weaken key references to PeerConnection and friends,r=jesup,smaug,ekr
☠☠ backed out by 12c12ba53d39 ☠ ☠
authorAdam Roach [:abr] <adam@nostrum.com>
Mon, 14 Jan 2013 17:00:20 -0600
changeset 118835 cebb008a72f9a08b37f68c34d4ccd51137e7a978
parent 118834 79addb03eb8672db2be1251846a234696550e316
child 118836 88c5a108a7fd1eaff565d162bc09118952338a5a
push id24180
push useremorley@mozilla.com
push dateTue, 15 Jan 2013 22:58:27 +0000
treeherdermozilla-central@72e34ce7fd92 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, smaug, ekr
bugs824919
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 824919: Weaken key references to PeerConnection and friends,r=jesup,smaug,ekr
dom/media/PeerConnection.js
media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
media/webrtc/signaling/test/Makefile.in
media/webrtc/signaling/test/signaling_unittests.cpp
--- a/dom/media/PeerConnection.js
+++ b/dom/media/PeerConnection.js
@@ -51,49 +51,62 @@ GlobalPCList.prototype = {
       }
       return _globalPCList.QueryInterface(iid);
     }
   },
 
   addPC: function(pc) {
     let winID = pc._winID;
     if (this._list[winID]) {
-      this._list[winID].push(pc);
+      this._list[winID].push(Components.utils.getWeakReference(pc));
     } else {
-      this._list[winID] = [pc];
+      this._list[winID] = [Components.utils.getWeakReference(pc)];
     }
+    this.removeNullRefs(winID);
+  },
+
+  removeNullRefs: function(winID) {
+    this._list[winID] = this._list[winID].filter(
+      function (e,i,a) { return e.get() !== null; });
   },
 
   hasActivePeerConnection: function(winID) {
+    this.removeNullRefs(winID);
     return this._list[winID] ? true : false;
   },
 
   observe: function(subject, topic, data) {
     if (topic == "inner-window-destroyed") {
       let winID = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
       if (this._list[winID]) {
-        this._list[winID].forEach(function(pc) {
-          pc._pc.close(false);
-          delete pc._observer;
-          pc._pc = null;
+        this._list[winID].forEach(function(pcref) {
+          let pc = pcref.get();
+          if (pc !== null) {
+            pc._pc.close(false);
+            delete pc._observer;
+            pc._pc = null;
+          }
         });
         delete this._list[winID];
       }
     } else if (topic == "profile-change-net-teardown" ||
                topic == "network:offline-about-to-go-offline") {
       // Delete all peerconnections on shutdown - synchronously (we need
       // them to be done deleting transports before we return)!
       // Also kill them if "Work Offline" is selected - more can be created
       // while offline, but attempts to connect them should fail.
       let array;
       while ((array = this._list.pop()) != undefined) {
-        array.forEach(function(pc) {
-          pc._pc.close(true);
-          delete pc._observer;
-          pc._pc = null;
+        array.forEach(function(pcref) {
+          let pc = pcref.get();
+          if (pc !== null) {
+            pc._pc.close(true);
+            delete pc._observer;
+            pc._pc = null;
+          }
         });
       };
       this._networkdown = true;
     }
     else if (topic == "network:offline-status-changed") {
       if (data == "offline") {
 	// this._list shold be empty here
         this._networkdown = true;
@@ -225,17 +238,19 @@ PeerConnection.prototype = {
                                     contractID: PC_CONTRACT,
                                     classDescription: "PeerConnection",
                                     interfaces: [
                                       Ci.nsIDOMRTCPeerConnection
                                     ],
                                     flags: Ci.nsIClassInfo.DOM_OBJECT}),
 
   QueryInterface: XPCOMUtils.generateQI([
-    Ci.nsIDOMRTCPeerConnection, Ci.nsIDOMGlobalObjectConstructor
+    Ci.nsIDOMRTCPeerConnection,
+    Ci.nsIDOMGlobalObjectConstructor,
+    Ci.nsISupportsWeakReference,
   ]),
 
   // Constructor is an explicit function, because of nsIDOMGlobalObjectConstructor.
   constructor: function(win) {
     if (!Services.prefs.getBoolPref("media.peerconnection.enabled")) {
       throw new Error("PeerConnection not enabled (did you set the pref?)");
     }
     if (this._win) {
@@ -541,17 +556,18 @@ PeerConnection.prototype = {
   }
 };
 
 // This is a seperate object because we don't want to expose it to DOM.
 function PeerConnectionObserver(dompc) {
   this._dompc = dompc;
 }
 PeerConnectionObserver.prototype = {
-  QueryInterface: XPCOMUtils.generateQI([Ci.IPeerConnectionObserver]),
+  QueryInterface: XPCOMUtils.generateQI([Ci.IPeerConnectionObserver,
+                                         Ci.nsISupportsWeakReference]),
 
   onCreateOfferSuccess: function(offer) {
     if (this._dompc._onCreateOfferSuccess) {
       try {
         this._dompc._onCreateOfferSuccess.onCallback({
           type: "offer", sdp: offer,
           __exposedProps__: { type: "rw", sdp: "rw" }
         });
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ -36,20 +36,24 @@ public:
 
   void Init()
     {
       nsCOMPtr<nsIObserverService> observerService =
         mozilla::services::GetObserverService();
       if (!observerService)
         return;
 
-      nsresult rv = observerService->AddObserver(this,
-                                                 NS_XPCOM_SHUTDOWN_OBSERVER_ID,
-                                                 false);
-      MOZ_ASSERT(rv == NS_OK);
+      nsresult rv = NS_OK;
+
+#ifdef MOZILLA_INTERNAL_API
+      rv = observerService->AddObserver(this,
+                                        NS_XPCOM_SHUTDOWN_OBSERVER_ID,
+                                        false);
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));
+#endif
       (void) rv;
     }
 
   virtual ~PeerConnectionCtxShutdown()
     {
       nsCOMPtr<nsIObserverService> observerService =
         mozilla::services::GetObserverService();
       if (observerService)
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -217,17 +217,19 @@ PeerConnectionImpl::PeerConnectionImpl()
   , mCall(NULL)
   , mReadyState(kNew)
   , mIceState(kIceGathering)
   , mPCObserver(NULL)
   , mWindow(NULL)
   , mIdentity(NULL)
   , mSTSThread(NULL)
   , mMedia(new PeerConnectionMedia(this)) {
+#ifdef MOZILLA_INTERNAL_API
   MOZ_ASSERT(NS_IsMainThread());
+#endif
 }
 
 PeerConnectionImpl::~PeerConnectionImpl()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
   PeerConnectionCtx::GetInstance()->mPeerConnections.erase(mHandle);
   CloseInt(false);
 
@@ -283,20 +285,23 @@ PeerConnectionImpl::CreateRemoteSourceSt
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 PeerConnectionImpl::Initialize(IPeerConnectionObserver* aObserver,
                                nsIDOMWindow* aWindow,
                                nsIThread* aThread) {
+#ifdef MOZILLA_INTERNAL_API
   MOZ_ASSERT(NS_IsMainThread());
+#endif
   MOZ_ASSERT(aObserver);
   MOZ_ASSERT(aThread);
-  mPCObserver = aObserver;
+
+  mPCObserver = do_GetWeakReference(aObserver);
 
   nsresult res;
 
 #ifdef MOZILLA_INTERNAL_API
   // This code interferes with the C++ unit test startup code.
   nsCOMPtr<nsISupports> nssDummy = do_GetService("@mozilla.org/psm;1", &res);
   NS_ENSURE_SUCCESS(res, res);
 #endif
@@ -516,35 +521,42 @@ PeerConnectionImpl::CreateDataChannel(co
 void
 PeerConnectionImpl::NotifyConnection()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 
   CSFLogDebugS(logTag, __FUNCTION__);
 
 #ifdef MOZILLA_INTERNAL_API
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return;
+  }
   RUN_ON_THREAD(mThread,
-                WrapRunnable(mPCObserver,
+                WrapRunnable(pco,
                              &IPeerConnectionObserver::NotifyConnection),
                 NS_DISPATCH_NORMAL);
 #endif
 }
 
 void
 PeerConnectionImpl::NotifyClosedConnection()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 
   CSFLogDebugS(logTag, __FUNCTION__);
 
 #ifdef MOZILLA_INTERNAL_API
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return;
+  }
   RUN_ON_THREAD(mThread,
-                WrapRunnable(mPCObserver,
-                             &IPeerConnectionObserver::NotifyClosedConnection),
-                NS_DISPATCH_NORMAL);
+    WrapRunnable(pco, &IPeerConnectionObserver::NotifyClosedConnection),
+    NS_DISPATCH_NORMAL);
 #endif
 }
 
 
 #ifdef MOZILLA_INTERNAL_API
 // Not a member function so that we don't need to keep the PC live.
 static void NotifyDataChannel_m(nsRefPtr<nsIDOMDataChannel> aChannel,
                                 nsCOMPtr<IPeerConnectionObserver> aObserver)
@@ -560,25 +572,30 @@ void
 PeerConnectionImpl::NotifyDataChannel(already_AddRefed<mozilla::DataChannel> aChannel)
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
   MOZ_ASSERT(aChannel.get());
 
   CSFLogDebugS(logTag, __FUNCTION__ << ": channel: " << static_cast<void*>(aChannel.get()));
 
 #ifdef MOZILLA_INTERNAL_API
-   nsCOMPtr<nsIDOMDataChannel> domchannel;
-   nsresult rv = NS_NewDOMDataChannel(aChannel, mWindow,
-                                      getter_AddRefs(domchannel));
+  nsCOMPtr<nsIDOMDataChannel> domchannel;
+  nsresult rv = NS_NewDOMDataChannel(aChannel, mWindow,
+                                     getter_AddRefs(domchannel));
   NS_ENSURE_SUCCESS_VOID(rv);
 
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return;
+  }
+
   RUN_ON_THREAD(mThread,
                 WrapRunnableNM(NotifyDataChannel_m,
                                domchannel.get(),
-                               mPCObserver),
+                               pco),
                 NS_DISPATCH_NORMAL);
 #endif
 }
 
 /**
  * Constraints look like this:
  *
  * {
@@ -991,37 +1008,45 @@ PeerConnectionImpl::onCallEvent(ccapi_ca
     case CONNECTED:
       CSFLogDebugS(logTag, "Setting PeerConnnection state to kActive");
       ChangeReadyState(kActive);
       break;
     default:
       break;
   }
 
-  if (mPCObserver) {
-    PeerConnectionObserverDispatch* runnable =
-        new PeerConnectionObserverDispatch(aInfo, this, mPCObserver);
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return;
+  }
 
-    if (mThread) {
-      mThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
-      return;
-    }
-    runnable->Run();
+  PeerConnectionObserverDispatch* runnable =
+      new PeerConnectionObserverDispatch(aInfo, this, pco);
+
+  if (mThread) {
+    mThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
+    return;
   }
+  runnable->Run();
+  delete runnable;
 }
 
 void
 PeerConnectionImpl::ChangeReadyState(PeerConnectionImpl::ReadyState aReadyState)
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
   mReadyState = aReadyState;
 
   // Note that we are passing an nsRefPtr<IPeerConnectionObserver> which
   // keeps the observer live.
-  RUN_ON_THREAD(mThread, WrapRunnable(mPCObserver,
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return;
+  }
+  RUN_ON_THREAD(mThread, WrapRunnable(pco,
                                       &IPeerConnectionObserver::OnStateChange,
                                       // static_cast needed to work around old Android NDK r5c compiler
                                       static_cast<int>(IPeerConnectionObserver::kReadyState)),
     NS_DISPATCH_NORMAL);
 }
 
 PeerConnectionWrapper::PeerConnectionWrapper(const std::string& handle)
     : impl_(nullptr) {
@@ -1065,24 +1090,26 @@ PeerConnectionImpl::IceGatheringComplete
   PC_AUTO_ENTER_API_CALL(false);
   MOZ_ASSERT(aCtx);
 
   CSFLogDebugS(logTag, __FUNCTION__ << ": ctx: " << static_cast<void*>(aCtx));
 
   mIceState = kIceWaiting;
 
 #ifdef MOZILLA_INTERNAL_API
-  if (mPCObserver) {
-    RUN_ON_THREAD(mThread,
-                  WrapRunnable(mPCObserver,
-                               &IPeerConnectionObserver::OnStateChange,
-                               // static_cast required to work around old C++ compiler on Android NDK r5c
-                               static_cast<int>(IPeerConnectionObserver::kIceState)),
-                  NS_DISPATCH_NORMAL);
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return NS_OK;
   }
+  RUN_ON_THREAD(mThread,
+                WrapRunnable(pco,
+                             &IPeerConnectionObserver::OnStateChange,
+                             // static_cast required to work around old C++ compiler on Android NDK r5c
+                             static_cast<int>(IPeerConnectionObserver::kIceState)),
+                NS_DISPATCH_NORMAL);
 #endif
   return NS_OK;
 }
 
 void
 PeerConnectionImpl::IceCompleted(NrIceCtx *aCtx)
 {
   // Do an async call here to unwind the stack. refptr keeps the PC alive.
@@ -1100,24 +1127,26 @@ PeerConnectionImpl::IceCompleted_m(NrIce
   PC_AUTO_ENTER_API_CALL(false);
   MOZ_ASSERT(aCtx);
 
   CSFLogDebugS(logTag, __FUNCTION__ << ": ctx: " << static_cast<void*>(aCtx));
 
   mIceState = kIceConnected;
 
 #ifdef MOZILLA_INTERNAL_API
-  if (mPCObserver) {
-    RUN_ON_THREAD(mThread,
-                  WrapRunnable(mPCObserver,
-                               &IPeerConnectionObserver::OnStateChange,
-                               // static_cast required to work around old C++ compiler on Android NDK r5c
-			       static_cast<int>(IPeerConnectionObserver::kIceState)),
-                  NS_DISPATCH_NORMAL);
+  nsCOMPtr<IPeerConnectionObserver> pco = do_QueryReferent(mPCObserver);
+  if (!pco) {
+    return NS_OK;
   }
+  RUN_ON_THREAD(mThread,
+                WrapRunnable(pco,
+                             &IPeerConnectionObserver::OnStateChange,
+                             // static_cast required to work around old C++ compiler on Android NDK r5c
+                             static_cast<int>(IPeerConnectionObserver::kIceState)),
+                NS_DISPATCH_NORMAL);
 #endif
   return NS_OK;
 }
 
 void
 PeerConnectionImpl::IceStreamReady(NrIceMediaStream *aStream)
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -7,16 +7,18 @@
 
 #include <string>
 #include <vector>
 #include <map>
 #include <cmath>
 
 #include "prlock.h"
 #include "mozilla/RefPtr.h"
+#include "nsWeakPtr.h"
+#include "nsIWeakReferenceUtils.h" // for the definition of nsWeakPtr
 #include "IPeerConnection.h"
 #include "nsComponentManagerUtils.h"
 #include "nsPIDOMWindow.h"
 
 #include "dtlsidentity.h"
 
 #include "peer_connection_types.h"
 #include "CallControlManager.h"
@@ -225,17 +227,19 @@ private:
   // The call
   CSF::CC_CallPtr mCall;
   ReadyState mReadyState;
 
   // ICE State
   IceState mIceState;
 
   nsCOMPtr<nsIThread> mThread;
-  nsCOMPtr<IPeerConnectionObserver> mPCObserver;
+  // Weak pointer to IPeerConnectionObserver
+  // This is only safe to use on the main thread
+  nsWeakPtr mPCObserver;
   nsCOMPtr<nsPIDOMWindow> mWindow;
 
   // The SDP sent in from JS - here for debugging.
   std::string mLocalRequestedSDP;
   std::string mRemoteRequestedSDP;
   // The SDP we are using.
   std::string mLocalSDP;
   std::string mRemoteSDP;
--- a/media/webrtc/signaling/test/Makefile.in
+++ b/media/webrtc/signaling/test/Makefile.in
@@ -128,16 +128,17 @@ LOCAL_INCLUDES += \
  -I$(topsrcdir)/media/mtransport/test \
  -I$(topsrcdir)/media/webrtc/signaling/include \
  -I$(topsrcdir)/media/webrtc/signaling/src/media-conduit \
  -I$(topsrcdir)/media/webrtc/signaling/src/mediapipeline \
  -I$(topsrcdir)/media/webrtc/signaling/src/sipcc/include \
  -I$(topsrcdir)/media/webrtc/signaling/src/peerconnection \
  -I$(topsrcdir)/media/webrtc/signaling/media-conduit\
  -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source/ \
+ -I$(topsrcdir)/xpcom/base/ \
  $(NULL)
 
 ifneq ($(OS_TARGET),WINNT)
 ifdef JS_SHARED_LIBRARY
 LIBS += $(MOZ_ZLIB_LIBS)
 endif
 
 CPP_UNIT_TESTS = \
--- a/media/webrtc/signaling/test/signaling_unittests.cpp
+++ b/media/webrtc/signaling/test/signaling_unittests.cpp
@@ -18,19 +18,21 @@ using namespace std;
 #include "nspr.h"
 #include "nss.h"
 #include "ssl.h"
 #include "prthread.h"
 
 #include "FakeMediaStreams.h"
 #include "FakeMediaStreamsImpl.h"
 #include "PeerConnectionImpl.h"
+#include "PeerConnectionCtx.h"
 #include "runnable_utils.h"
 #include "nsStaticComponents.h"
 #include "nsIDOMRTCPeerConnection.h"
+#include "nsWeakReference.h"
 
 #include "mtransport_test_utils.h"
 MtransportTestUtils *test_utils;
 
 
 
 static int kDefaultTimeout = 5000;
 
@@ -113,17 +115,18 @@ enum offerAnswerFlags
   ANSWER_AUDIO = (1<<8),
   ANSWER_VIDEO = (1<<9),
 
   OFFER_AV = OFFER_AUDIO | OFFER_VIDEO,
   ANSWER_AV = ANSWER_AUDIO | ANSWER_VIDEO
 };
 
 
-class TestObserver : public IPeerConnectionObserver
+class TestObserver : public IPeerConnectionObserver,
+                     public nsSupportsWeakReference
 {
 public:
   enum Action {
     OFFER,
     ANSWER
   };
 
   enum StateType {
@@ -158,17 +161,19 @@ public:
   uint32_t lastStateType;
   bool onAddStreamCalled;
 
 private:
   sipcc::PeerConnectionImpl *pc;
   std::vector<nsDOMMediaStream *> streams;
 };
 
-NS_IMPL_THREADSAFE_ISUPPORTS1(TestObserver, IPeerConnectionObserver)
+NS_IMPL_THREADSAFE_ISUPPORTS2(TestObserver,
+                              IPeerConnectionObserver,
+                              nsISupportsWeakReference)
 
 NS_IMETHODIMP
 TestObserver::OnCreateOfferSuccess(const char* offer)
 {
   lastString = strdup(offer);
   state = stateSuccess;
   cout << "onCreateOfferSuccess = " << offer << endl;
   return NS_OK;
@@ -484,29 +489,37 @@ class SignalingAgent {
   SignalingAgent() : pc(nullptr) {}
 
   ~SignalingAgent() {
     pc->GetMainThread()->Dispatch(
       WrapRunnable(this, &SignalingAgent::Close),
       NS_DISPATCH_SYNC);
   }
 
-  void Init(nsCOMPtr<nsIThread> thread)
+  void Init_m(nsCOMPtr<nsIThread> thread)
   {
     size_t found = 2;
     ASSERT_TRUE(found > 0);
 
     pc = sipcc::PeerConnectionImpl::CreatePeerConnection();
     ASSERT_TRUE(pc);
 
     pObserver = new TestObserver(pc);
     ASSERT_TRUE(pObserver);
 
     ASSERT_EQ(pc->Initialize(pObserver, nullptr, thread), NS_OK);
 
+  }
+
+  void Init(nsCOMPtr<nsIThread> thread)
+  {
+    thread->Dispatch(
+      WrapRunnable(this, &SignalingAgent::Init_m, thread),
+      NS_DISPATCH_SYNC);
+
     ASSERT_TRUE_WAIT(sipcc_state() == sipcc::PeerConnectionImpl::kStarted,
                      kDefaultTimeout);
     ASSERT_TRUE_WAIT(ice_state() == sipcc::PeerConnectionImpl::kIceWaiting, 5000);
     cout << "Init Complete" << endl;
   }
 
   uint32_t sipcc_state()
   {
@@ -1754,11 +1767,18 @@ int main(int argc, char **argv) {
   for(int i=0; i<argc; i++) {
     if (!strcmp(argv[i],"-t")) {
       kDefaultTimeout = 20000;
     }
   }
 
   ::testing::AddGlobalTestEnvironment(new test::SignalingEnvironment);
   int result = RUN_ALL_TESTS();
+
+  // Because we don't initialize on the main thread, we can't register for
+  // XPCOM shutdown callbacks (where the context is usually shut down) --
+  // so we need to explictly destroy the context.
+  sipcc::PeerConnectionCtx::Destroy();
   delete test_utils;
+
+
   return result;
 }