Bug 1072044: Fire ICE signals on main. r=mt

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp

@@ -618,7 +618,7 @@ PeerConnectionImpl::Initialize(PeerConnectionObserver& aObserver,
       this,
       &PeerConnectionImpl::IceConnectionStateChange);
 
-  mMedia->SignalCandidate.connect(this, &PeerConnectionImpl::CandidateReady_s);
+  mMedia->SignalCandidate.connect(this, &PeerConnectionImpl::CandidateReady);
 
   // Initialize the media object.
   res = mMedia->Init(aConfiguration->getStunServers(),
@@ -2040,54 +2040,10 @@ toDomIceGatheringState(NrIceCtx::GatheringState state) {
   MOZ_CRASH();
 }
 
-// This is called from the STS thread and so we need to thunk
-// to the main thread.
-void PeerConnectionImpl::IceConnectionStateChange(
-    NrIceCtx* ctx,
-    NrIceCtx::ConnectionState state) {
-  (void)ctx;
-  // Do an async call here to unwind the stack. refptr keeps the PC alive.
-  nsRefPtr<PeerConnectionImpl> pc(this);
-  RUN_ON_THREAD(mThread,
-                WrapRunnable(pc,
-                             &PeerConnectionImpl::IceConnectionStateChange_m,
-                             toDomIceConnectionState(state)),
-                NS_DISPATCH_NORMAL);
-}
-
 void
-PeerConnectionImpl::IceGatheringStateChange(
+PeerConnectionImpl::CandidateReady(const std::string& candidate,
-    NrIceCtx* ctx,
+                                   uint16_t level) {
-    NrIceCtx::GatheringState state)
+  PC_AUTO_ENTER_API_CALL_VOID_RETURN(false);
-{
-  (void)ctx;
-  // Do an async call here to unwind the stack. refptr keeps the PC alive.
-  nsRefPtr<PeerConnectionImpl> pc(this);
-  RUN_ON_THREAD(mThread,
-                WrapRunnable(pc,
-                             &PeerConnectionImpl::IceGatheringStateChange_m,
-                             toDomIceGatheringState(state)),
-                NS_DISPATCH_NORMAL);
-}
-
-void
-PeerConnectionImpl::CandidateReady_s(const std::string& candidate,
-                                     uint16_t level)
-{
-  ASSERT_ON_THREAD(mSTSThread);
-  nsRefPtr<PeerConnectionImpl> pc(this);
-  RUN_ON_THREAD(mThread,
-                WrapRunnable(pc,
-                             &PeerConnectionImpl::CandidateReady_m,
-                             candidate,
-                             level),
-                NS_DISPATCH_NORMAL);
-}
-
-nsresult
-PeerConnectionImpl::CandidateReady_m(const std::string& candidate,
-                                     uint16_t level) {
-  PC_AUTO_ENTER_API_CALL(false);
 
   if (mLocalSDP.empty()) {
     // It is not appropriate to trickle yet; buffer.
@@ -2097,8 +2053,6 @@ PeerConnectionImpl::CandidateReady_m(const std::string& candidate,
       FoundIceCandidate(candidate, level);
     }
   }
-
-  return NS_OK;
 }
 
 void
@@ -2110,7 +2064,7 @@ PeerConnectionImpl::StartTrickle() {
   }
 
   // If the buffer was empty to begin with, we have already sent the
-  // end-of-candidates event in IceGatheringStateChange_m.
+  // end-of-candidates event in IceGatheringStateChange.
   if (mIceGatheringState == PCImplIceGatheringState::Complete &&
       !mCandidateBuffer.empty()) {
     SendLocalIceCandidateToContent(0, "", "");
@@ -2199,33 +2153,35 @@ static bool isFailed(PCImplIceConnectionState state) {
 }
 #endif
 
-nsresult
+void PeerConnectionImpl::IceConnectionStateChange(
-PeerConnectionImpl::IceConnectionStateChange_m(PCImplIceConnectionState aState)
+    NrIceCtx* ctx,
-{
+    NrIceCtx::ConnectionState state) {
-  PC_AUTO_ENTER_API_CALL(false);
+  PC_AUTO_ENTER_API_CALL_VOID_RETURN(false);
 
   CSFLogDebug(logTag, "%s", __FUNCTION__);
 
+  auto domState = toDomIceConnectionState(state);
+
 #ifdef MOZILLA_INTERNAL_API
-  if (!isDone(mIceConnectionState) && isDone(aState)) {
+  if (!isDone(mIceConnectionState) && isDone(domState)) {
     // mIceStartTime can be null if going directly from New to Closed, in which
     // case we don't count it as a success or a failure.
     if (!mIceStartTime.IsNull()){
       TimeDuration timeDelta = TimeStamp::Now() - mIceStartTime;
-      if (isSucceeded(aState)) {
+      if (isSucceeded(domState)) {
         Telemetry::Accumulate(Telemetry::WEBRTC_ICE_SUCCESS_TIME,
                               timeDelta.ToMilliseconds());
-      } else if (isFailed(aState)) {
+      } else if (isFailed(domState)) {
         Telemetry::Accumulate(Telemetry::WEBRTC_ICE_FAILURE_TIME,
                               timeDelta.ToMilliseconds());
       }
     }
 
-    if (isSucceeded(aState)) {
+    if (isSucceeded(domState)) {
       Telemetry::Accumulate(
           Telemetry::WEBRTC_ICE_ADD_CANDIDATE_ERRORS_GIVEN_SUCCESS,
           mAddCandidateErrorCount);
-    } else if (isFailed(aState)) {
+    } else if (isFailed(domState)) {
       Telemetry::Accumulate(
           Telemetry::WEBRTC_ICE_ADD_CANDIDATE_ERRORS_GIVEN_FAILURE,
           mAddCandidateErrorCount);
@@ -2233,7 +2189,7 @@ PeerConnectionImpl::IceConnectionStateChange_m(PCImplIceConnectionState aState)
   }
 #endif
 
-  mIceConnectionState = aState;
+  mIceConnectionState = domState;
 
   // 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...
@@ -2269,7 +2225,7 @@ PeerConnectionImpl::IceConnectionStateChange_m(PCImplIceConnectionState aState)
 
   nsRefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
-    return NS_OK;
+    return;
   }
   WrappableJSErrorResult rv;
   RUN_ON_THREAD(mThread,
@@ -2278,17 +2234,18 @@ PeerConnectionImpl::IceConnectionStateChange_m(PCImplIceConnectionState aState)
                              PCObserverStateType::IceConnectionState,
                              rv, static_cast<JSCompartment*>(nullptr)),
                 NS_DISPATCH_NORMAL);
-  return NS_OK;
 }
 
-nsresult
+void
-PeerConnectionImpl::IceGatheringStateChange_m(PCImplIceGatheringState aState)
+PeerConnectionImpl::IceGatheringStateChange(
+    NrIceCtx* ctx,
+    NrIceCtx::GatheringState state)
 {
-  PC_AUTO_ENTER_API_CALL(false);
+  PC_AUTO_ENTER_API_CALL_VOID_RETURN(false);
 
   CSFLogDebug(logTag, "%s", __FUNCTION__);
 
-  mIceGatheringState = aState;
+  mIceGatheringState = toDomIceGatheringState(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...
@@ -2308,7 +2265,7 @@ PeerConnectionImpl::IceGatheringStateChange_m(PCImplIceGatheringState aState)
 
   nsRefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
-    return NS_OK;
+    return;
   }
   WrappableJSErrorResult rv;
   RUN_ON_THREAD(mThread,
@@ -2322,8 +2279,6 @@ PeerConnectionImpl::IceGatheringStateChange_m(PCImplIceGatheringState aState)
       mCandidateBuffer.empty()) {
     SendLocalIceCandidateToContent(0, "", "");
   }
-
-  return NS_OK;
 }
 
 #ifdef MOZILLA_INTERNAL_API

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h

@@ -204,6 +204,12 @@ class RTCStatsQuery {
       nsresult res = CheckApiState(assert_ice_ready);             \
       if (NS_FAILED(res)) return res; \
     } while(0)
+#define PC_AUTO_ENTER_API_CALL_VOID_RETURN(assert_ice_ready) \
+    do { \
+      /* do/while prevents res from conflicting with locals */    \
+      nsresult res = CheckApiState(assert_ice_ready);             \
+      if (NS_FAILED(res)) return; \
+    } while(0)
 #define PC_AUTO_ENTER_API_CALL_NO_CHECK() CheckThread()
 
 class PeerConnectionImpl MOZ_FINAL : public nsISupports,
@@ -629,14 +635,7 @@ private:
   // Shut down media - called on main thread only
   void ShutdownMedia();
 
-  // ICE callbacks run on the right thread.
+  void CandidateReady(const std::string& candidate, uint16_t level);
-  nsresult IceConnectionStateChange_m(
-      mozilla::dom::PCImplIceConnectionState aState);
-  nsresult IceGatheringStateChange_m(
-      mozilla::dom::PCImplIceGatheringState aState);
-
-  void CandidateReady_s(const std::string& candidate, uint16_t level);
-  nsresult CandidateReady_m(const std::string& candidate, uint16_t level);
   void SendLocalIceCandidateToContent(uint16_t level,
                                       const std::string& mid,
                                       const std::string& candidate);

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp

@@ -240,10 +240,10 @@ nsresult PeerConnectionMedia::Init(const std::vector<NrIceStunServer>& stun_serv
   }
   mIceCtx->SignalGatheringStateChange.connect(
       this,
-      &PeerConnectionMedia::IceGatheringStateChange);
+      &PeerConnectionMedia::IceGatheringStateChange_s);
   mIceCtx->SignalConnectionStateChange.connect(
       this,
-      &PeerConnectionMedia::IceConnectionStateChange);
+      &PeerConnectionMedia::IceConnectionStateChange_s);
 
   // Create three streams to start with.
   // One each for audio, video and DataChannel
@@ -283,7 +283,7 @@ nsresult PeerConnectionMedia::Init(const std::vector<NrIceStunServer>& stun_serv
     mIceStreams[i]->SignalReady.connect(this, &PeerConnectionMedia::IceStreamReady);
     mIceStreams[i]->SignalCandidate.connect(
         this,
-        &PeerConnectionMedia::OnCandidateFound);
+        &PeerConnectionMedia::OnCandidateFound_s);
   }
 
   // TODO(ekr@rtfm.com): When we have a generic error reporting mechanism,
@@ -591,16 +591,73 @@ PeerConnectionMedia::AddRemoteStreamHint(int aIndex, bool aIsVideo)
 
 
 void
-PeerConnectionMedia::IceGatheringStateChange(NrIceCtx* ctx,
+PeerConnectionMedia::IceGatheringStateChange_s(NrIceCtx* ctx,
-                                             NrIceCtx::GatheringState state)
+                                               NrIceCtx::GatheringState state)
 {
+  ASSERT_ON_THREAD(mSTSThread);
+  // ShutdownMediaTransport_s has not run yet because it unhooks this function
+  // from its signal, which means that SelfDestruct_m has not been dispatched
+  // yet either, so this PCMedia will still be around when this dispatch reaches
+  // main.
+  GetMainThread()->Dispatch(
+    WrapRunnable(this,
+                 &PeerConnectionMedia::IceGatheringStateChange_m,
+                 ctx,
+                 state),
+    NS_DISPATCH_NORMAL);
+}
+
+void
+PeerConnectionMedia::IceConnectionStateChange_s(NrIceCtx* ctx,
+                                                NrIceCtx::ConnectionState state)
+{
+  ASSERT_ON_THREAD(mSTSThread);
+  // ShutdownMediaTransport_s has not run yet because it unhooks this function
+  // from its signal, which means that SelfDestruct_m has not been dispatched
+  // yet either, so this PCMedia will still be around when this dispatch reaches
+  // main.
+  GetMainThread()->Dispatch(
+    WrapRunnable(this,
+                 &PeerConnectionMedia::IceConnectionStateChange_m,
+                 ctx,
+                 state),
+    NS_DISPATCH_NORMAL);
+}
+
+void
+PeerConnectionMedia::OnCandidateFound_s(NrIceMediaStream *aStream,
+                                        const std::string &candidate)
+{
+  ASSERT_ON_THREAD(mSTSThread);
+  MOZ_ASSERT(aStream);
+
+  CSFLogDebug(logTag, "%s: %s", __FUNCTION__, aStream->name().c_str());
+
+  // ShutdownMediaTransport_s has not run yet because it unhooks this function
+  // from its signal, which means that SelfDestruct_m has not been dispatched
+  // yet either, so this PCMedia will still be around when this dispatch reaches
+  // main.
+  GetMainThread()->Dispatch(
+    WrapRunnable(this,
+                 &PeerConnectionMedia::OnCandidateFound_m,
+                 candidate,
+                 aStream->GetLevel()),
+    NS_DISPATCH_NORMAL);
+}
+
+void
+PeerConnectionMedia::IceGatheringStateChange_m(NrIceCtx* ctx,
+                                               NrIceCtx::GatheringState state)
+{
+  ASSERT_ON_THREAD(mMainThread);
   SignalIceGatheringStateChange(ctx, state);
 }
 
 void
-PeerConnectionMedia::IceConnectionStateChange(NrIceCtx* ctx,
+PeerConnectionMedia::IceConnectionStateChange_m(NrIceCtx* ctx,
-                                              NrIceCtx::ConnectionState state)
+                                                NrIceCtx::ConnectionState state)
 {
+  ASSERT_ON_THREAD(mMainThread);
   SignalIceConnectionStateChange(ctx, state);
 }
 
@@ -613,14 +670,11 @@ PeerConnectionMedia::IceStreamReady(NrIceMediaStream *aStream)
 }
 
 void
-PeerConnectionMedia::OnCandidateFound(NrIceMediaStream *aStream,
+PeerConnectionMedia::OnCandidateFound_m(const std::string &candidate,
-                                      const std::string &candidate)
+                                        uint16_t level)
 {
-  MOZ_ASSERT(aStream);
+  ASSERT_ON_THREAD(mMainThread);
-
+  SignalCandidate(candidate, level);
-  CSFLogDebug(logTag, "%s: %s", __FUNCTION__, aStream->name().c_str());
-
-  SignalCandidate(candidate, aStream->GetLevel());
 }
 
 

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h

@@ -415,14 +415,21 @@ class PeerConnectionMedia : public sigslot::has_slots<> {
   void SelfDestruct_m();
 
   // ICE events
-  void IceGatheringStateChange(mozilla::NrIceCtx* ctx,
+  void IceGatheringStateChange_s(mozilla::NrIceCtx* ctx,
                                mozilla::NrIceCtx::GatheringState state);
-  void IceConnectionStateChange(mozilla::NrIceCtx* ctx,
+  void IceConnectionStateChange_s(mozilla::NrIceCtx* ctx,
                                 mozilla::NrIceCtx::ConnectionState state);
   void IceStreamReady(mozilla::NrIceMediaStream *aStream);
-  void OnCandidateFound(mozilla::NrIceMediaStream *aStream,
+  void OnCandidateFound_s(mozilla::NrIceMediaStream *aStream,
                         const std::string &candidate);
 
+  void IceGatheringStateChange_m(mozilla::NrIceCtx* ctx,
+                                 mozilla::NrIceCtx::GatheringState state);
+  void IceConnectionStateChange_m(mozilla::NrIceCtx* ctx,
+                                  mozilla::NrIceCtx::ConnectionState state);
+  void OnCandidateFound_m(const std::string &candidate, uint16_t level);
+
+
   // The parent PC
   PeerConnectionImpl *mParent;
   // and a loose handle on it for event driven stuff