Bug 906986 - Rework rollback/finalize to include a committed state. draft
authorMichael Froman <mfroman@mozilla.com>
Tue, 05 Apr 2016 20:12:04 -0500
changeset 348072 154d8aa990138a0a652eecc2e388ad44937330c9
parent 348071 3040c6537ad783f17bb675de106f6a6134505219
child 517774 e2b5677ec49b6dd8eb299a3f4e1bb2e5dad2a65f
push id14740
push usermfroman@nostrum.com
push dateWed, 06 Apr 2016 14:28:49 +0000
bugs906986
milestone48.0a1
Bug 906986 - Rework rollback/finalize to include a committed state. MozReview-Commit-ID: z7uEn5xEBf
media/mtransport/nricectxhandler.cpp
media/mtransport/nricectxhandler.h
media/mtransport/transportlayerice.cpp
media/mtransport/transportlayerice.h
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/media/mtransport/nricectxhandler.cpp
+++ b/media/mtransport/nricectxhandler.cpp
@@ -16,17 +16,18 @@ namespace mozilla {
 
 MOZ_MTLOG_MODULE("mtransport")
 
 NrIceCtxHandler::NrIceCtxHandler(const std::string& name,
                                  bool offerer,
                                  NrIceCtx::Policy policy)
   : current_ctx(new NrIceCtx(name, offerer, policy)),
     old_ctx(nullptr),
-    restart_count(0)
+    restart_count(0),
+    restart_state(ICE_RESTART_NONE)
 {
 }
 
 RefPtr<NrIceCtxHandler>
 NrIceCtxHandler::Create(const std::string& name,
                         bool offerer,
                         bool allow_loopback,
                         bool tcp_enabled,
@@ -88,44 +89,60 @@ NrIceCtxHandler::CreateCtx(const std::st
   return new_ctx;
 }
 
 
 bool
 NrIceCtxHandler::BeginIceRestart(RefPtr<NrIceCtx> new_ctx)
 {
   MOZ_ASSERT(!old_ctx, "existing ice restart in progress");
-  if (old_ctx) {
+  if (restart_state != ICE_RESTART_NONE) {
     MOZ_MTLOG(ML_ERROR, "Existing ice restart in progress");
     return false; // ice restart already in progress
   }
 
   if (new_ctx == nullptr) {
     return false;
   }
 
   ++restart_count;
   old_ctx = current_ctx;
   current_ctx = new_ctx;
+  restart_state = ICE_RESTART_IN_PROGRESS;
   return true;
 }
 
 
 void
+NrIceCtxHandler::CommitIceRestart()
+{
+  if (restart_state != ICE_RESTART_IN_PROGRESS) {
+    return;
+  }
+
+  // called after offer/answer completes, not allowed to rollback afterwards
+  restart_state = ICE_RESTART_COMMITTED;
+}
+
+
+void
 NrIceCtxHandler::FinalizeIceRestart()
 {
   // no harm calling this even if we're not in the middle of restarting
   old_ctx = nullptr;
+  restart_state = ICE_RESTART_NONE;
 }
 
 
 void
 NrIceCtxHandler::RollbackIceRestart()
 {
-  if (old_ctx == nullptr) {
+  if (restart_state != ICE_RESTART_IN_PROGRESS) {
     return;
   }
+
   current_ctx = old_ctx;
   old_ctx = nullptr;
+  restart_state = ICE_RESTART_NONE;
 }
 
 
 } // close namespace
--- a/media/mtransport/nricectxhandler.h
+++ b/media/mtransport/nricectxhandler.h
@@ -2,16 +2,21 @@
 #define nricectxhandler_h__
 
 #include "nricectx.h"
 
 namespace mozilla {
 
 class NrIceCtxHandler {
 public:
+  enum RestartState { ICE_RESTART_NONE,
+                      ICE_RESTART_IN_PROGRESS,
+                      ICE_RESTART_COMMITTED
+  };
+
   // TODO(ekr@rtfm.com): Too many bools here. Bug 1193437.
   static RefPtr<NrIceCtxHandler> Create(const std::string& name,
                                         bool offerer,
                                         bool allow_loopback = false,
                                         bool tcp_enabled = true,
                                         bool allow_link_local = false,
                                         bool hide_non_default = false,
                                         NrIceCtx::Policy policy =
@@ -24,17 +29,19 @@ public:
   RefPtr<NrIceCtx> CreateCtx(bool hide_non_default = false) const; // for test
   RefPtr<NrIceCtx> CreateCtx(const std::string& ufrag,
                              const std::string& pwd,
                              bool hide_non_default) const;
 
   RefPtr<NrIceCtx> ctx() { return current_ctx; }
 
   bool BeginIceRestart(RefPtr<NrIceCtx> new_ctx);
-  bool IsRestarting() const { return (old_ctx != nullptr); }
+  bool IsRestarting() const { return (restart_state != ICE_RESTART_NONE); }
+  RestartState GetRestartState() const { return restart_state; }
+  void CommitIceRestart(); // point of no return, can't rollback after this
   void FinalizeIceRestart();
   void RollbackIceRestart();
 
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrIceCtxHandler)
 
 private:
   NrIceCtxHandler(const std::string& name,
@@ -42,13 +49,14 @@ private:
                   NrIceCtx::Policy policy);
   NrIceCtxHandler() = delete;
   ~NrIceCtxHandler() {}
   DISALLOW_COPY_ASSIGN(NrIceCtxHandler);
 
   RefPtr<NrIceCtx> current_ctx;
   RefPtr<NrIceCtx> old_ctx; // for while restart is in progress
   int restart_count; // used to differentiate streams between restarted ctx
+  RestartState restart_state;
 };
 
 } // close namespace
 
 #endif // nricectxhandler_h__
--- a/media/mtransport/transportlayerice.cpp
+++ b/media/mtransport/transportlayerice.cpp
@@ -123,38 +123,37 @@ void TransportLayerIce::PostSetup() {
   target_ = ctx_->thread();
 
   stream_->SignalReady.connect(this, &TransportLayerIce::IceReady);
   stream_->SignalFailed.connect(this, &TransportLayerIce::IceFailed);
   stream_->SignalPacketReceived.connect(this,
                                         &TransportLayerIce::IcePacketReceived);
   if (stream_->state() == NrIceMediaStream::ICE_OPEN) {
     TL_SET_STATE(TS_OPEN);
-    // Reset old ice stream if new stream is good
-    ResetOldStream();
   }
 }
 
 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);
   old_stream_->SignalFailed.disconnect(this);
   old_stream_->SignalPacketReceived.disconnect(this);
   old_stream_ = nullptr;
 }
 
 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);
   stream_->SignalFailed.disconnect(this);
   stream_->SignalPacketReceived.disconnect(this);
   stream_ = old_stream_;
   old_stream_ = nullptr;
 
@@ -196,18 +195,16 @@ void TransportLayerIce::IceReady(NrIceMe
   CheckThread();
   // only handle the current stream (not the old stream during restart)
   if (stream != stream_) {
     return;
   }
   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) {
   CheckThread();
   // only handle the current stream (not the old stream during restart)
   if (stream != stream_) {
     return;
   }
--- a/media/mtransport/transportlayerice.h
+++ b/media/mtransport/transportlayerice.h
@@ -33,33 +33,34 @@ class TransportLayerIce : public Transpo
   explicit TransportLayerIce(const std::string& name);
 
   virtual ~TransportLayerIce();
 
   void SetParameters(RefPtr<NrIceCtx> ctx,
                      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);
 
   // Slots for ICE
   void IceCandidate(NrIceMediaStream *stream, const std::string&);
   void IceReady(NrIceMediaStream *stream);
   void IceFailed(NrIceMediaStream *stream);
   void IcePacketReceived(NrIceMediaStream *stream, int component,
                          const unsigned char *data, int len);
 
   TRANSPORT_LAYER_ID("ice")
 
  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_;
   RefPtr<NrIceMediaStream> stream_;
   int component_;
 
   // used to hold the old stream
   RefPtr<NrIceMediaStream> old_stream_;
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -1561,17 +1561,19 @@ static void DeferredCreateOffer(const st
 }
 
 // Used by unit tests and the IDL CreateOffer.
 NS_IMETHODIMP
 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->ice_ctx_hdlr()->GetRestartState() == 
+          NrIceCtxHandler::ICE_RESTART_IN_PROGRESS) {
     RollbackIceRestart();
   }
 
   RefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
     return NS_OK;
   }
 
@@ -1581,18 +1583,31 @@ PeerConnectionImpl::CreateOffer(const Js
         WrapRunnableNM(DeferredCreateOffer, mHandle, aOptions));
     STAMP_TIMECARD(mTimeCard, "Deferring CreateOffer (not ready)");
     return NS_OK;
   }
 
   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->ice_ctx_hdlr()->GetRestartState() ==
+            NrIceCtxHandler::ICE_RESTART_IN_PROGRESS) {
+      // we're mid-restart and can rollback
+      RollbackIceRestart();
+    } else if (mMedia->ice_ctx_hdlr()->GetRestartState() ==
+                   NrIceCtxHandler::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)) {
       CSFLogError(logTag, "%s: SetupIceRestart failed, res=%u",
                            __FUNCTION__,
                            static_cast<unsigned>(nrv));
       return nrv;
     }
@@ -1641,23 +1656,28 @@ PeerConnectionImpl::CreateAnswer()
   if (!pco) {
     return NS_OK;
   }
 
   CSFLogDebug(logTag, "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->ice_ctx_hdlr()->GetRestartState() ==
+            NrIceCtxHandler::ICE_RESTART_COMMITTED) {
+      FinalizeIceRestart();
+    } else if (!mMedia->ice_ctx_hdlr()->IsRestarting()) {
+      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;
+      }
     }
   }
 
   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;
@@ -1735,16 +1755,25 @@ PeerConnectionImpl::RollbackIceRestart()
     return nrv;
   }
   mPreviousIceUfrag = "";
   mPreviousIcePwd = "";
 
   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)
 {
   PC_AUTO_ENTER_API_CALL(true);
 
   if (!aSDP) {
     CSFLogError(logTag, "%s - aSDP is NULL", __FUNCTION__);
     return NS_ERROR_FAILURE;
@@ -2868,18 +2897,22 @@ PeerConnectionImpl::SetSignalingState_m(
        !rollback)) {
     mMedia->EnsureTransports(*mJsepSession);
   }
 
   mSignalingState = aSignalingState;
 
   bool fireNegotiationNeeded = false;
   if (mSignalingState == PCImplSignalingState::SignalingStable) {
-    if (rollback && mMedia->ice_ctx_hdlr()->IsRestarting()) {
+    if (rollback &&
+        mMedia->ice_ctx_hdlr()->GetRestartState() ==
+            NrIceCtxHandler::ICE_RESTART_IN_PROGRESS) {
       RollbackIceRestart();
+    } else if (mMedia->ice_ctx_hdlr()->IsRestarting()) {
+      mMedia->CommitIceRestart();
     }
 
     // Either negotiation is done, or we've rolled back. In either case, we
     // need to re-evaluate whether further negotiation is required.
     mNegotiationNeeded = false;
     // If we're rolling back a local offer, we might need to remove some
     // transports, but nothing further needs to be done.
     mMedia->ActivateOrRemoveTransports(*mJsepSession);
@@ -3167,20 +3200,17 @@ void PeerConnectionImpl::IceConnectionSt
 #endif
 
   mIceConnectionState = domState;
 
   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 = "";
+      FinalizeIceRestart();
     }
   }
 
   // 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 (mIceConnectionState) {
     case PCImplIceConnectionState::New:
       STAMP_TIMECARD(mTimeCard, "Ice state: new");
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -708,16 +708,17 @@ private:
                                             const std::string& trackId);
 
   nsresult AddTrackToJsepSession(SdpMediaSection::MediaType type,
                                  const std::string& streamId,
                                  const std::string& trackId);
 
   nsresult SetupIceRestart();
   nsresult RollbackIceRestart();
+  void FinalizeIceRestart();
 
 #if !defined(MOZILLA_EXTERNAL_LINKAGE)
   static void GetStatsForPCObserver_s(
       const std::string& pcHandle,
       nsAutoPtr<RTCStatsQuery> query);
 
   // Sends an RTCStatsReport to JS. Must run on main thread.
   static void DeliverStatsReportToPCObserver_m(
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ -604,32 +604,63 @@ PeerConnectionMedia::BeginIceRestart_s(R
 
   mIceCtxHdlr->BeginIceRestart(new_ctx);
   if (mIceCtxHdlr->IsRestarting()) {
     ConnectSignals(mIceCtxHdlr->ctx().get(), originalCtx.get());
   }
 }
 
 void
+PeerConnectionMedia::CommitIceRestart()
+{
+  ASSERT_ON_THREAD(mMainThread);
+
+  RUN_ON_THREAD(GetSTSThread(),
+                WrapRunnable(
+                    RefPtr<PeerConnectionMedia>(this),
+                    &PeerConnectionMedia::CommitIceRestart_s),
+                NS_DISPATCH_NORMAL);
+}
+
+void
+PeerConnectionMedia::CommitIceRestart_s()
+{
+  ASSERT_ON_THREAD(mSTSThread);
+
+  mIceCtxHdlr->CommitIceRestart();
+}
+
+void
 PeerConnectionMedia::FinalizeIceRestart()
 {
   ASSERT_ON_THREAD(mMainThread);
 
   RUN_ON_THREAD(GetSTSThread(),
                 WrapRunnable(
                     RefPtr<PeerConnectionMedia>(this),
                     &PeerConnectionMedia::FinalizeIceRestart_s),
                 NS_DISPATCH_NORMAL);
 }
 
 void
 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();
 }
 
 void
 PeerConnectionMedia::RollbackIceRestart()
 {
   ASSERT_ON_THREAD(mMainThread);
 
@@ -639,23 +670,35 @@ PeerConnectionMedia::RollbackIceRestart(
                     &PeerConnectionMedia::RollbackIceRestart_s),
                 NS_DISPATCH_NORMAL);
 }
 
 void
 PeerConnectionMedia::RollbackIceRestart_s()
 {
   ASSERT_ON_THREAD(mSTSThread);
-  if (!mIceCtxHdlr->IsRestarting()) {
+  if (mIceCtxHdlr->GetRestartState() !=
+          NrIceCtxHandler::ICE_RESTART_IN_PROGRESS) {
     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());
 }
 
 bool
 PeerConnectionMedia::GetPrefDefaultAddressOnly() const
 {
   ASSERT_ON_THREAD(mMainThread); // will crash on STS thread
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ -253,16 +253,18 @@ class PeerConnectionMedia : public sigsl
   void ActivateOrRemoveTransports(const JsepSession& aSession);
 
   // Start ICE checks.
   void StartIceChecks(const JsepSession& session);
 
   // 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
   void RollbackIceRestart();
 
   // Process a trickle ICE candidate.
   void AddIceCandidate(const std::string& candidate, const std::string& mid,
                        uint32_t aMLine);
@@ -446,16 +448,17 @@ class PeerConnectionMedia : public sigsl
   void FlushIceCtxOperationQueueIfReady();
   void PerformOrEnqueueIceCtxOperation(nsIRunnable* runnable);
   void EnsureIceGathering_s();
   void StartIceChecks_s(bool aIsControlling,
                         bool aIsIceLite,
                         const std::vector<std::string>& aIceOptionsList);
 
   void BeginIceRestart_s(RefPtr<NrIceCtx> new_ctx);
+  void CommitIceRestart_s();
   void FinalizeIceRestart_s();
   void RollbackIceRestart_s();
   bool GetPrefDefaultAddressOnly() const;
 
   void ConnectSignals(NrIceCtx *aCtx, NrIceCtx *aOldCtx=nullptr);
 
   // Process a trickle ICE candidate.
   void AddIceCandidate_s(const std::string& aCandidate, const std::string& aMid,