Bug 928221 r=jesup, abr
☠☠ backed out by 77f70432fdcd ☠ ☠
authorJan-Ivar Bruaroey <jib@mozilla.com>
Fri, 18 Oct 2013 17:22:05 -0400
changeset 165241 dc2b71e57211b1a71f17937aa3da729d5ecf4426
parent 165207 c4f64a12e6ef7bb6ae8fed23290a700081c7aebc
child 165242 64ad24d86f5952a612eb5c8e095f11e0bbd2c83a
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, abr
bugs928221
milestone27.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 928221 r=jesup, abr
dom/media/PeerConnection.js
dom/webidl/PeerConnectionObserver.webidl
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
media/webrtc/signaling/test/FakePCObserver.h
media/webrtc/signaling/test/signaling_unittests.cpp
--- a/dom/media/PeerConnection.js
+++ b/dom/media/PeerConnection.js
@@ -886,16 +886,17 @@ RTCError.prototype = {
     "INCOMPATIBLE_MEDIASTREAMTRACK",
     "INTERNAL_ERROR"
   ]
 };
 
 // This is a separate object because we don't want to expose it to DOM.
 function PeerConnectionObserver() {
   this._dompc = null;
+  this._guard = new WeakReferent(this);
 }
 PeerConnectionObserver.prototype = {
   classDescription: "PeerConnectionObserver",
   classID: PC_OBS_CID,
   contractID: PC_OBS_CONTRACT,
   QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports,
                                          Ci.nsIDOMGlobalPropertyInitializer]),
   init: function(win) { this._win = win; },
@@ -1187,15 +1188,29 @@ PeerConnectionObserver.prototype = {
   },
 
   notifyClosedConnection: function() {
     this.dispatchEvent(new this._dompc._win.Event("closedconnection"));
   },
 
   getSupportedConstraints: function(dict) {
     return dict;
+  },
+
+  get weakReferent() {
+    return this._guard;
   }
 };
 
+// A PeerConnectionObserver member that c++ can do weak references on
+
+function WeakReferent(parent) {
+  this._parent = parent; // prevents parent from going away without us
+}
+WeakReferent.prototype = {
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports,
+                                         Ci.nsISupportsWeakReference]),
+};
+
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory(
   [GlobalPCList, RTCIceCandidate, RTCSessionDescription, RTCPeerConnection,
    RTCStatsReport, PeerConnectionObserver]
 );
--- a/dom/webidl/PeerConnectionObserver.webidl
+++ b/dom/webidl/PeerConnectionObserver.webidl
@@ -1,14 +1,16 @@
 /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+interface nsISupports;
+
 [ChromeOnly,
  JSImplementation="@mozilla.org/dom/peerconnectionobserver;1",
  Constructor (object domPC)]
 interface PeerConnectionObserver
 {
   /* JSEP callbacks */
   void onCreateOfferSuccess(DOMString offer);
   void onCreateOfferError(unsigned long name, DOMString message);
@@ -35,16 +37,19 @@ interface PeerConnectionObserver
   void onStateChange(PCObserverStateType state);
 
   /* Changes to MediaStreams */
   void onAddStream(MediaStream stream);
   void onRemoveStream();
   void onAddTrack();
   void onRemoveTrack();
 
+  /* Used by c++ to know when Observer goes away */
+  readonly attribute nsISupports weakReferent;
+
   /* Helper function to access supported constraints defined in webidl. Needs to
    * be in a separate webidl object we hold, so putting it here was convenient.
    */
 // TODO: Bug 863949
 //  MediaConstraintSet getSupportedConstraints(optional
   object getSupportedConstraints(optional
       MediaConstraintSet constraints);
 };
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -650,17 +650,17 @@ PeerConnectionImpl::Initialize(PeerConne
   // Invariant: we receive configuration one way or the other but not both (XOR)
   MOZ_ASSERT(!aConfiguration != !aRTCConfiguration);
 #ifdef MOZILLA_INTERNAL_API
   MOZ_ASSERT(NS_IsMainThread());
 #endif
   MOZ_ASSERT(aThread);
   mThread = do_QueryInterface(aThread);
 
-  mPCObserver.Init(&aObserver);
+  mPCObserver.Set(&aObserver);
 
   // Find the STS thread
 
   mSTSThread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &res);
   MOZ_ASSERT(mSTSThread);
 
 #ifdef MOZILLA_INTERNAL_API
   // This code interferes with the C++ unit test startup code.
@@ -1378,22 +1378,16 @@ PeerConnectionImpl::Close()
 }
 
 
 nsresult
 PeerConnectionImpl::CloseInt()
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
 
-  // Clear raw pointer to observer since PeerConnection.js does not guarantee
-  // the observer's existence past Close().
-  //
-  // Any outstanding runnables hold RefPtr<> references to observer.
-  mPCObserver.Close();
-
   if (mInternal->mCall) {
     CSFLogInfo(logTag, "%s: Closing PeerConnectionImpl %s; "
                "ending call", __FUNCTION__, mHandle.c_str());
     mInternal->mCall->endCall();
   }
 #ifdef MOZILLA_INTERNAL_API
   if (mDataConnection) {
     CSFLogInfo(logTag, "%s: Destroying DataChannelConnection %p for %s",
@@ -1757,10 +1751,35 @@ PeerConnectionImpl::GetRemoteStreams(nsT
     result.AppendElement(info->GetMediaStream());
   }
   return NS_OK;
 #else
   return NS_ERROR_FAILURE;
 #endif
 }
 
+// WeakConcretePtr gets around WeakPtr not working on concrete types by using
+// the attribute getWeakReferent, a member that supports weak refs, as a guard.
+
+void
+PeerConnectionImpl::WeakConcretePtr::Set(PeerConnectionObserver *aObserver) {
+  mObserver = aObserver;
+#ifdef MOZILLA_INTERNAL_API
+  MOZ_ASSERT(NS_IsMainThread());
+  JSErrorResult rv;
+  nsCOMPtr<nsISupports> tmp = aObserver->GetWeakReferent(rv);
+  MOZ_ASSERT(!rv.Failed());
+  mWeakPtr = do_GetWeakReference(tmp);
+#else
+  mWeakPtr = do_GetWeakReference(aObserver);
+#endif
+}
+
+PeerConnectionObserver *
+PeerConnectionImpl::WeakConcretePtr::MayGet() {
+#ifdef MOZILLA_INTERNAL_API
+  MOZ_ASSERT(NS_IsMainThread());
+#endif
+  nsCOMPtr<nsISupports> guard = do_QueryReferent(mWeakPtr);
+  return (!guard) ? nullptr : mObserver;
+}
 
 }  // end sipcc namespace
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -7,16 +7,19 @@
 
 #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 "sigslot.h"
 #include "nricectx.h"
 #include "nricemediastream.h"
 #include "nsComponentManagerUtils.h"
 #include "nsPIDOMWindow.h"
 #include "nsIThread.h"
 
 #include "mozilla/ErrorResult.h"
@@ -497,39 +500,29 @@ private:
   mozilla::ScopedDeletePtr<Internal> mInternal;
   mozilla::dom::PCImplReadyState mReadyState;
   mozilla::dom::PCImplSignalingState mSignalingState;
 
   // ICE State
   mozilla::dom::PCImplIceState mIceState;
 
   nsCOMPtr<nsIThread> mThread;
-  // We hold a raw pointer to PeerConnectionObserver (no WeakRefs to concretes!)
-  // which is an invariant guaranteed to exist between Initialize() and Close().
-  // We explicitly clear it in Close(). We wrap it in a helper, to encourage
-  // testing against nullptr before use. Use in Runnables requires wrapping
-  // access in RefPtr<> since they may execute after close. This is only safe
-  // to use on the main thread
+  // WeakConcretePtr to PeerConnectionObserver. TODO: Remove after bug 928535
   //
+  // This is only safe to use on the main thread
   // TODO: Remove if we ever properly wire PeerConnection for cycle-collection.
-  class WeakReminder
+  class WeakConcretePtr
   {
   public:
-    WeakReminder() : mObserver(nullptr) {}
-    void Init(PeerConnectionObserver *aObserver) {
-      mObserver = aObserver;
-    }
-    void Close() {
-      mObserver = nullptr;
-    }
-    PeerConnectionObserver *MayGet() {
-      return mObserver;
-    }
+    WeakConcretePtr() : mObserver(nullptr) {}
+    void Set(PeerConnectionObserver *aObserver);
+    PeerConnectionObserver *MayGet();
   private:
     PeerConnectionObserver *mObserver;
+    nsWeakPtr mWeakPtr;
   } 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;
--- a/media/webrtc/signaling/test/FakePCObserver.h
+++ b/media/webrtc/signaling/test/FakePCObserver.h
@@ -16,28 +16,29 @@
 #include "MediaSegment.h"
 #include "StreamBuffer.h"
 #include "nsTArray.h"
 #include "nsIRunnable.h"
 #include "nsISupportsImpl.h"
 #include "nsIDOMMediaStream.h"
 #include "mozilla/dom/PeerConnectionObserverEnumsBinding.h"
 #include "PeerConnectionImpl.h"
+#include "nsWeakReference.h"
 
 namespace sipcc {
 class PeerConnectionImpl;
 }
 
 class nsIDOMWindow;
 class nsIDOMDataChannel;
 
 namespace test {
 using namespace mozilla::dom;
 
-class AFakePCObserver : public nsISupports
+class AFakePCObserver : public nsSupportsWeakReference
 {
 protected:
   typedef mozilla::ErrorResult ER;
 public:
   enum Action {
     OFFER,
     ANSWER
   };
--- a/media/webrtc/signaling/test/signaling_unittests.cpp
+++ b/media/webrtc/signaling/test/signaling_unittests.cpp
@@ -268,17 +268,17 @@ public:
   NS_IMETHODIMP OnRemoveStream(ER&);
   NS_IMETHODIMP OnAddTrack(ER&);
   NS_IMETHODIMP OnRemoveTrack(ER&);
   NS_IMETHODIMP OnAddIceCandidateSuccess(ER&);
   NS_IMETHODIMP OnAddIceCandidateError(uint32_t code, const char *msg, ER&);
   NS_IMETHODIMP OnIceCandidate(uint16_t level, const char *mid, const char *cand, ER&);
 };
 
-NS_IMPL_ISUPPORTS0(TestObserver)
+NS_IMPL_ISUPPORTS1(TestObserver, nsISupportsWeakReference)
 
 NS_IMETHODIMP
 TestObserver::OnCreateOfferSuccess(const char* offer, ER&)
 {
   lastString = strdup(offer);
   state = stateSuccess;
   std::cout << name << ": onCreateOfferSuccess = " << std::endl << indent(offer)
             << std::endl;