Track pending ICE restarts independently for different media sections.

webrtc/api/webrtcsession.cc

Index: webrtc/api/webrtcsession.cc
diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc
index 92a5f0aec45979b80cea44b430cbaa9af80a726f..69d123a6634f32d8d542de63cf563e040793899c 100644
--- a/webrtc/api/webrtcsession.cc
+++ b/webrtc/api/webrtcsession.cc
@@ -457,69 +457,37 @@ uint32_t ConvertIceTransportTypeToCandidateFilter(
   return cricket::CF_NONE;
 }
 
-// Help class used to remember if a a remote peer has requested ice restart by
-// by sending a description with new ice ufrag and password.
-class IceRestartAnswerLatch {
- public:
-  IceRestartAnswerLatch() : ice_restart_(false) { }
-
-  // Returns true if CheckForRemoteIceRestart has been called with a new session
-  // description where ice password and ufrag has changed since last time
-  // Reset() was called.
-  bool Get() const {
-    return ice_restart_;
-  }
-
-  void Reset() {
-    if (ice_restart_) {
-      ice_restart_ = false;
-    }
+// Returns true if |new_desc| requests an ICE restart (i.e., new ufrag/pwd).
+bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc,
+                              const SessionDescriptionInterface* new_desc,
+                              const std::string& content_name) {
+  if (!old_desc) {
+    return false;
   }
-
-  // This method has two purposes: 1. Return whether |new_desc| requests
-  // an ICE restart (i.e., new ufrag/pwd). 2. If it requests an ICE restart
-  // and it is an OFFER, remember this in |ice_restart_| so that the next
-  // Local Answer will be created with new ufrag and pwd.
-  bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc,
-                                const SessionDescriptionInterface* new_desc) {
-    if (!old_desc) {
-      return false;
-    }
-    const SessionDescription* new_sd = new_desc->description();
-    const SessionDescription* old_sd = old_desc->description();
-    const ContentInfos& contents = new_sd->contents();
-    for (size_t index = 0; index < contents.size(); ++index) {
-      const ContentInfo* cinfo = &contents[index];
-      if (cinfo->rejected) {
-        continue;
-      }
-      // If the content isn't rejected, check if ufrag and password has
-      // changed.
-      const cricket::TransportDescription* new_transport_desc =
-          new_sd->GetTransportDescriptionByName(cinfo->name);
-      const cricket::TransportDescription* old_transport_desc =
-          old_sd->GetTransportDescriptionByName(cinfo->name);
-      if (!new_transport_desc || !old_transport_desc) {
-        // No transport description exist. This is not an ice restart.
-        continue;
-      }
-      if (cricket::IceCredentialsChanged(old_transport_desc->ice_ufrag,
-                                         old_transport_desc->ice_pwd,
-                                         new_transport_desc->ice_ufrag,
-                                         new_transport_desc->ice_pwd)) {
-        LOG(LS_INFO) << "Remote peer request ice restart.";
-        if (new_desc->type() == SessionDescriptionInterface::kOffer) {
-          ice_restart_ = true;
-        }
-        return true;
-      }
-    }
+  const SessionDescription* new_sd = new_desc->description();

[pthatcher1, 2016/02/17 06:46:58]

Using existing style, this would be "new_desc" and "old_desc".

[Taylor Brandstetter, 2016/02/17 21:43:50]

But one is a SessionDescription and one is a SessionDescriptionInterface. (note
that this code is just being moved around)

+  const SessionDescription* old_sd = old_desc->description();
+  const ContentInfo* cinfo = new_sd->GetContentByName(content_name);
+  if (!cinfo || cinfo->rejected) {
     return false;
   }
-
- private:
-  bool ice_restart_;
-};
+  // If the content isn't rejected, check if ufrag and password has changed.
+  const cricket::TransportDescription* new_transport_desc =
+      new_sd->GetTransportDescriptionByName(content_name);
+  const cricket::TransportDescription* old_transport_desc =
+      old_sd->GetTransportDescriptionByName(content_name);
+  if (!new_transport_desc || !old_transport_desc) {
+    // No transport description exists. This is not an ICE restart.
+    return false;
+  }
+  if (cricket::IceCredentialsChanged(
+          old_transport_desc->ice_ufrag, old_transport_desc->ice_pwd,
+          new_transport_desc->ice_ufrag, new_transport_desc->ice_pwd)) {
+    LOG(LS_INFO) << "Remote peer requests ICE restart for " << content_name
+                 << ".";
+    return true;
+  }
+  return false;
+}
 
 WebRtcSession::WebRtcSession(webrtc::MediaControllerInterface* media_controller,
                              rtc::Thread* signaling_thread,
@@ -543,7 +511,6 @@ WebRtcSession::WebRtcSession(webrtc::MediaControllerInterface* media_controller,
       older_version_remote_peer_(false),
       dtls_enabled_(false),
       data_channel_type_(cricket::DCT_NONE),
-      ice_restart_latch_(new IceRestartAnswerLatch),
       metrics_observer_(NULL) {
   transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLED);
   transport_controller_->SignalConnectionState.connect(
@@ -708,6 +675,20 @@ void WebRtcSession::Close() {
   ASSERT(!data_channel_);
 }
 
+cricket::BaseChannel* WebRtcSession::GetChannel(
+    const std::string& content_name) {
+  if (voice_channel() && voice_channel()->content_name() == content_name) {
+    return voice_channel();
+  }
+  if (video_channel() && video_channel()->content_name() == content_name) {
+    return video_channel();
+  }
+  if (data_channel() && data_channel()->content_name() == content_name) {
+    return data_channel();
+  }
+  return nullptr;
+}

[honghaiz3, 2016/02/12 18:21:04]

Did you change this part?
Either way, it would be better to leave it at the same location as before to
minimize the diff. 

[Taylor Brandstetter, 2016/02/12 20:43:02]

No, I didn't change it. But I moved it in the header file to the "public"
section, so I moved it in the cc file too. I find it useful to keep things in
order.

+
 void WebRtcSession::SetSdesPolicy(cricket::SecurePolicy secure_policy) {
   webrtc_session_desc_factory_->SetSdesPolicy(secure_policy);
 }
@@ -795,6 +776,8 @@ bool WebRtcSession::SetLocalDescription(SessionDescriptionInterface* desc,
     UseCandidatesInSessionDescription(remote_desc_.get());
   }
 
+  pending_ice_restarts_.clear();

[pthatcher1, 2016/02/17 06:46:58]

What if the call to SetLocalDescription didn't actually change the ufrag/pwd for
a content name that was triggered by the remote side (because the JS called
createOffer, then setRemote, then  setLocal, or some crazy thing).

[Taylor Brandstetter, 2016/02/17 21:43:50]

Well, for one, the issue you describe occurred before this CL too, so I don't
think we need to fix it here.

And second, changing the ICE ufrag/password is illegal anyway according to the
spec. So right now it should be considered a "use at your own risk" type of
munging. And once we actually do make it illegal, we won't have to worry about
this at all.

[pthatcher1, 2016/02/17 21:53:38]

But is it legal to do this?

var remoteOffer1 = ...;  
var localAnswer1 = pc.createAnswer(remoteOffer1);
var remoteOffer2 = ...;  // Changes ufrag/pwd
pc.setRemoteDesription(remoteOffer2);
pc.setLocalDescription(remoteAnswer1);
var localOffer1 = pc.createOffer();


If so, does localOffer1 have a changed ufrag/pwd or not?

[Taylor Brandstetter, 2016/02/18 00:33:21]

This is "legal", but very dangerous. Setting an old SDP blob is the same to us
as munging a new SDP blob to look like an old one. So, once we make munging the
ufrag/password illegal, setLocalDescription will throw an error there. It would
also throw an error if the second remote description added an m-line.

[pthatcher1, 2016/02/19 02:33:43]

It would still make me feel a lot better if we did something like:

if (ice_ufrag_or_pwd_changed) {
  pending_ice_restarts_.clear();
}


But I won't let it block submitting this CL.

[Taylor Brandstetter, 2016/02/19 21:59:26]

Ok, let's say the spec DOES allow munging the ufrag/pwd. We still would want to
reset the "pending restart" flag here. The flag is only valid for a single
offer/answer exchange.  The next generated answer should only have a new
ufrag/pwd if the next offer does. Right?

The only question is: If you set a pranswer with the old ufrag/pwd, should the
next generated answer have old or new credentials? Since this is unspecified
behavior (since munging the ufrag/pwd is illegal), I think we're free to decide
what happens for the time being, and "old credentials" makes enough sense to me.

[pthatcher1, 2016/02/20 05:57:25]

I was thinking of the next createOffer from the local side should generate a new
ufrag/pwd.  But now that I think about it more, I realize that would just
trigger a new ICE restart from the remote side again, which probably is not what
we want.  

Perhaps what we should do is say "if you manage to set unchanged local
ufrag/pwd, then you get a half-restart".  The more I think about it, maybe
that's a feature and not a bug.

So, I agree with you now: just clear the flag.

+
   if (error() != ERROR_NONE) {
     return BadLocalSdp(desc->type(), GetSessionErrorMsg(), err_desc);
   }
@@ -838,20 +821,30 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc,
     return BadRemoteSdp(desc->type(), kInvalidCandidates, err_desc);
   }
 
-  // Check if this new SessionDescription contains new ice ufrag and password
-  // that indicates the remote peer requests ice restart.
-  bool ice_restart =
-      ice_restart_latch_->CheckForRemoteIceRestart(old_remote_desc.get(), desc);
-  // We retain all received candidates only if ICE is not restarted.
-  // When ICE is restarted, all previous candidates belong to an old generation
-  // and should not be kept.
-  // TODO(deadbeef): This goes against the W3C spec which says the remote
-  // description should only contain candidates from the last set remote
-  // description plus any candidates added since then. We should remove this
-  // once we're sure it won't break anything.
-  if (!ice_restart) {
-    WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
-        old_remote_desc.get(), desc);
+  if (old_remote_desc) {
+    for (const cricket::ContentInfo& content :
+         old_remote_desc->description()->contents()) {
+      // Check if this new SessionDescription contains new ICE ufrag and
+      // password that indicates the remote peer requests an ICE restart.
+      // TODO(deadbeef): When we start storing both the current and pending
+      // remote description, this should reset pending_ice_restarts and compare
+      // against the current description.
+      if (CheckForRemoteIceRestart(old_remote_desc.get(), desc, content.name)) {
+        if (action == kOffer) {
+          pending_ice_restarts_.insert(content.name);
+        }
+      } else {
+        // We retain all received candidates only if ICE is not restarted.
+        // When ICE is restarted, all previous candidates belong to an old
+        // generation and should not be kept.
+        // TODO(deadbeef): This goes against the W3C spec which says the remote
+        // description should only contain candidates from the last set remote
+        // description plus any candidates added since then. We should remove
+        // this once we're sure it won't break anything.
+        WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
+            old_remote_desc.get(), content.name, desc);
+      }
+    }
   }
 
   if (error() != ERROR_NONE) {
@@ -1110,20 +1103,6 @@ bool WebRtcSession::GetRemoteSSLCertificate(const std::string& transport_name,
   return transport_controller_->GetRemoteSSLCertificate(transport_name, cert);
 }
 
-cricket::BaseChannel* WebRtcSession::GetChannel(
-    const std::string& content_name) {
-  if (voice_channel() && voice_channel()->content_name() == content_name) {
-    return voice_channel();
-  }
-  if (video_channel() && video_channel()->content_name() == content_name) {
-    return video_channel();
-  }
-  if (data_channel() && data_channel()->content_name() == content_name) {
-    return data_channel();
-  }
-  return nullptr;
-}
-
 bool WebRtcSession::EnableBundle(const cricket::ContentGroup& bundle) {
   const std::string* first_content_name = bundle.FirstContentName();
   if (!first_content_name) {
@@ -1448,12 +1427,9 @@ cricket::DataChannelType WebRtcSession::data_channel_type() const {
   return data_channel_type_;
 }
 
-bool WebRtcSession::IceRestartPending() const {
-  return ice_restart_latch_->Get();
-}
-
-void WebRtcSession::ResetIceRestartLatch() {
-  ice_restart_latch_->Reset();
+bool WebRtcSession::IceRestartPending(const std::string& content_name) const {
+  return pending_ice_restarts_.find(content_name) !=
+         pending_ice_restarts_.end();
 }
 
 void WebRtcSession::OnCertificateReady(