Bug 1550177 - Part 2: Remove a stack-unwind, and simplify signal handling considerably. r=mjf
authorByron Campen [:bwc] <docfaraday@gmail.com>
Wed, 19 Jun 2019 16:54:45 +0000
changeset 479215 4ce46b3be46ebf9c56fc53259e94291a5f00ef16
parent 479214 22a2b3e65223662566074fcdac57a9e3eeb0e20b
child 479216 01c9aca208929febfaac5f1860e11ff7cbef64b1
push id36174
push useropoprus@mozilla.com
push dateWed, 19 Jun 2019 21:38:13 +0000
treeherdermozilla-central@5b9a3de04646 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjf
bugs1550177
milestone69.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 1550177 - Part 2: Remove a stack-unwind, and simplify signal handling considerably. r=mjf Differential Revision: https://phabricator.services.mozilla.com/D30992
dom/media/PeerConnection.jsm
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
--- a/dom/media/PeerConnection.jsm
+++ b/dom/media/PeerConnection.jsm
@@ -1582,16 +1582,20 @@ class RTCPeerConnection {
     }
     return this._impl.signalingState;
   }
 
   changeIceGatheringState(state) {
     this._iceGatheringState = state;
     _globalPCList.notifyLifecycleObservers(this, "icegatheringstatechange");
     this.dispatchEvent(new this._win.Event("icegatheringstatechange"));
+    if (this.iceGatheringState === "complete") {
+      this.dispatchEvent(new this._win.RTCPeerConnectionIceEvent(
+        "icecandidate", { candidate: null }));
+    }
   }
 
   changeIceConnectionState(state) {
     if (state != this._iceConnectionState) {
       this._iceConnectionState = state;
       _globalPCList.notifyLifecycleObservers(this, "iceconnectionstatechange");
       this.dispatchEvent(new this._win.Event("iceconnectionstatechange"));
     }
@@ -1780,18 +1784,16 @@ class PeerConnectionObserver {
 
   onIceCandidate(sdpMLineIndex, sdpMid, candidate, usernameFragment) {
     let win = this._dompc._win;
     if (candidate || sdpMid || usernameFragment) {
       if (candidate.includes(" typ relay ")) {
         this._dompc._iceGatheredRelayCandidates = true;
       }
       candidate = new win.RTCIceCandidate({ candidate, sdpMid, sdpMLineIndex, usernameFragment });
-    } else {
-      candidate = null;
     }
     this.dispatchEvent(new win.RTCPeerConnectionIceEvent("icecandidate",
                                                          { candidate }));
   }
 
   // This method is primarily responsible for updating iceConnectionState.
   // This state is defined in the WebRTC specification as follows:
   //
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -508,25 +508,16 @@ nsresult PeerConnectionImpl::Initialize(
   // Initialize the media object.
   res = mMedia->Init();
   if (NS_FAILED(res)) {
     CSFLogError(LOGTAG, "%s: Couldn't initialize media object", __FUNCTION__);
     ShutdownMedia();
     return res;
   }
 
-  // Connect ICE slots.
-  mMedia->SignalIceGatheringStateChange.connect(
-      this, &PeerConnectionImpl::IceGatheringStateChange);
-  mMedia->SignalUpdateDefaultCandidate.connect(
-      this, &PeerConnectionImpl::UpdateDefaultCandidate);
-  mMedia->SignalIceConnectionStateChange.connect(
-      this, &PeerConnectionImpl::IceConnectionStateChange);
-  mMedia->SignalCandidate.connect(this, &PeerConnectionImpl::CandidateReady);
-
   PeerConnectionCtx::GetInstance()->mPeerConnections[mHandle] = this;
 
   return NS_OK;
 }
 
 void PeerConnectionImpl::Initialize(PeerConnectionObserver& aObserver,
                                     nsGlobalWindowInner& aWindow,
                                     const RTCConfiguration& aConfiguration,
@@ -2408,36 +2399,23 @@ void PeerConnectionImpl::CandidateReady(
     return;
   }
 
   CSFLogDebug(LOGTAG, "Passing local candidate to content: %s",
               candidate.c_str());
   SendLocalIceCandidateToContent(level, mid, candidate, ufrag);
 }
 
-static void SendLocalIceCandidateToContentImpl(
-    const RefPtr<PeerConnectionObserver>& aPCObserver, uint16_t level,
-    const std::string& mid, const std::string& candidate,
-    const std::string& ufrag) {
-  JSErrorResult rv;
-  aPCObserver->OnIceCandidate(level, ObString(mid.c_str()),
-                              ObString(candidate.c_str()),
-                              ObString(ufrag.c_str()), rv);
-}
-
 void PeerConnectionImpl::SendLocalIceCandidateToContent(
     uint16_t level, const std::string& mid, const std::string& candidate,
     const std::string& ufrag) {
-  // We dispatch this because OnSetLocalDescriptionSuccess does a setTimeout(0)
-  // to unwind the stack, but the event handlers don't. We need to ensure that
-  // the candidates do not skip ahead of the callback.
-  NS_DispatchToMainThread(
-      WrapRunnableNM(&SendLocalIceCandidateToContentImpl, mPCObserver, level,
-                     mid, candidate, ufrag),
-      NS_DISPATCH_NORMAL);
+  JSErrorResult rv;
+  mPCObserver->OnIceCandidate(level, ObString(mid.c_str()),
+                              ObString(candidate.c_str()),
+                              ObString(ufrag.c_str()), rv);
 }
 
 static bool isDone(RTCIceConnectionState state) {
   return state != RTCIceConnectionState::Checking &&
          state != RTCIceConnectionState::New;
 }
 
 static bool isSucceeded(RTCIceConnectionState state) {
@@ -2511,21 +2489,36 @@ void PeerConnectionImpl::IceConnectionSt
     default:
       MOZ_ASSERT_UNREACHABLE("Unexpected mIceConnectionState!");
   }
 
   WrappableJSErrorResult rv;
   mPCObserver->OnStateChange(PCObserverStateType::IceConnectionState, rv);
 }
 
+void PeerConnectionImpl::OnCandidateFound(const std::string& aTransportId,
+                                          const CandidateInfo& aCandidateInfo) {
+  if (!aCandidateInfo.mDefaultHostRtp.empty()) {
+    UpdateDefaultCandidate(aCandidateInfo.mDefaultHostRtp,
+                           aCandidateInfo.mDefaultPortRtp,
+                           aCandidateInfo.mDefaultHostRtcp,
+                           aCandidateInfo.mDefaultPortRtcp, aTransportId);
+  }
+  CandidateReady(aCandidateInfo.mCandidate, aTransportId,
+                 aCandidateInfo.mUfrag);
+}
+
 void PeerConnectionImpl::IceGatheringStateChange(
     dom::RTCIceGatheringState state) {
   PC_AUTO_ENTER_API_CALL_VOID_RETURN(false);
 
-  CSFLogDebug(LOGTAG, "%s", __FUNCTION__);
+  CSFLogDebug(LOGTAG, "%s %d", __FUNCTION__, static_cast<int>(state));
+  if (mIceGatheringState == state) {
+    return;
+  }
 
   mIceGatheringState = state;
 
   // Would be nice if we had a means of converting one of these dom enums
   // to a string that wasn't almost as much text as this switch statement...
   switch (mIceGatheringState) {
     case RTCIceGatheringState::New:
       STAMP_TIMECARD(mTimeCard, "Ice gathering state: new");
@@ -2535,26 +2528,18 @@ void PeerConnectionImpl::IceGatheringSta
       break;
     case RTCIceGatheringState::Complete:
       STAMP_TIMECARD(mTimeCard, "Ice gathering state: complete");
       break;
     default:
       MOZ_ASSERT_UNREACHABLE("Unexpected mIceGatheringState!");
   }
 
-  WrappableJSErrorResult rv;
-  mThread->Dispatch(
-      WrapRunnable(mPCObserver, &PeerConnectionObserver::OnStateChange,
-                   PCObserverStateType::IceGatheringState, rv,
-                   static_cast<JS::Realm*>(nullptr)),
-      NS_DISPATCH_NORMAL);
-
-  if (mIceGatheringState == RTCIceGatheringState::Complete) {
-    SendLocalIceCandidateToContent(0, "", "", "");
-  }
+  JSErrorResult rv;
+  mPCObserver->OnStateChange(PCObserverStateType::IceGatheringState, rv);
 }
 
 void PeerConnectionImpl::UpdateDefaultCandidate(
     const std::string& defaultAddr, uint16_t defaultPort,
     const std::string& defaultRtcpAddr, uint16_t defaultRtcpPort,
     const std::string& transportId) {
   CSFLogDebug(LOGTAG, "%s", __FUNCTION__);
   mJsepSession->UpdateDefaultCandidate(
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -10,17 +10,16 @@
 #include <vector>
 #include <map>
 #include <cmath>
 
 #include "prlock.h"
 #include "mozilla/RefPtr.h"
 #include "nsAutoPtr.h"
 #include "IPeerConnection.h"
-#include "sigslot.h"
 #include "nsComponentManagerUtils.h"
 #include "nsPIDOMWindow.h"
 #include "nsIUUIDGenerator.h"
 #include "nsIThread.h"
 #include "mozilla/Mutex.h"
 
 // Work around nasty macro in webrtc/voice_engine/voice_engine_defines.h
 #ifdef GetLastError
@@ -50,16 +49,17 @@ namespace test {
 #ifdef USE_FAKE_PCOBSERVER
 class AFakePCObserver;
 #endif
 }  // namespace test
 
 class nsDOMDataChannel;
 
 namespace mozilla {
+struct CandidateInfo;
 class DataChannel;
 class DtlsIdentity;
 class MediaPipeline;
 class TransceiverImpl;
 
 namespace dom {
 class RTCCertificate;
 struct RTCConfiguration;
@@ -159,18 +159,17 @@ typedef MozPromise<UniquePtr<RTCStatsQue
     nsresult res = CheckApiState(assert_ice_ready);          \
     if (NS_FAILED(res)) return;                              \
   } while (0)
 #define PC_AUTO_ENTER_API_CALL_NO_CHECK() CheckThread()
 
 class PeerConnectionImpl final
     : public nsISupports,
       public mozilla::DataChannelConnection::DataConnectionListener,
-      public dom::PrincipalChangeObserver<dom::MediaStreamTrack>,
-      public sigslot::has_slots<> {
+      public dom::PrincipalChangeObserver<dom::MediaStreamTrack> {
   struct Internal;  // Avoid exposing c includes to bindings
 
  public:
   explicit PeerConnectionImpl(
       const mozilla::dom::GlobalObject* aGlobal = nullptr);
 
   NS_DECL_THREADSAFE_ISUPPORTS
 
@@ -202,28 +201,29 @@ class PeerConnectionImpl final
   virtual const std::string& GetHandle();
 
   // Name suitable for exposing to content
   virtual const std::string& GetName();
 
   // ICE events
   void IceConnectionStateChange(dom::RTCIceConnectionState state);
   void IceGatheringStateChange(dom::RTCIceGatheringState state);
+  void OnCandidateFound(const std::string& aTransportId,
+                        const CandidateInfo& aCandidateInfo);
   void UpdateDefaultCandidate(const std::string& defaultAddr,
                               uint16_t defaultPort,
                               const std::string& defaultRtcpAddr,
                               uint16_t defaultRtcpPort,
                               const std::string& transportId);
 
   static void ListenThread(void* aData);
   static void ConnectThread(void* aData);
 
   // Get the main thread
   nsCOMPtr<nsIThread> GetMainThread() {
-    PC_AUTO_ENTER_API_CALL_NO_CHECK();
     return mThread;
   }
 
   // Get the STS thread
   nsIEventTarget* GetSTSThread() {
     PC_AUTO_ENTER_API_CALL_NO_CHECK();
     return mSTSThread;
   }
@@ -427,19 +427,17 @@ class PeerConnectionImpl final
     mozilla::dom::RTCIceConnectionState state;
     IceConnectionState(&state);
     return state;
   }
 
   NS_IMETHODIMP IceGatheringState(mozilla::dom::RTCIceGatheringState* aState);
 
   mozilla::dom::RTCIceGatheringState IceGatheringState() {
-    mozilla::dom::RTCIceGatheringState state;
-    IceGatheringState(&state);
-    return state;
+    return mIceGatheringState;
   }
 
   NS_IMETHODIMP Close();
 
   void Close(ErrorResult& rv) { rv = Close(); }
 
   bool PluginCrash(uint32_t aPluginID, const nsAString& aPluginName);
 
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ -91,17 +91,17 @@ void PeerConnectionMedia::StunAddrsHandl
   if (pcm_) {
     pcm_->mStunAddrs = addrs;
     pcm_->mLocalAddrsCompleted = true;
     pcm_->mStunAddrsRequest = nullptr;
     pcm_->FlushIceCtxOperationQueueIfReady();
     // If parent process returns 0 STUN addresses, change ICE connection
     // state to failed.
     if (!pcm_->mStunAddrs.Length()) {
-      pcm_->SignalIceConnectionStateChange(dom::RTCIceConnectionState::Failed);
+      pcm_->IceConnectionStateChange_m(dom::RTCIceConnectionState::Failed);
     }
 
     pcm_ = nullptr;
   }
 }
 
 PeerConnectionMedia::PeerConnectionMedia(PeerConnectionImpl* parent)
     : mTransportHandler(parent->GetTransportHandler()),
@@ -500,16 +500,17 @@ void PeerConnectionMedia::SelfDestruct()
 
   mQueuedIceCtxOperations.clear();
 
   // Shutdown the transport (async)
   RUN_ON_THREAD(
       mSTSThread,
       WrapRunnable(this, &PeerConnectionMedia::ShutdownMediaTransport_s),
       NS_DISPATCH_NORMAL);
+  mParent = nullptr;
 
   CSFLogDebug(LOGTAG, "%s: Media shut down", __FUNCTION__);
 }
 
 void PeerConnectionMedia::SelfDestruct_m() {
   CSFLogDebug(LOGTAG, "%s: ", __FUNCTION__);
 
   ASSERT_ON_THREAD(mMainThread);
@@ -672,51 +673,46 @@ void PeerConnectionMedia::OnCandidateFou
       WrapRunnable(this, &PeerConnectionMedia::OnCandidateFound_m, aTransportId,
                    aCandidateInfo),
       NS_DISPATCH_NORMAL);
 }
 
 void PeerConnectionMedia::IceGatheringStateChange_m(
     dom::RTCIceGatheringState aState) {
   ASSERT_ON_THREAD(mMainThread);
-  SignalIceGatheringStateChange(aState);
+  if (mParent) {
+    mParent->IceGatheringStateChange(aState);
+  }
 }
 
 void PeerConnectionMedia::IceConnectionStateChange_m(
     dom::RTCIceConnectionState aState) {
   ASSERT_ON_THREAD(mMainThread);
-  SignalIceConnectionStateChange(aState);
+  if (mParent) {
+    mParent->IceConnectionStateChange(aState);
+  }
 }
 
 void PeerConnectionMedia::OnCandidateFound_m(
     const std::string& aTransportId, const CandidateInfo& aCandidateInfo) {
   ASSERT_ON_THREAD(mMainThread);
-  if (!aCandidateInfo.mDefaultHostRtp.empty()) {
-    SignalUpdateDefaultCandidate(aCandidateInfo.mDefaultHostRtp,
-                                 aCandidateInfo.mDefaultPortRtp,
-                                 aCandidateInfo.mDefaultHostRtcp,
-                                 aCandidateInfo.mDefaultPortRtcp, aTransportId);
+  if (mParent) {
+    mParent->OnCandidateFound(aTransportId, aCandidateInfo);
   }
-  SignalCandidate(aCandidateInfo.mCandidate, aTransportId,
-                  aCandidateInfo.mUfrag);
 }
 
 void PeerConnectionMedia::AlpnNegotiated_s(const std::string& aAlpn) {
   GetMainThread()->Dispatch(
-      WrapRunnableNM(&PeerConnectionMedia::AlpnNegotiated_m, mParentHandle,
-                     aAlpn),
+      WrapRunnable(this, &PeerConnectionMedia::AlpnNegotiated_m, aAlpn),
       NS_DISPATCH_NORMAL);
 }
 
-void PeerConnectionMedia::AlpnNegotiated_m(const std::string& aParentHandle,
-                                           const std::string& aAlpn) {
-  PeerConnectionWrapper pcWrapper(aParentHandle);
-  PeerConnectionImpl* pc = pcWrapper.impl();
-  if (pc) {
-    pc->OnAlpnNegotiated(aAlpn);
+void PeerConnectionMedia::AlpnNegotiated_m(const std::string& aAlpn) {
+  if (mParent) {
+    mParent->OnAlpnNegotiated(aAlpn);
   }
 }
 
 /**
  * Tells you if any local track is isolated to a specific peer identity.
  * Obviously, we want all the tracks to be isolated equally so that they can
  * all be sent or not.  We check once when we are setting a local description
  * and that determines if we flip the "privacy requested" bit on.  Once the bit
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ -122,32 +122,17 @@ class PeerConnectionMedia : public sigsl
   // here.
   std::vector<RefPtr<TransceiverImpl>>& GetTransceivers() {
     return mTransceivers;
   }
 
   nsPIDOMWindowInner* GetWindow() const;
 
   void AlpnNegotiated_s(const std::string& aAlpn);
-  static void AlpnNegotiated_m(const std::string& aParentHandle,
-                               const std::string& aAlpn);
-
-  // ICE state signals
-  sigslot::signal1<mozilla::dom::RTCIceGatheringState>
-      SignalIceGatheringStateChange;
-  sigslot::signal1<mozilla::dom::RTCIceConnectionState>
-      SignalIceConnectionStateChange;
-  // This passes a candidate:... attribute, transport id, and ufrag
-  // end-of-candidates is signaled with the empty string
-  sigslot::signal3<const std::string&, const std::string&, const std::string&>
-      SignalCandidate;
-  // This passes address, port, transport id of the default candidate.
-  sigslot::signal5<const std::string&, uint16_t, const std::string&, uint16_t,
-                   const std::string&>
-      SignalUpdateDefaultCandidate;
+  void AlpnNegotiated_m(const std::string& aAlpn);
 
   // TODO: Move to PeerConnectionImpl
   RefPtr<WebRtcCallWrapper> mCall;
 
   // mtransport objects
   RefPtr<MediaTransportHandler> mTransportHandler;
 
  private: