Bug 1429085 - only initiate ice restart in PeerConnectionMedia if jsep create offer/answer succeeds. r=drno
authorMichael Froman <mfroman@mozilla.com>
Fri, 12 Jan 2018 15:17:50 -0600
changeset 453431 29006b3597ccb3e279af3aa61045fb14a57d0e56
parent 453430 c51cdba4c57d0e6b1c22e826d425c3329374fa35
child 453432 a16df11dae3fba36cb958b06917f35d9d0591f78
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdrno
bugs1429085
milestone59.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 1429085 - only initiate ice restart in PeerConnectionMedia if jsep create offer/answer succeeds. r=drno Separate setting up the ice credentials for ice restart from the actual restart call into PeerConnectionMedia. This allows waiting until after the call to JsepSessionImpl::CreateOffer or JsepSessionImpl::CreateAnswer succeeds. MozReview-Commit-ID: Hex0lNstv0H
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -1502,16 +1502,17 @@ PeerConnectionImpl::CreateOffer(const Js
     PeerConnectionCtx::GetInstance()->queueJSEPOperation(
         WrapRunnableNM(DeferredCreateOffer, mHandle, aOptions));
     STAMP_TIMECARD(mTimeCard, "Deferring CreateOffer (not ready)");
     return NS_OK;
   }
 
   CSFLogDebug(LOGTAG, "CreateOffer()");
 
+  bool iceRestartPrimed = false;
   nsresult nrv;
   if (restartIce &&
       !mJsepSession->GetLocalDescription(kJsepDescriptionCurrent).empty()) {
     // If restart is requested and a restart is already in progress, we
     // need to make room for the restart request so we either rollback
     // or finalize to "clear" the previous restart.
     if (mMedia->GetIceRestartState() ==
             PeerConnectionMedia::ICE_RESTART_PROVISIONAL) {
@@ -1520,23 +1521,24 @@ PeerConnectionImpl::CreateOffer(const Js
     } else if (mMedia->GetIceRestartState() ==
                    PeerConnectionMedia::ICE_RESTART_COMMITTED) {
       // we're mid-restart and can't rollback, finalize restart even
       // though we're not really ready yet
       FinalizeIceRestart();
     }
 
     CSFLogInfo(LOGTAG, "Offerer restarting ice");
-    nrv = SetupIceRestart();
+    nrv = SetupIceRestartCredentials();
     if (NS_FAILED(nrv)) {
       CSFLogError(LOGTAG, "%s: SetupIceRestart failed, res=%u",
                            __FUNCTION__,
                            static_cast<unsigned>(nrv));
       return nrv;
     }
+    iceRestartPrimed = true;
   }
 
   nrv = ConfigureJsepSessionCodecs();
   if (NS_FAILED(nrv)) {
     CSFLogError(LOGTAG, "Failed to configure codecs");
     return nrv;
   }
 
@@ -1554,18 +1556,30 @@ PeerConnectionImpl::CreateOffer(const Js
         break;
       default:
         error = kInternalError;
     }
     std::string errorString = mJsepSession->GetLastError();
 
     CSFLogError(LOGTAG, "%s: pc = %s, error = %s",
                 __FUNCTION__, mHandle.c_str(), errorString.c_str());
+
+    if (iceRestartPrimed) {
+      // reset the ice credentials because CreateOffer failed
+      ResetIceCredentials();
+    }
+
     pco->OnCreateOfferError(error, ObString(errorString.c_str()), rv);
   } else {
+    // wait until we know CreateOffer succeeds before we actually start
+    // the ice restart gears turning.
+    if (iceRestartPrimed) {
+      BeginIceRestart();
+    }
+
     UpdateSignalingState();
     pco->OnCreateOfferSuccess(ObString(offer.c_str()), rv);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -1575,30 +1589,32 @@ PeerConnectionImpl::CreateAnswer()
 
   RefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
     return NS_OK;
   }
 
   CSFLogDebug(LOGTAG, "CreateAnswer()");
 
+  bool iceRestartPrimed = false;
   nsresult nrv;
   if (mJsepSession->RemoteIceIsRestarting()) {
     if (mMedia->GetIceRestartState() ==
             PeerConnectionMedia::ICE_RESTART_COMMITTED) {
       FinalizeIceRestart();
     } else if (!mMedia->IsIceRestarting()) {
       CSFLogInfo(LOGTAG, "Answerer restarting ice");
-      nrv = SetupIceRestart();
+      nrv = SetupIceRestartCredentials();
       if (NS_FAILED(nrv)) {
         CSFLogError(LOGTAG, "%s: SetupIceRestart failed, res=%u",
                              __FUNCTION__,
                              static_cast<unsigned>(nrv));
         return nrv;
       }
+      iceRestartPrimed = true;
     }
   }
 
   STAMP_TIMECARD(mTimeCard, "Create Answer");
   // TODO(bug 1098015): Once RTCAnswerOptions is standardized, we'll need to
   // add it as a param to CreateAnswer, and convert it here.
   JsepAnswerOptions options;
   std::string answer;
@@ -1613,27 +1629,39 @@ PeerConnectionImpl::CreateAnswer()
         break;
       default:
         error = kInternalError;
     }
     std::string errorString = mJsepSession->GetLastError();
 
     CSFLogError(LOGTAG, "%s: pc = %s, error = %s",
                 __FUNCTION__, mHandle.c_str(), errorString.c_str());
+
+    if (iceRestartPrimed) {
+      // reset the ice credentials because CreateAnswer failed
+      ResetIceCredentials();
+    }
+
     pco->OnCreateAnswerError(error, ObString(errorString.c_str()), rv);
   } else {
+    // wait until we know CreateAnswer succeeds before we actually start
+    // the ice restart gears turning.
+    if (iceRestartPrimed) {
+      BeginIceRestart();
+    }
+
     UpdateSignalingState();
     pco->OnCreateAnswerSuccess(ObString(answer.c_str()), rv);
   }
 
   return NS_OK;
 }
 
 nsresult
-PeerConnectionImpl::SetupIceRestart()
+PeerConnectionImpl::SetupIceRestartCredentials()
 {
   if (mMedia->IsIceRestarting()) {
     CSFLogError(LOGTAG, "%s: ICE already restarting",
                          __FUNCTION__);
     return NS_ERROR_UNEXPECTED;
   }
 
   std::string ufrag = mMedia->ice_ctx()->GetNewUfrag();
@@ -1643,47 +1671,59 @@ PeerConnectionImpl::SetupIceRestart()
                          __FUNCTION__,
                          ufrag.c_str(), pwd.c_str());
     return NS_ERROR_UNEXPECTED;
   }
 
   // hold on to the current ice creds in case of rollback
   mPreviousIceUfrag = mJsepSession->GetUfrag();
   mPreviousIcePwd = mJsepSession->GetPwd();
-  mMedia->BeginIceRestart(ufrag, pwd);
 
   nsresult nrv = mJsepSession->SetIceCredentials(ufrag, pwd);
   if (NS_FAILED(nrv)) {
     CSFLogError(LOGTAG, "%s: Couldn't set ICE credentials, res=%u",
                          __FUNCTION__,
                          static_cast<unsigned>(nrv));
     return nrv;
   }
 
   return NS_OK;
 }
 
+void
+PeerConnectionImpl::BeginIceRestart()
+{
+  mMedia->BeginIceRestart(mJsepSession->GetUfrag(), mJsepSession->GetPwd());
+}
+
+nsresult
+PeerConnectionImpl::ResetIceCredentials()
+{
+  nsresult nrv = mJsepSession->SetIceCredentials(mPreviousIceUfrag, mPreviousIcePwd);
+  mPreviousIceUfrag = "";
+  mPreviousIcePwd = "";
+
+  if (NS_FAILED(nrv)) {
+    CSFLogError(LOGTAG, "%s: Couldn't reset ICE credentials, res=%u",
+                         __FUNCTION__,
+                         static_cast<unsigned>(nrv));
+    return nrv;
+  }
+
+  return NS_OK;
+}
+
 nsresult
 PeerConnectionImpl::RollbackIceRestart()
 {
   mMedia->RollbackIceRestart();
+  ++mIceRollbackCount;
+
   // put back the previous ice creds
-  nsresult nrv = mJsepSession->SetIceCredentials(mPreviousIceUfrag,
-                                                 mPreviousIcePwd);
-  if (NS_FAILED(nrv)) {
-    CSFLogError(LOGTAG, "%s: Couldn't set ICE credentials, res=%u",
-                         __FUNCTION__,
-                         static_cast<unsigned>(nrv));
-    return nrv;
-  }
-  mPreviousIceUfrag = "";
-  mPreviousIcePwd = "";
-  ++mIceRollbackCount;
-
-  return NS_OK;
+  return ResetIceCredentials();
 }
 
 void
 PeerConnectionImpl::FinalizeIceRestart()
 {
   mMedia->FinalizeIceRestart();
   // clear the previous ice creds since they are no longer needed
   mPreviousIceUfrag = "";
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -708,17 +708,19 @@ private:
       uint16_t* level) const;
 
   nsresult AddRtpTransceiverToJsepSession(RefPtr<JsepTransceiver>& transceiver);
   already_AddRefed<TransceiverImpl> CreateTransceiverImpl(
       JsepTransceiver* aJsepTransceiver,
       dom::MediaStreamTrack* aSendTrack,
       ErrorResult& aRv);
 
-  nsresult SetupIceRestart();
+  nsresult SetupIceRestartCredentials();
+  void BeginIceRestart();
+  nsresult ResetIceCredentials();
   nsresult RollbackIceRestart();
   void FinalizeIceRestart();
 
   static void GetStatsForPCObserver_s(
       const std::string& pcHandle,
       nsAutoPtr<RTCStatsQuery> query);
 
   // Sends an RTCStatsReport to JS. Must run on main thread.