Backed out changeset dc2b71e57211 (bug 928221) because it calls a non-existing GetWeakReferent function
authorEhsan Akhgari <ehsan@mozilla.com>
Sat, 19 Oct 2013 10:48:41 -0400
changeset 166206 77f70432fdcd1c525d57c6ef8f558cc155869e1b
parent 166205 061a1f7cb896d7166456536ac1245a0a5524e2ff
child 166207 14789271ad201a947a2f6b2dbebe1415f7eb85ff
push id428
push userbbajaj@mozilla.com
push dateTue, 28 Jan 2014 00:16:25 +0000
treeherdermozilla-release@cd72a7ff3a75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs928221
milestone27.0a1
backs outdc2b71e57211b1a71f17937aa3da729d5ecf4426
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
Backed out changeset dc2b71e57211 (bug 928221) because it calls a non-existing GetWeakReferent function
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,17 +886,16 @@ 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; },
@@ -1188,29 +1187,15 @@ 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,16 +1,14 @@
 /* -*- 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);
@@ -37,19 +35,16 @@ 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.Set(&aObserver);
+  mPCObserver.Init(&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,16 +1378,22 @@ 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",
@@ -1751,35 +1757,10 @@ 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,19 +7,16 @@
 
 #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"
@@ -500,29 +497,39 @@ private:
   mozilla::ScopedDeletePtr<Internal> mInternal;
   mozilla::dom::PCImplReadyState mReadyState;
   mozilla::dom::PCImplSignalingState mSignalingState;
 
   // ICE State
   mozilla::dom::PCImplIceState mIceState;
 
   nsCOMPtr<nsIThread> mThread;
-  // WeakConcretePtr to PeerConnectionObserver. TODO: Remove after bug 928535
+  // 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
   //
-  // This is only safe to use on the main thread
   // TODO: Remove if we ever properly wire PeerConnection for cycle-collection.
-  class WeakConcretePtr
+  class WeakReminder
   {
   public:
-    WeakConcretePtr() : mObserver(nullptr) {}
-    void Set(PeerConnectionObserver *aObserver);
-    PeerConnectionObserver *MayGet();
+    WeakReminder() : mObserver(nullptr) {}
+    void Init(PeerConnectionObserver *aObserver) {
+      mObserver = aObserver;
+    }
+    void Close() {
+      mObserver = nullptr;
+    }
+    PeerConnectionObserver *MayGet() {
+      return mObserver;
+    }
   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,29 +16,28 @@
 #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 nsSupportsWeakReference
+class AFakePCObserver : public nsISupports
 {
 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_ISUPPORTS1(TestObserver, nsISupportsWeakReference)
+NS_IMPL_ISUPPORTS0(TestObserver)
 
 NS_IMETHODIMP
 TestObserver::OnCreateOfferSuccess(const char* offer, ER&)
 {
   lastString = strdup(offer);
   state = stateSuccess;
   std::cout << name << ": onCreateOfferSuccess = " << std::endl << indent(offer)
             << std::endl;