Track pending ICE restarts independently for different media sections.

webrtc/api/webrtcsession.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 439 matching lines @@
         return cricket::CF_RELAY;
     case PeerConnectionInterface::kNoHost:
         return (cricket::CF_ALL & ~cricket::CF_HOST);
     case PeerConnectionInterface::kAll:
         return cricket::CF_ALL;
     default: ASSERT(false);
   }
   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;
-    }
-  }
-
-  // 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;
-      }
-    }
+// 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;
   }
-
- private:
-  bool ice_restart_;
-};
+  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;
+  }
+  // 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,
                              rtc::Thread* worker_thread,
                              cricket::PortAllocator* port_allocator)
     : signaling_thread_(signaling_thread),
       worker_thread_(worker_thread),
       port_allocator_(port_allocator),
       // RFC 3264: The numeric value of the session id and version in the
       // o line MUST be representable with a "64 bit signed integer".
       // Due to this constraint session id |sid_| is max limited to LLONG_MAX.
       sid_(rtc::ToString(rtc::CreateRandomId64() & LLONG_MAX)),
       transport_controller_(new cricket::TransportController(signaling_thread,
                                                              worker_thread,
                                                              port_allocator)),
       media_controller_(media_controller),
       channel_manager_(media_controller_->channel_manager()),
       ice_observer_(NULL),
       ice_connection_state_(PeerConnectionInterface::kIceConnectionNew),
       ice_connection_receiving_(true),
       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(
       this, &WebRtcSession::OnTransportControllerConnectionState);
   transport_controller_->SignalReceiving.connect(
       this, &WebRtcSession::OnTransportControllerReceiving);
   transport_controller_->SignalGatheringState.connect(
       this, &WebRtcSession::OnTransportControllerGatheringState);
   transport_controller_->SignalCandidatesGathered.connect(
       this, &WebRtcSession::OnTransportControllerCandidatesGathered);
@@ skipping 144 matching lines @@
 }
 
 void WebRtcSession::Close() {
   SetState(STATE_CLOSED);
   RemoveUnusedChannels(nullptr);
   ASSERT(!voice_channel_);
   ASSERT(!video_channel_);
   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);
 }
 
 cricket::SecurePolicy WebRtcSession::SdesPolicy() const {
   return webrtc_session_desc_factory_->SdesPolicy();
 }
 
 bool WebRtcSession::GetSslRole(const std::string& transport_name,
                                rtc::SSLRole* role) {
@@ skipping 67 matching lines @@
 
   if (!UpdateSessionState(action, cricket::CS_LOCAL, err_desc)) {
     return false;
   }
 
   if (remote_desc_) {
     // Now that we have a local description, we can push down remote candidates.
     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);
   }
   return true;
 }
 
 bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc,
                                          std::string* err_desc) {
   ASSERT(signaling_thread()->IsCurrent());
 
@@ skipping 23 matching lines @@
   // NOTE: Candidates allocation will be initiated only when SetLocalDescription
   // is called.
   if (!UpdateSessionState(action, cricket::CS_REMOTE, err_desc)) {
     return false;
   }
 
   if (local_desc_ && !UseCandidatesInSessionDescription(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) {
     return BadRemoteSdp(desc->type(), GetSessionErrorMsg(), err_desc);
   }
 
   // Set the the ICE connection state to connecting since the connection may
   // become writable with peer reflexive candidates before any remote candidate
   // is signaled.
   // TODO(pthatcher): This is a short-term solution for crbug/446908. A real fix
@@ skipping 238 matching lines @@
   return transport_controller_->GetLocalCertificate(transport_name,
                                                     certificate);
 }
 
 bool WebRtcSession::GetRemoteSSLCertificate(const std::string& transport_name,
                                             rtc::SSLCertificate** cert) {
   ASSERT(signaling_thread()->IsCurrent());
   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) {
     LOG(LS_WARNING) << "Tried to BUNDLE with no contents.";
     return false;
   }
   const std::string& transport_name = *first_content_name;
   cricket::BaseChannel* first_channel = GetChannel(transport_name);
 
   auto maybe_set_transport = [this, bundle, transport_name,
@@ skipping 304 matching lines @@
 }
 
 bool WebRtcSession::ReadyToSendData() const {
   return data_channel_ && data_channel_->ready_to_send_data();
 }
 
 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(
     const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
   transport_controller_->SetLocalCertificate(certificate);
 }
 
 bool WebRtcSession::waiting_for_certificate_for_testing() const {
   return webrtc_session_desc_factory_->waiting_for_certificate_for_testing();
 }
@@ skipping 692 matching lines @@
   }
 }
 
 void WebRtcSession::OnSentPacket_w(cricket::TransportChannel* channel,
                                    const rtc::SentPacket& sent_packet) {
   RTC_DCHECK(worker_thread()->IsCurrent());
   media_controller_->call_w()->OnSentPacket(sent_packet);
 }
 
 }  // namespace webrtc