Bug 906986 - Rework rollback/finalize to include a committed state. r=bwc, r=drno

MozReview-Commit-ID: z7uEn5xEBf

--HG--
extra : rebase_source : 18565a3c49045af9445a93de8753110169caf465

media/mtransport/transportlayerice.cpp

@@ -128,8 +128,6 @@ void TransportLayerIce::PostSetup() {
                                         &TransportLayerIce::IcePacketReceived);
   if (stream_->state() == NrIceMediaStream::ICE_OPEN) {
     TL_SET_STATE(TS_OPEN);
-    // Reset old ice stream if new stream is good
-    ResetOldStream();
   }
 }
 
@@ -137,7 +135,7 @@ void TransportLayerIce::ResetOldStream() {
   if (old_stream_ == nullptr) {
     return; // no work to do
   }
-  // ICE Ready on the new stream, we can forget the old stream now
+  // ICE restart successful on the new stream, we can forget the old stream now
   MOZ_MTLOG(ML_INFO, LAYER_INFO << "ResetOldStream(" << old_stream_->name()
                                 << ")");
   old_stream_->SignalReady.disconnect(this);
@@ -150,6 +148,7 @@ void TransportLayerIce::RestoreOldStream() {
   if (old_stream_ == nullptr) {
     return; // no work to do
   }
+  // ICE restart rollback, we need to restore the old stream
   MOZ_MTLOG(ML_INFO, LAYER_INFO << "RestoreOldStream(" << old_stream_->name()
                                 << ")");
   stream_->SignalReady.disconnect(this);
@@ -201,8 +200,6 @@ void TransportLayerIce::IceReady(NrIceMediaStream *stream) {
   MOZ_MTLOG(ML_INFO, LAYER_INFO << "ICE Ready(" << stream->name() << ","
     << component_ << ")");
   TL_SET_STATE(TS_OPEN);
-  // Reset old ice stream if new stream is good after ice restart
-  ResetOldStream();
 }
 
 void TransportLayerIce::IceFailed(NrIceMediaStream *stream) {

media/mtransport/transportlayerice.h

@@ -38,6 +38,9 @@ class TransportLayerIce : public TransportLayer {
                      RefPtr<NrIceMediaStream> stream,
                      int component);
 
+  void ResetOldStream(); // called after successful ice restart
+  void RestoreOldStream(); // called after unsuccessful ice restart
+
   // Transport layer overrides.
   virtual TransportResult SendPacket(const unsigned char *data, size_t len);
 
@@ -53,8 +56,6 @@ class TransportLayerIce : public TransportLayer {
  private:
   DISALLOW_COPY_ASSIGN(TransportLayerIce);
   void PostSetup();
-  void ResetOldStream(); // called after successful ice restart
-  void RestoreOldStream(); // called after unsuccessful ice restart
 
   const std::string name_;
   RefPtr<NrIceCtx> ctx_;

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

@@ -227,7 +227,7 @@ MediaPipelineFactory::CreateOrGetTransportFlow(
 
   flow = mPCMedia->GetTransportFlow(aLevel, aIsRtcp);
   if (flow) {
-    if (mPCMedia->ice_ctx_hdlr()->IsRestarting()) {
+    if (mPCMedia->IsIceRestarting()) {
       MOZ_MTLOG(ML_INFO, "Flow[" << flow->id() << "]: "
                                  << "detected ICE restart - level: "
                                  << aLevel << " rtcp: " << aIsRtcp);

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

@@ -1545,7 +1545,9 @@ PeerConnectionImpl::CreateOffer(const JsepOfferOptions& aOptions)
 {
   PC_AUTO_ENTER_API_CALL(true);
   bool restartIce = aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart);
-  if (!restartIce && mMedia->ice_ctx_hdlr()->IsRestarting()) {
+  if (!restartIce &&
+      mMedia->GetIceRestartState() ==
+          PeerConnectionMedia::ICE_RESTART_PROVISIONAL) {
     RollbackIceRestart();
   }
 
@@ -1565,8 +1567,21 @@ PeerConnectionImpl::CreateOffer(const JsepOfferOptions& aOptions)
   CSFLogDebug(logTag, "CreateOffer()");
 
   nsresult nrv;
-  if (aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart) &&
-      !mMedia->ice_ctx_hdlr()->IsRestarting()) {
+  if (restartIce) {
+    // 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) {
+      // we're mid-restart and can rollback
+      RollbackIceRestart();
+    } 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();
     if (NS_FAILED(nrv)) {
@@ -1625,13 +1640,18 @@ PeerConnectionImpl::CreateAnswer()
 
   nsresult nrv;
   if (mJsepSession->RemoteIceIsRestarting()) {
-    CSFLogInfo(logTag, "Answerer restarting ice");
-    nrv = SetupIceRestart();
-    if (NS_FAILED(nrv)) {
-      CSFLogError(logTag, "%s: SetupIceRestart failed, res=%u",
-                           __FUNCTION__,
-                           static_cast<unsigned>(nrv));
-      return nrv;
+    if (mMedia->GetIceRestartState() ==
+            PeerConnectionMedia::ICE_RESTART_COMMITTED) {
+      FinalizeIceRestart();
+    } else if (!mMedia->IsIceRestarting()) {
+      CSFLogInfo(logTag, "Answerer restarting ice");
+      nrv = SetupIceRestart();
+      if (NS_FAILED(nrv)) {
+        CSFLogError(logTag, "%s: SetupIceRestart failed, res=%u",
+                             __FUNCTION__,
+                             static_cast<unsigned>(nrv));
+        return nrv;
+      }
     }
   }
 
@@ -1669,7 +1689,7 @@ PeerConnectionImpl::CreateAnswer()
 nsresult
 PeerConnectionImpl::SetupIceRestart()
 {
-  if (mMedia->ice_ctx_hdlr()->IsRestarting()) {
+  if (mMedia->IsIceRestarting()) {
     CSFLogError(logTag, "%s: ICE already restarting",
                          __FUNCTION__);
     return NS_ERROR_UNEXPECTED;
@@ -1719,6 +1739,15 @@ PeerConnectionImpl::RollbackIceRestart()
   return NS_OK;
 }
 
+void
+PeerConnectionImpl::FinalizeIceRestart()
+{
+  mMedia->FinalizeIceRestart();
+  // clear the previous ice creds since they are no longer needed
+  mPreviousIceUfrag = "";
+  mPreviousIcePwd = "";
+}
+
 NS_IMETHODIMP
 PeerConnectionImpl::SetLocalDescription(int32_t aAction, const char* aSDP)
 {
@@ -2929,8 +2958,13 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState,
 
   bool fireNegotiationNeeded = false;
   if (mSignalingState == PCImplSignalingState::SignalingStable) {
-    if (rollback && mMedia->ice_ctx_hdlr()->IsRestarting()) {
-      RollbackIceRestart();
+    if (mMedia->GetIceRestartState() ==
+            PeerConnectionMedia::ICE_RESTART_PROVISIONAL) {
+      if (rollback) {
+        RollbackIceRestart();
+      } else {
+        mMedia->CommitIceRestart();
+      }
     }
 
     // Either negotiation is done, or we've rolled back. In either case, we
@@ -3227,11 +3261,8 @@ void PeerConnectionImpl::IceConnectionStateChange(
   if (mIceConnectionState == PCImplIceConnectionState::Connected ||
       mIceConnectionState == PCImplIceConnectionState::Completed ||
       mIceConnectionState == PCImplIceConnectionState::Failed) {
-    if (mMedia->ice_ctx_hdlr()->IsRestarting()) {
-      mMedia->FinalizeIceRestart();
-      // clear the previous ice creds since they are no longer needed
-      mPreviousIceUfrag = "";
-      mPreviousIcePwd = "";
+    if (mMedia->IsIceRestarting()) {
+      FinalizeIceRestart();
     }
   }
 

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

@@ -714,6 +714,7 @@ private:
 
   nsresult SetupIceRestart();
   nsresult RollbackIceRestart();
+  void FinalizeIceRestart();
 
 #if !defined(MOZILLA_EXTERNAL_LINKAGE)
   static void GetStatsForPCObserver_s(

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

@@ -239,7 +239,8 @@ PeerConnectionMedia::PeerConnectionMedia(PeerConnectionImpl *parent)
       mUuidGen(MakeUnique<PCUuidGenerator>()),
       mMainThread(mParent->GetMainThread()),
       mSTSThread(mParent->GetSTSThread()),
-      mProxyResolveCompleted(false) {
+      mProxyResolveCompleted(false),
+      mIceRestartState(ICE_RESTART_NONE) {
 }
 
 nsresult PeerConnectionMedia::Init(const std::vector<NrIceStunServer>& stun_servers,
@@ -586,11 +587,30 @@ PeerConnectionMedia::StartIceChecks_s(
   mIceCtxHdlr->ctx()->StartChecks();
 }
 
+bool
+PeerConnectionMedia::IsIceRestarting() const
+{
+  ASSERT_ON_THREAD(mMainThread);
+
+  return (mIceRestartState != ICE_RESTART_NONE);
+}
+
+PeerConnectionMedia::IceRestartState
+PeerConnectionMedia::GetIceRestartState() const
+{
+  ASSERT_ON_THREAD(mMainThread);
+
+  return mIceRestartState;
+}
+
 void
 PeerConnectionMedia::BeginIceRestart(const std::string& ufrag,
                                      const std::string& pwd)
 {
   ASSERT_ON_THREAD(mMainThread);
+  if (IsIceRestarting()) {
+    return;
+  }
 
   bool default_address_only = GetPrefDefaultAddressOnly();
   RefPtr<NrIceCtx> new_ctx = mIceCtxHdlr->CreateCtx(ufrag,
@@ -603,6 +623,8 @@ PeerConnectionMedia::BeginIceRestart(const std::string& ufrag,
                     &PeerConnectionMedia::BeginIceRestart_s,
                     new_ctx),
                 NS_DISPATCH_NORMAL);
+
+  mIceRestartState = ICE_RESTART_PROVISIONAL;
 }
 
 void
@@ -613,22 +635,37 @@ PeerConnectionMedia::BeginIceRestart_s(RefPtr<NrIceCtx> new_ctx)
   // hold the original context so we can disconnect signals if needed
   RefPtr<NrIceCtx> originalCtx = mIceCtxHdlr->ctx();
 
-  mIceCtxHdlr->BeginIceRestart(new_ctx);
-  if (mIceCtxHdlr->IsRestarting()) {
+  if (mIceCtxHdlr->BeginIceRestart(new_ctx)) {
     ConnectSignals(mIceCtxHdlr->ctx().get(), originalCtx.get());
   }
 }
 
+void
+PeerConnectionMedia::CommitIceRestart()
+{
+  ASSERT_ON_THREAD(mMainThread);
+  if (mIceRestartState != ICE_RESTART_PROVISIONAL) {
+    return;
+  }
+
+  mIceRestartState = ICE_RESTART_COMMITTED;
+}
+
 void
 PeerConnectionMedia::FinalizeIceRestart()
 {
   ASSERT_ON_THREAD(mMainThread);
+  if (!IsIceRestarting()) {
+    return;
+  }
 
   RUN_ON_THREAD(GetSTSThread(),
                 WrapRunnable(
                     RefPtr<PeerConnectionMedia>(this),
                     &PeerConnectionMedia::FinalizeIceRestart_s),
                 NS_DISPATCH_NORMAL);
+
+  mIceRestartState = ICE_RESTART_NONE;
 }
 
 void
@@ -636,6 +673,17 @@ PeerConnectionMedia::FinalizeIceRestart_s()
 {
   ASSERT_ON_THREAD(mSTSThread);
 
+  // reset old streams since we don't need them anymore
+  for (auto i = mTransportFlows.begin();
+       i != mTransportFlows.end();
+       ++i) {
+    RefPtr<TransportFlow> aFlow = i->second;
+    if (!aFlow) continue;
+    TransportLayerIce* ice =
+      static_cast<TransportLayerIce*>(aFlow->GetLayer(TransportLayerIce::ID()));
+    ice->ResetOldStream();
+  }
+
   mIceCtxHdlr->FinalizeIceRestart();
 }
 
@@ -643,6 +691,9 @@ void
 PeerConnectionMedia::RollbackIceRestart()
 {
   ASSERT_ON_THREAD(mMainThread);
+  if (mIceRestartState != ICE_RESTART_PROVISIONAL) {
+    return;
+  }
 
   RUN_ON_THREAD(GetSTSThread(),
                 WrapRunnable(
@@ -655,13 +706,21 @@ void
 PeerConnectionMedia::RollbackIceRestart_s()
 {
   ASSERT_ON_THREAD(mSTSThread);
-  if (!mIceCtxHdlr->IsRestarting()) {
-    return;
-  }
 
   // hold the restart context so we can disconnect signals
   RefPtr<NrIceCtx> restartCtx = mIceCtxHdlr->ctx();
 
+  // restore old streams since we're rolling back
+  for (auto i = mTransportFlows.begin();
+       i != mTransportFlows.end();
+       ++i) {
+    RefPtr<TransportFlow> aFlow = i->second;
+    if (!aFlow) continue;
+    TransportLayerIce* ice =
+      static_cast<TransportLayerIce*>(aFlow->GetLayer(TransportLayerIce::ID()));
+    ice->RestoreOldStream();
+  }
+
   mIceCtxHdlr->RollbackIceRestart();
   ConnectSignals(mIceCtxHdlr->ctx().get(), restartCtx.get());
 }

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

@@ -288,6 +288,11 @@ class PeerConnectionMedia : public sigslot::has_slots<> {
  public:
   explicit PeerConnectionMedia(PeerConnectionImpl *parent);
 
+  enum IceRestartState { ICE_RESTART_NONE,
+                         ICE_RESTART_PROVISIONAL,
+                         ICE_RESTART_COMMITTED
+  };
+
   PeerConnectionImpl* GetPC() { return mParent; }
   nsresult Init(const std::vector<NrIceStunServer>& stun_servers,
                 const std::vector<NrIceTurnServer>& turn_servers,
@@ -316,9 +321,14 @@ class PeerConnectionMedia : public sigslot::has_slots<> {
   // Start ICE checks.
   void StartIceChecks(const JsepSession& session);
 
+  bool IsIceRestarting() const;
+  IceRestartState GetIceRestartState() const;
+
   // Begin ICE restart
   void BeginIceRestart(const std::string& ufrag,
                        const std::string& pwd);
+  // Commit ICE Restart - offer/answer complete, no rollback possible
+  void CommitIceRestart();
   // Finalize ICE restart
   void FinalizeIceRestart();
   // Abort ICE restart
@@ -613,6 +623,9 @@ class PeerConnectionMedia : public sigslot::has_slots<> {
   // Used to store the result of the request.
   UniquePtr<NrIceProxyServer> mProxyServer;
 
+  // Used to track the state of ice restart
+  IceRestartState mIceRestartState;
+
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PeerConnectionMedia)
 };