Fixed a bug in determining ICE roles.

When the initial offer side uses the ICE lite implementation, and
initiates a peer connection with an endpoint with the full
implementation, the offer side assumes the controlled ICE role per
RFC5245 and the remote endpoint MUST take the controlling role.

This logic was partially implemented in SetRemoteTransportDescription in
reflection where the endpoint switches its role to the controlling after
receiving the offer. The bug was caused by the following
SetLocalDescription at the remote endpoint after creating the answer,
which overrides the role to the controlled since it has no initial offer
and the role is not reflected in SetLocalTransportDescription. This
results in no nomination of candidate pairs and timeout of establishing
the peer connection.

The fix adds reflection on one's ICE role in SetLocalTransportDescription.
This fix also takes into account the case when both sides use the lite
implementation of ICE and the initial offer side MUST take the controlling
role per RFC5245 in this case, which is the default behavior in the
current implementation.

Bug: webrtc:8531
Change-Id: I65edd296c155bff51fcdb28709975e6837f302d5
Reviewed-on: https://webrtc-review.googlesource.com/26780
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@google.com>
Cr-Commit-Position: refs/heads/master@{#21053}

pc/peerconnection_ice_unittest.cc

@@ -12,6 +12,7 @@
 #include "p2p/base/teststunserver.h"
 #include "p2p/client/basicportallocator.h"
 #include "pc/mediasession.h"
+#include "pc/peerconnection.h"
 #include "pc/peerconnectionwrapper.h"
 #include "pc/sdputils.h"
 #ifdef WEBRTC_ANDROID
@@ -19,6 +20,7 @@
 #endif
 #include "api/audio_codecs/builtin_audio_decoder_factory.h"
 #include "api/audio_codecs/builtin_audio_encoder_factory.h"
+#include "api/peerconnectionproxy.h"
 #include "pc/test/fakeaudiocapturemodule.h"
 #include "rtc_base/fakenetwork.h"
 #include "rtc_base/gunit.h"
@@ -152,6 +154,16 @@ class PeerConnectionIceUnitTest : public ::testing::Test {
     }
   }
 
+  // Set ICE mode on the given session description.
+  void SetIceMode(SessionDescriptionInterface* sdesc,
+                  const cricket::IceMode ice_mode) {
+    auto* desc = sdesc->description();
+    for (const auto& content : desc->contents()) {
+      auto* transport_info = desc->GetTransportInfoByName(content.name);
+      transport_info->description.ice_mode = ice_mode;
+    }
+  }
+
   cricket::TransportDescription* GetFirstTransportDescription(
       SessionDescriptionInterface* sdesc) {
     auto* desc = sdesc->description();
@@ -172,6 +184,20 @@ class PeerConnectionIceUnitTest : public ::testing::Test {
     return &transport_info->description;
   }
 
+  // TODO(qingsi): Rewrite this method in terms of the standard IceTransport
+  // after it is implemented.
+  cricket::IceRole GetIceRole(const WrapperPtr& pc_wrapper_ptr) {
+    auto* pc_proxy = reinterpret_cast<
+        PeerConnectionProxyWithInternal<PeerConnectionInterface>*>(
+        pc_wrapper_ptr->pc());
+    PeerConnection* pc =
+        reinterpret_cast<PeerConnection*>(pc_proxy->internal());
+    return pc->voice_channel()
+        ->rtp_dtls_transport()
+        ->ice_transport()
+        ->GetIceRole();
+  }
+
   bool AddCandidateToFirstTransport(cricket::Candidate* candidate,
                                     SessionDescriptionInterface* sdesc) {
     auto* desc = sdesc->description();
@@ -788,4 +814,53 @@ TEST_F(PeerConnectionIceUnitTest,
             local_transports[1].description.ice_pwd);
 }
 
+// Test that when the initial offerer (caller) uses the lite implementation of
+// ICE and the callee uses the full implementation, the caller takes the
+// CONTROLLED role and the callee takes the CONTROLLING role. This is specified
+// in RFC5245 Section 5.1.1.
+TEST_F(PeerConnectionIceUnitTest,
+       OfferFromLiteIceControlledAndAnswerFromFullIceControlling) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  auto offer = caller->CreateOffer();
+  SetIceMode(offer.get(), cricket::IceMode::ICEMODE_LITE);
+  ASSERT_TRUE(
+      caller->SetLocalDescription(CloneSessionDescription(offer.get())));
+  ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer)));
+
+  auto answer = callee->CreateAnswer();
+  SetIceMode(answer.get(), cricket::IceMode::ICEMODE_FULL);
+  ASSERT_TRUE(
+      callee->SetLocalDescription(CloneSessionDescription(answer.get())));
+  ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer)));
+
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, GetIceRole(caller));
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, GetIceRole(callee));
+}
+
+// Test that when the caller and the callee both use the lite implementation of
+// ICE, the initial offerer (caller) takes the CONTROLLING role and the callee
+// takes the CONTROLLED role. This is specified in RFC5245 Section 5.1.1.
+TEST_F(PeerConnectionIceUnitTest,
+       OfferFromLiteIceControllingAndAnswerFromLiteIceControlled) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  auto offer = caller->CreateOffer();
+  SetIceMode(offer.get(), cricket::IceMode::ICEMODE_LITE);
+  ASSERT_TRUE(
+      caller->SetLocalDescription(CloneSessionDescription(offer.get())));
+  ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer)));
+
+  auto answer = callee->CreateAnswer();
+  SetIceMode(answer.get(), cricket::IceMode::ICEMODE_LITE);
+  ASSERT_TRUE(
+      callee->SetLocalDescription(CloneSessionDescription(answer.get())));
+  ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer)));
+
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, GetIceRole(caller));
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, GetIceRole(callee));
+}
+
 }  // namespace webrtc

pc/transportcontroller.cc

@@ -597,6 +597,19 @@ bool TransportController::SetLocalTransportDescription_n(
     return true;
   }
 
+  // The initial offer side may use ICE Lite, in which case, per RFC5245 Section
+  // 5.1.1, the answer side should take the controlling role if it is in the
+  // full ICE mode.
+  //
+  // When both sides use ICE Lite, the initial offer side must take the
+  // controlling role, and this is the default logic implemented in
+  // SetLocalDescription in PeerConnection.
+  if (transport->remote_description() &&
+      transport->remote_description()->ice_mode == ICEMODE_LITE &&
+      ice_role_ == ICEROLE_CONTROLLED && tdesc.ice_mode == ICEMODE_FULL) {
+    SetIceRole_n(ICEROLE_CONTROLLING);
+  }
+
   // Older versions of Chrome expect the ICE role to be re-determined when an
   // ICE restart occurs, and also don't perform conflict resolution correctly,
   // so for now we can't safely stop doing this, unless the application opts in
@@ -645,6 +658,15 @@ bool TransportController::SetRemoteTransportDescription_n(
     return true;
   }
 
+  // If we use ICE Lite and the remote endpoint uses the full implementation of
+  // ICE, the local endpoint must take the controlled role, and the other side
+  // must be the controlling role.
+  if (transport->local_description() &&
+      transport->local_description()->ice_mode == ICEMODE_LITE &&
+      ice_role_ == ICEROLE_CONTROLLING && tdesc.ice_mode == ICEMODE_FULL) {
+    SetIceRole_n(ICEROLE_CONTROLLED);
+  }
+
   RTC_LOG(LS_INFO) << "Set remote transport description on " << transport_name;
   return transport->SetRemoteTransportDescription(tdesc, action, err);
 }