First step of passive aggressive nomination.

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 68c46d6a74da4c3553cbcfc0877535a611aa9c7d..98773699d3cef33bd475c239fc2124711011edf7 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1884,3 +1884,184 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) {
   EXPECT_TRUE_WAIT(ch.receiving(), 1000);
   EXPECT_TRUE_WAIT(!ch.receiving(), 1000);
 }
+
+// When the p2p transport channel with a controlled role received a candidate,

[pthatcher1, 2015/08/07 22:50:22]

You could just shorten this to 'The controlled side will select a connection as
the "best connection" based on priority until the controlling side nominates a
connection, at which point the controlled side will select that connection as
the "best connection".

[honghaiz3, 2015/08/10 20:15:43]

Done.

+// it should set up the best connection if the best connection was not nominated
+// by the controlling side (via use_candidate attribute). If the best connection
+// was nominated by the controlling side, the connection receiving a new
+// candidate will not be set as the best connection.
+TEST_F(P2PTransportChannelPingTest, TestPassiveAggressiveNomination) {

[pthatcher1, 2015/08/07 22:50:22]

It's not yet passive aggressive nomination.  It's more like
"SelectConnectionBeforeNominationBaseOnPrioirty"

[honghaiz3, 2015/08/10 20:15:43]

Done. Shortened the name a little bit. 

+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
+  ch.Connect();
+  ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1));
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(conn1, ch.best_connection());
+
+  // When a higher priority candidate comes in, the new connection is chosen
+  // as the best connection.
+  ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 10));
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(conn2, ch.best_connection());
+
+  // If a stun request with use-candidate attribute arrives, the receiving
+  // connection will be set as the best connection, even though
+  // its priority is lower.
+  ch.OnCandidate(CreateCandidate("3.3.3.3", 3, 1));
+  cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
+  ASSERT_TRUE(conn3 != nullptr);
+  // Because it has a lower priority, the best connection is still conn2.
+  EXPECT_EQ(conn2, ch.best_connection());
+  conn3->ReceivedPingResponse();  // Become writable.
+  // But if it is nominated via use_candidate, it is chosen as the best
+  // connection.
+  conn3->set_nominated(true);
+  conn3->SignalUseCandidate(conn3);

[pthatcher1, 2015/08/07 22:50:22]

Maybe we should just fire SignalNominated within set_nominated.

[honghaiz3, 2015/08/10 20:15:43]

That's what I did in a previous patch. But Justin preferred this way.
Justin, with the name changes of the signal, do you think it is reasonable to
put the SignalNominated inside set_nominated?

+  EXPECT_EQ(conn3, ch.best_connection());
+
+  // Even if another higher priority candidate arrives,
+  // it will not be set as the best connection because the best connection
+  // is nominated by the controlling side.
+  ch.OnCandidate(CreateCandidate("4.4.4.4", 4, 100));
+  cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
+  ASSERT_TRUE(conn4 != nullptr);
+  EXPECT_EQ(conn3, ch.best_connection());
+  // But if it is nominated via use_candidate and writable, it will be set as
+  // the best connection.
+  conn4->set_nominated(true);
+  conn4->SignalUseCandidate(conn4);
+  // Not switched yet because conn4 is not writable.
+  EXPECT_EQ(conn3, ch.best_connection());
+  // The best connection switches after conn4 becomes writable.
+  conn4->ReceivedPingResponse();
+  EXPECT_EQ(conn4, ch.best_connection());
+}
+
+// For the controlled side, when a stun request with an unknown address is
+// received, it will be selected as the best connection if the best connection
+// is not nominated by the controlling side and vice versa. If the received
+// stun request with an unknown address contains a use_candidate attribute, it
+// will be treated as a nomination from the controlling side.

[pthatcher1, 2015/08/07 22:50:22]

I think you could shorten this to "The controlled side will select a connection
before the controlled side nominates peer reflexive candidates, and will honor
nominations from peer reflexive candidates."

[honghaiz3, 2015/08/10 20:15:43]

Done. Thanks!

+TEST_F(P2PTransportChannelPingTest, TestReceivingUnknownAddress) {

[pthatcher1, 2015/08/07 22:50:22]

This is kind of two tests in one: 
SelectConnectionBeforeNominationWithPeerReflexCandidates and
HonorNominatedPeerReflexCandidates.  Maybe just
TestNominationOfPeerReflexCandidates.

[honghaiz3, 2015/08/10 20:15:43]

I think this is more on nominations from Unknown Address. 
I just happened to choose the peer reflexive priority for the request although
that should not matter that much (I want it to be higher than the one in
nominated to test that even if the nominated one has lower priority, it should
still be selected). 
Revised names and comments a little bit (all three added tests have name
SelectionConnection....

+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
+  ch.Connect();
+  // A minimal STUN message with prflx priority.
+  cricket::IceMessage request;
+  request.SetType(cricket::STUN_BINDING_REQUEST);
+  request.AddAttribute(new cricket::StunByteStringAttribute(
+      cricket::STUN_ATTR_USERNAME, kIceUfrag[1]));
+  uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24;
+  request.AddAttribute(new cricket::StunUInt32Attribute(
+      cricket::STUN_ATTR_PRIORITY, prflx_priority));
+  cricket::Port* port = GetPort(&ch);
+  port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1),
+                             cricket::PROTO_UDP, &request, kIceUfrag[1], false);
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(conn1, ch.best_connection());
+  conn1->ReceivedPingResponse();
+  EXPECT_EQ(conn1, ch.best_connection());
+
+  // Another connection is nominated via use_candidate.
+  ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 1));
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn2 != nullptr);
+  // Because it has a lower priority, the best connection is still conn1.
+  EXPECT_EQ(conn1, ch.best_connection());
+  // When it is nominated via use_candidate and writable, it is chosen as the
+  // best connection.
+  conn2->ReceivedPingResponse();  // Become writable.
+  conn2->set_nominated(true);
+  conn2->SignalUseCandidate(conn2);
+  EXPECT_EQ(conn2, ch.best_connection());
+
+  // Another request with unknown address, it will not be set as the best
+  // connection because the best connection was nominated by the controlling
+  // side.
+  port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 3),
+                             cricket::PROTO_UDP, &request, kIceUfrag[1], false);
+  cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
+  ASSERT_TRUE(conn3 != nullptr);
+  conn3->ReceivedPingResponse();  // Become writable.
+  EXPECT_EQ(conn2, ch.best_connection());
+
+  // However if the request contains use_candidate attribute, it will be
+  // selected as the best connection.
+  request.AddAttribute(
+      new cricket::StunByteStringAttribute(cricket::STUN_ATTR_USE_CANDIDATE));
+  port->SignalUnknownAddress(port, rtc::SocketAddress("4.4.4.4", 4),
+                             cricket::PROTO_UDP, &request, kIceUfrag[1], false);
+  cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
+  ASSERT_TRUE(conn4 != nullptr);
+  // conn4 is not the best connection yet because it is not writable.
+  EXPECT_EQ(conn2, ch.best_connection());
+  conn4->ReceivedPingResponse();  // Become writable.
+  EXPECT_EQ(conn4, ch.best_connection());
+}
+
+// For the controlled side, if a data packet is received on a connection,
+// the sending path will mirror the receiving path if it is writable and
+// the current best connection is not nominated by the controlling side.
+// If the current best connection is already nominated by the controlling side,
+// it will not switch the best connection (the sending path).

[pthatcher1, 2015/08/07 22:50:22]

This could be shortened to 'The controlled side will select a connection as the
"best connection" based on media received until the controlling side nominates a
connection, at which point the controlled side will select that connection as
the "best connection".

[honghaiz3, 2015/08/10 20:15:43]

Done.

+TEST_F(P2PTransportChannelPingTest, TestSendingPathMirrorReceivingPath) {

[pthatcher1, 2015/08/07 22:50:22]

This might be called SelectConnectionBeforeNominationBaseOnMediaReceived

[honghaiz3, 2015/08/10 20:15:43]

Shortened a little bit. Since the comments have described the tests in more
details, guess don't have to make the name capture everything. 

+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
+  ch.Connect();
+  ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 10));
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(conn1, ch.best_connection());
+
+  // If a data packet is received on conn2, the best connection should
+  // switch to conn2 because the controlled side must mirror the media path
+  // chosen by the controlling side.
+  ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 1));
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn2 != nullptr);
+  conn2->ReceivedPing();  // Become readable.
+  // Do not switch because it is not writable.
+  conn2->OnReadPacket("ABC", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(conn1, ch.best_connection());
+
+  conn2->ReceivedPingResponse();  // Become writable.
+  // Switch because it is writable.
+  conn2->OnReadPacket("DEF", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(conn2, ch.best_connection());
+
+  // Now another STUN message with an unknown address and use_candidate will
+  // nominate the best connection.
+  cricket::IceMessage request;
+  request.SetType(cricket::STUN_BINDING_REQUEST);
+  request.AddAttribute(new cricket::StunByteStringAttribute(
+      cricket::STUN_ATTR_USERNAME, kIceUfrag[1]));
+  uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24;
+  request.AddAttribute(new cricket::StunUInt32Attribute(
+      cricket::STUN_ATTR_PRIORITY, prflx_priority));
+  request.AddAttribute(
+      new cricket::StunByteStringAttribute(cricket::STUN_ATTR_USE_CANDIDATE));
+  cricket::Port* port = GetPort(&ch);
+  port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 3),
+                             cricket::PROTO_UDP, &request, kIceUfrag[1], false);
+  cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
+  ASSERT_TRUE(conn3 != nullptr);
+  EXPECT_EQ(conn2, ch.best_connection());  // Not writable yet.
+  conn3->ReceivedPingResponse();           // Become writable.
+  EXPECT_EQ(conn3, ch.best_connection());
+
+  // Now another data packet will not switch the best connection because the
+  // best connection was nominated by the controlling side.
+  conn2->ReceivedPing();
+  conn2->ReceivedPingResponse();
+  conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(conn3, ch.best_connection());
+}