From 91042f834d26986529bf5ac0e2a034a8a8b15524 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 15 Jul 2016 17:48:13 -0700 Subject: [PATCH] Restore the behavior where an ICE restart redetermines the ICE role. We thought we could safely remove this, but older versions of Chrome don't do role conflict resolution properly, so it's actually not safe to yet. BUG=628676 Review-Url: https://codereview.webrtc.org/2152963003 Cr-Commit-Position: refs/heads/master@{#13492} --- webrtc/p2p/base/transport.h | 18 +- webrtc/p2p/base/transportcontroller.cc | 15 ++ .../p2p/base/transportcontroller_unittest.cc | 190 ++++++++++-------- 3 files changed, 133 insertions(+), 90 deletions(-) diff --git a/webrtc/p2p/base/transport.h b/webrtc/p2p/base/transport.h index e4753d217d..a7ca72ff8e 100644 --- a/webrtc/p2p/base/transport.h +++ b/webrtc/p2p/base/transport.h @@ -314,24 +314,26 @@ class Transport : public sigslot::has_slots<> { return false; } - protected: - // These are called by Create/DestroyChannel above in order to create or - // destroy the appropriate type of channel. - virtual TransportChannelImpl* CreateTransportChannel(int component) = 0; - virtual void DestroyTransportChannel(TransportChannelImpl* channel) = 0; - // The current local transport description, for use by derived classes - // when performing transport description negotiation. + // when performing transport description negotiation, and possibly used + // by the transport controller. const TransportDescription* local_description() const { return local_description_.get(); } // The current remote transport description, for use by derived classes - // when performing transport description negotiation. + // when performing transport description negotiation, and possibly used + // by the transport controller. const TransportDescription* remote_description() const { return remote_description_.get(); } + protected: + // These are called by Create/DestroyChannel above in order to create or + // destroy the appropriate type of channel. + virtual TransportChannelImpl* CreateTransportChannel(int component) = 0; + virtual void DestroyTransportChannel(TransportChannelImpl* channel) = 0; + // Pushes down the transport parameters from the local description, such // as the ICE ufrag and pwd. // Derived classes can override, but must call the base as well. diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 2860f87b6d..b5021ccecc 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -446,6 +446,21 @@ bool TransportController::SetLocalTransportDescription_n( return true; } + // 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. + // See: https://bugs.chromium.org/p/chromium/issues/detail?id=628676 + // TODO(deadbeef): Remove this when these old versions of Chrome reach a low + // enough population. + if (transport->local_description() && + IceCredentialsChanged(transport->local_description()->ice_ufrag, + transport->local_description()->ice_pwd, + tdesc.ice_ufrag, tdesc.ice_pwd)) { + IceRole new_ice_role = + (action == CA_OFFER) ? ICEROLE_CONTROLLING : ICEROLE_CONTROLLED; + SetIceRole(new_ice_role); + } + return transport->SetLocalTransportDescription(tdesc, action, err); } diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index f068bb52db..2b11286e99 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -28,17 +28,10 @@ static const char kIceUfrag1[] = "TESTICEUFRAG0001"; static const char kIcePwd1[] = "TESTICEPWD00000000000001"; static const char kIceUfrag2[] = "TESTICEUFRAG0002"; static const char kIcePwd2[] = "TESTICEPWD00000000000002"; +static const char kIceUfrag3[] = "TESTICEUFRAG0003"; +static const char kIcePwd3[] = "TESTICEPWD00000000000003"; -using cricket::Candidate; -using cricket::Candidates; -using cricket::FakeTransportChannel; -using cricket::FakeTransportController; -using cricket::IceConnectionState; -using cricket::IceGatheringState; -using cricket::TransportChannel; -using cricket::TransportController; -using cricket::TransportDescription; -using cricket::TransportStats; +namespace cricket { // Only subclassing from FakeTransportController because currently that's the // only way to have a TransportController with fake TransportChannels. @@ -93,7 +86,7 @@ class TransportControllerTest : public testing::Test, Candidate c; c.set_address(rtc::SocketAddress("192.168.1.1", 8000)); c.set_component(1); - c.set_protocol(cricket::UDP_PROTOCOL_NAME); + c.set_protocol(UDP_PROTOCOL_NAME); c.set_priority(1); return c; } @@ -108,20 +101,20 @@ class TransportControllerTest : public testing::Test, } void CreateChannelsAndCompleteConnection_w() { - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); FakeTransportChannel* channel2 = CreateChannel("video", 1); ASSERT_NE(nullptr, channel2); TransportDescription local_desc(std::vector(), kIceUfrag1, - kIcePwd1, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); std::string err; - transport_controller_->SetLocalTransportDescription( - "audio", local_desc, cricket::CA_OFFER, &err); - transport_controller_->SetLocalTransportDescription( - "video", local_desc, cricket::CA_OFFER, &err); + transport_controller_->SetLocalTransportDescription("audio", local_desc, + CA_OFFER, &err); + transport_controller_->SetLocalTransportDescription("video", local_desc, + CA_OFFER, &err); transport_controller_->MaybeStartGathering(); channel1->SignalCandidateGathered(channel1, CreateCandidate(1)); channel2->SignalCandidateGathered(channel2, CreateCandidate(1)); @@ -137,10 +130,10 @@ class TransportControllerTest : public testing::Test, channel2->SetConnectionCount(1); } - cricket::IceConfig CreateIceConfig( + IceConfig CreateIceConfig( int receiving_timeout, - cricket::ContinualGatheringPolicy continual_gathering_policy) { - cricket::IceConfig config; + ContinualGatheringPolicy continual_gathering_policy) { + IceConfig config; config.receiving_timeout = receiving_timeout; config.continual_gathering_policy = continual_gathering_policy; return config; @@ -185,9 +178,9 @@ class TransportControllerTest : public testing::Test, std::unique_ptr transport_controller_; // Information received from signals from transport controller. - IceConnectionState connection_state_ = cricket::kIceConnectionConnecting; + IceConnectionState connection_state_ = kIceConnectionConnecting; bool receiving_ = false; - IceGatheringState gathering_state_ = cricket::kIceGatheringNew; + IceGatheringState gathering_state_ = kIceGatheringNew; // transport_name => candidates std::map candidates_; // Counts of each signal emitted. @@ -206,12 +199,12 @@ TEST_F(TransportControllerTest, TestSetIceConfig) { ASSERT_NE(nullptr, channel1); transport_controller_->SetIceConfig( - CreateIceConfig(1000, cricket::GATHER_CONTINUALLY)); + CreateIceConfig(1000, GATHER_CONTINUALLY)); EXPECT_EQ(1000, channel1->receiving_timeout()); EXPECT_TRUE(channel1->gather_continually()); transport_controller_->SetIceConfig( - CreateIceConfig(1000, cricket::GATHER_CONTINUALLY_AND_RECOVER)); + CreateIceConfig(1000, GATHER_CONTINUALLY_AND_RECOVER)); // Test that value stored in controller is applied to new channels. FakeTransportChannel* channel2 = CreateChannel("video", 1); ASSERT_NE(nullptr, channel2); @@ -236,15 +229,15 @@ TEST_F(TransportControllerTest, TestSetIceRole) { FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); - EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel1->GetIceRole()); - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLED); - EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel1->GetIceRole()); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); + EXPECT_EQ(ICEROLE_CONTROLLING, channel1->GetIceRole()); + transport_controller_->SetIceRole(ICEROLE_CONTROLLED); + EXPECT_EQ(ICEROLE_CONTROLLED, channel1->GetIceRole()); // Test that value stored in controller is applied to new channels. FakeTransportChannel* channel2 = CreateChannel("video", 1); ASSERT_NE(nullptr, channel2); - EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel2->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLED, channel2->GetIceRole()); } // Test that when one channel encounters a role conflict, the ICE role is @@ -255,19 +248,19 @@ TEST_F(TransportControllerTest, TestIceRoleConflict) { FakeTransportChannel* channel2 = CreateChannel("video", 1); ASSERT_NE(nullptr, channel2); - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); - EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel1->GetIceRole()); - EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel2->GetIceRole()); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); + EXPECT_EQ(ICEROLE_CONTROLLING, channel1->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLING, channel2->GetIceRole()); channel1->SignalRoleConflict(channel1); - EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel1->GetIceRole()); - EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel2->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLED, channel1->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLED, channel2->GetIceRole()); // Should be able to handle a second role conflict. The remote endpoint can // change its role/tie-breaker when it does an ICE restart. channel2->SignalRoleConflict(channel2); - EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel1->GetIceRole()); - EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel2->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLING, channel1->GetIceRole()); + EXPECT_EQ(ICEROLE_CONTROLLING, channel2->GetIceRole()); } TEST_F(TransportControllerTest, TestGetSslRole) { @@ -335,18 +328,18 @@ TEST_F(TransportControllerTest, TestSetLocalTransportDescription) { FakeTransportChannel* channel = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel); TransportDescription local_desc(std::vector(), kIceUfrag1, - kIcePwd1, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); std::string err; EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( - "audio", local_desc, cricket::CA_OFFER, &err)); + "audio", local_desc, CA_OFFER, &err)); // Check that ICE ufrag and pwd were propagated to channel. EXPECT_EQ(kIceUfrag1, channel->ice_ufrag()); EXPECT_EQ(kIcePwd1, channel->ice_pwd()); // After setting local description, we should be able to start gathering // candidates. transport_controller_->MaybeStartGathering(); - EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(1, gathering_state_signal_count_); } @@ -354,11 +347,11 @@ TEST_F(TransportControllerTest, TestSetRemoteTransportDescription) { FakeTransportChannel* channel = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel); TransportDescription remote_desc(std::vector(), kIceUfrag1, - kIcePwd1, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); std::string err; EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription( - "audio", remote_desc, cricket::CA_OFFER, &err)); + "audio", remote_desc, CA_OFFER, &err)); // Check that ICE ufrag and pwd were propagated to channel. EXPECT_EQ(kIceUfrag1, channel->remote_ice_ufrag()); EXPECT_EQ(kIcePwd1, channel->remote_ice_pwd()); @@ -384,17 +377,17 @@ TEST_F(TransportControllerTest, TestReadyForRemoteCandidates) { std::string err; TransportDescription remote_desc(std::vector(), kIceUfrag1, - kIcePwd1, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription( - "audio", remote_desc, cricket::CA_OFFER, &err)); + "audio", remote_desc, CA_OFFER, &err)); EXPECT_FALSE(transport_controller_->ReadyForRemoteCandidates("audio")); TransportDescription local_desc(std::vector(), kIceUfrag2, - kIcePwd2, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd2, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( - "audio", local_desc, cricket::CA_ANSWER, &err)); + "audio", local_desc, CA_ANSWER, &err)); EXPECT_TRUE(transport_controller_->ReadyForRemoteCandidates("audio")); } @@ -435,7 +428,7 @@ TEST_F(TransportControllerTest, TestCreateAndDestroyChannel) { TEST_F(TransportControllerTest, TestSignalConnectionStateFailed) { // Need controlling ICE role to get in failed state. - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); FakeTransportChannel* channel2 = CreateChannel("video", 1); @@ -447,12 +440,12 @@ TEST_F(TransportControllerTest, TestSignalConnectionStateFailed) { channel1->SetCandidatesGatheringComplete(); channel1->SetConnectionCount(1); channel1->SetConnectionCount(0); - EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionFailed, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); } TEST_F(TransportControllerTest, TestSignalConnectionStateConnected) { - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); FakeTransportChannel* channel2 = CreateChannel("video", 1); @@ -468,25 +461,24 @@ TEST_F(TransportControllerTest, TestSignalConnectionStateConnected) { channel3->SetCandidatesGatheringComplete(); channel3->SetConnectionCount(1); channel3->SetConnectionCount(0); - EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionFailed, connection_state_, kTimeout); // Signal count of 1 means that the only signal emitted was "failed". EXPECT_EQ(1, connection_state_signal_count_); // Destroy the failed channel to return to "connecting" state. DestroyChannel("video", 2); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, - kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnecting, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); // Make the remaining channel reach a connected state. channel2->SetConnectionCount(2); channel2->SetWritable(true); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnected, connection_state_, kTimeout); EXPECT_EQ(3, connection_state_signal_count_); } TEST_F(TransportControllerTest, TestSignalConnectionStateComplete) { - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); FakeTransportChannel* channel2 = CreateChannel("video", 1); @@ -502,26 +494,25 @@ TEST_F(TransportControllerTest, TestSignalConnectionStateComplete) { channel3->SetCandidatesGatheringComplete(); channel3->SetConnectionCount(1); channel3->SetConnectionCount(0); - EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionFailed, connection_state_, kTimeout); // Signal count of 1 means that the only signal emitted was "failed". EXPECT_EQ(1, connection_state_signal_count_); // Destroy the failed channel to return to "connecting" state. DestroyChannel("video", 2); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, - kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnecting, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); // Make the remaining channel reach a connected state. channel2->SetCandidatesGatheringComplete(); channel2->SetConnectionCount(2); channel2->SetWritable(true); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnected, connection_state_, kTimeout); EXPECT_EQ(3, connection_state_signal_count_); // Finally, transition to completed state. channel2->SetConnectionCount(1); - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionCompleted, connection_state_, kTimeout); EXPECT_EQ(4, connection_state_signal_count_); } @@ -539,14 +530,14 @@ TEST_F(TransportControllerTest, TestDestroyTransportAndStayConnected) { channel2->SetCandidatesGatheringComplete(); channel2->SetConnectionCount(2); channel2->SetWritable(true); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnected, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); // Destroy one channel, then "complete" the other one, so we reach // a known state. DestroyChannel("video", 1); channel1->SetConnectionCount(1); - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionCompleted, connection_state_, kTimeout); // Signal count of 2 means the deletion didn't cause any unexpected signals EXPECT_EQ(2, connection_state_signal_count_); } @@ -560,12 +551,11 @@ TEST_F(TransportControllerTest, TestDestroyLastTransportWhileConnected) { channel->SetCandidatesGatheringComplete(); channel->SetConnectionCount(2); channel->SetWritable(true); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnected, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); DestroyChannel("audio", 1); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, - kTimeout); + EXPECT_EQ_WAIT(kIceConnectionConnecting, connection_state_, kTimeout); // Signal count of 2 means the deletion didn't cause any unexpected signals EXPECT_EQ(2, connection_state_signal_count_); } @@ -593,7 +583,7 @@ TEST_F(TransportControllerTest, TestSignalGatheringStateGathering) { ASSERT_NE(nullptr, channel); channel->MaybeStartGathering(); // Should be in the gathering state as soon as any transport starts gathering. - EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(1, gathering_state_signal_count_); } @@ -606,25 +596,25 @@ TEST_F(TransportControllerTest, TestSignalGatheringStateComplete) { ASSERT_NE(nullptr, channel3); channel3->MaybeStartGathering(); - EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(1, gathering_state_signal_count_); // Have one channel finish gathering, then destroy it, to make sure gathering // completion wasn't signalled if only one transport finished gathering. channel3->SetCandidatesGatheringComplete(); DestroyChannel("data", 1); - EXPECT_EQ_WAIT(cricket::kIceGatheringNew, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringNew, gathering_state_, kTimeout); EXPECT_EQ(2, gathering_state_signal_count_); // Make remaining channels start and then finish gathering. channel1->MaybeStartGathering(); channel2->MaybeStartGathering(); - EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(3, gathering_state_signal_count_); channel1->SetCandidatesGatheringComplete(); channel2->SetCandidatesGatheringComplete(); - EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringComplete, gathering_state_, kTimeout); EXPECT_EQ(4, gathering_state_signal_count_); } @@ -634,22 +624,22 @@ TEST_F(TransportControllerTest, TestSignalGatheringStateComplete) { // transport completes, then we start bundling video on the audio transport. TEST_F(TransportControllerTest, TestSignalingWhenLastIncompleteTransportDestroyed) { - transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); + transport_controller_->SetIceRole(ICEROLE_CONTROLLING); FakeTransportChannel* channel1 = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel1); FakeTransportChannel* channel2 = CreateChannel("video", 1); ASSERT_NE(nullptr, channel2); channel1->SetCandidatesGatheringComplete(); - EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(1, gathering_state_signal_count_); channel1->SetConnectionCount(1); channel1->SetWritable(true); DestroyChannel("video", 1); - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionCompleted, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); - EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringComplete, gathering_state_, kTimeout); EXPECT_EQ(2, gathering_state_signal_count_); } @@ -659,11 +649,11 @@ TEST_F(TransportControllerTest, TestSignalCandidatesGathered) { // Transport won't signal candidates until it has a local description. TransportDescription local_desc(std::vector(), kIceUfrag1, - kIcePwd1, cricket::ICEMODE_FULL, - cricket::CONNECTIONROLE_ACTPASS, nullptr); + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); std::string err; EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( - "audio", local_desc, cricket::CA_OFFER, &err)); + "audio", local_desc, CA_OFFER, &err)); transport_controller_->MaybeStartGathering(); channel->SignalCandidateGathered(channel, CreateCandidate(1)); @@ -676,14 +666,14 @@ TEST_F(TransportControllerTest, TestSignalingOccursOnSignalingThread) { CreateChannelsAndCompleteConnectionOnWorkerThread(); // connecting --> connected --> completed - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ_WAIT(kIceConnectionCompleted, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); EXPECT_TRUE_WAIT(receiving_, kTimeout); EXPECT_EQ(1, receiving_signal_count_); // new --> gathering --> complete - EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout); + EXPECT_EQ_WAIT(kIceGatheringComplete, gathering_state_, kTimeout); EXPECT_EQ(2, gathering_state_signal_count_); EXPECT_EQ_WAIT(1U, candidates_["audio"].size(), kTimeout); @@ -692,3 +682,39 @@ TEST_F(TransportControllerTest, TestSignalingOccursOnSignalingThread) { EXPECT_TRUE(!signaled_on_non_signaling_thread_); } + +// 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. +// See: https://bugs.chromium.org/p/chromium/issues/detail?id=628676 +// TODO(deadbeef): Remove this when these old versions of Chrome reach a low +// enough population. +TEST_F(TransportControllerTest, IceRoleRedeterminedOnIceRestart) { + FakeTransportChannel* channel = CreateChannel("audio", 1); + ASSERT_NE(nullptr, channel); + std::string err; + // Do an initial offer answer, so that the next offer is an ICE restart. + transport_controller_->SetIceRole(ICEROLE_CONTROLLED); + TransportDescription remote_desc(std::vector(), kIceUfrag1, + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, CA_OFFER, &err)); + TransportDescription local_desc(std::vector(), kIceUfrag2, + kIcePwd2, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, CA_ANSWER, &err)); + EXPECT_EQ(ICEROLE_CONTROLLED, channel->GetIceRole()); + + // The endpoint that initiated an ICE restart should take the controlling + // role. + TransportDescription ice_restart_desc(std::vector(), kIceUfrag3, + kIcePwd3, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", ice_restart_desc, CA_OFFER, &err)); + EXPECT_EQ(ICEROLE_CONTROLLING, channel->GetIceRole()); +} + +} // namespace cricket {