Bug 1429085 - only initiate ice restart in PeerConnectionMedia if jsep create offer/answer succeeds. r?drno draft
authorMichael Froman <mfroman@mozilla.com>
Fri, 12 Jan 2018 15:17:50 -0600
changeset 719936 f2b3c261af8f29d101854db806d25efa6a550cea
parent 719729 8460d515739cc6609f985a9ece90711700818f06
child 745929 7c7b900f452f173cbcd50fe95d63196f8ad1c156
push id95398
push userbmo:mfroman@nostrum.com
push dateFri, 12 Jan 2018 22:39:34 +0000
reviewersdrno
bugs1429085
milestone59.0a1
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
@@ -710,17 +710,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.