Don't re-determine ICE role on an ICE restart.

webrtc/p2p/base/transport_unittest.cc

 /*
  *  Copyright 2011 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 79 matching lines @@
 
 // Verifies that IceCredentialsChanged returns true when either ufrag or pwd
 // changed, and false in other cases.
 TEST_F(TransportTest, TestIceCredentialsChanged) {
   EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u2", "p2"));
   EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u2", "p1"));
   EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p2"));
   EXPECT_FALSE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p1"));
 }
 
-// This test verifies that the callee's ICE role changes from controlled to
-// controlling when the callee triggers an ICE restart.
+// This test verifies that the callee's ICE role remains the same when the
+// callee triggers an ICE restart.
+//
+// RFC5245 currently says that the role *should* change on an ICE restart,
+// but this rule was intended for an ICE restart that occurs when an endpoint
+// is changing to ICE lite (which we already handle). See discussion here:
+// https://mailarchive.ietf.org/arch/msg/ice/C0_QRCTNcwtvUF12y28jQicPR10
 TEST_F(TransportTest, TestIceControlledToControllingOnIceRestart) {
   EXPECT_TRUE(SetupChannel());
   transport_->SetIceRole(cricket::ICEROLE_CONTROLLED);
 
   cricket::TransportDescription desc(kIceUfrag1, kIcePwd1);
   ASSERT_TRUE(transport_->SetRemoteTransportDescription(desc,
                                                         cricket::CA_OFFER,
                                                         NULL));
   ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
                                                        cricket::CA_ANSWER,
                                                        NULL));
   EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
 
   cricket::TransportDescription new_local_desc(kIceUfrag2, kIcePwd2);
   ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
                                                        cricket::CA_OFFER,
                                                        NULL));
-  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
-  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel_->GetIceRole());
 }
 
-// This test verifies that the caller's ICE role changes from controlling to
-// controlled when the callee triggers an ICE restart.
+// This test verifies that the caller's ICE role remains the same when the
+// callee triggers an ICE restart.
+//
+// RFC5245 currently says that the role *should* change on an ICE restart,
+// but this rule was intended for an ICE restart that occurs when an endpoint
+// is changing to ICE lite (which we already handle). See discussion here:
+// https://mailarchive.ietf.org/arch/msg/ice/C0_QRCTNcwtvUF12y28jQicPR10
 TEST_F(TransportTest, TestIceControllingToControlledOnIceRestart) {
   EXPECT_TRUE(SetupChannel());
   transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
 
   cricket::TransportDescription desc(kIceUfrag1, kIcePwd1);
   ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
                                                        cricket::CA_OFFER,
                                                        NULL));
   ASSERT_TRUE(transport_->SetRemoteTransportDescription(desc,
                                                         cricket::CA_ANSWER,
                                                         NULL));
   EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
 
   cricket::TransportDescription new_local_desc(kIceUfrag2, kIcePwd2);
   ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
                                                        cricket::CA_ANSWER,
                                                        NULL));
-  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
-  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel_->GetIceRole());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
 }
 
 // This test verifies that the caller's ICE role is still controlling after the
 // callee triggers ICE restart if the callee's ICE mode is LITE.
 TEST_F(TransportTest, TestIceControllingOnIceRestartIfRemoteIsIceLite) {
   EXPECT_TRUE(SetupChannel());
   transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
 
   cricket::TransportDescription desc(kIceUfrag1, kIcePwd1);
   ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
@@ skipping 251 matching lines @@
     remote_desc.connection_role = param.remote_role;
 
     ASSERT_TRUE(transport_->SetRemoteTransportDescription(
         remote_desc, param.remote_action, nullptr));
     ASSERT_TRUE(transport_->SetLocalTransportDescription(
         local_desc, param.local_action, nullptr));
     EXPECT_FALSE(
         transport_->NegotiateRole(param.local_action, &ssl_role, &error_desc));
   }
 }