Fixing bug where Connection drops packets when presumed writable.

The "should I simulate EWOULDBLOCK?" determination now happens
solely in P2PTransportChannel. This also fixes a bug where the
"last packet id" was set even if no packet was sent.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Review URL: https://codereview.webrtc.org/2099783002 .

Cr-Commit-Position: refs/heads/master@{#13307}

webrtc/p2p/base/candidatepairinterface.h

@@ -21,8 +21,6 @@ class CandidatePairInterface {
 
   virtual const Candidate& local_candidate() const = 0;
   virtual const Candidate& remote_candidate() const = 0;
-
-  virtual bool ReadyToSendMedia() const = 0;
 };
 
 }  // namespace cricket

webrtc/p2p/base/dtlstransportchannel.cc

@@ -665,10 +665,11 @@ void DtlsTransportChannelWrapper::OnRouteChange(
 void DtlsTransportChannelWrapper::OnSelectedCandidatePairChanged(
     TransportChannel* channel,
     CandidatePairInterface* selected_candidate_pair,
-    int last_sent_packet_id) {
+    int last_sent_packet_id,
+    bool ready_to_send) {
   ASSERT(channel == channel_);
   SignalSelectedCandidatePairChanged(this, selected_candidate_pair,
-                                     last_sent_packet_id);
+                                     last_sent_packet_id, ready_to_send);
 }
 
 void DtlsTransportChannelWrapper::OnChannelStateChanged(

webrtc/p2p/base/dtlstransportchannel.h

@@ -220,7 +220,8 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl {
   void OnSelectedCandidatePairChanged(
       TransportChannel* channel,
       CandidatePairInterface* selected_candidate_pair,
-      int last_sent_packet_id);
+      int last_sent_packet_id,
+      bool ready_to_send);
   void OnChannelStateChanged(TransportChannelImpl* channel);
   void Reconnect();
 

webrtc/p2p/base/faketransportcontroller.h

@@ -476,8 +476,6 @@ class FakeCandidatePair : public CandidatePairInterface {
     return remote_candidate_;
   }
 
-  bool ReadyToSendMedia() const override { return true; }
-
  private:
   Candidate local_candidate_;
   Candidate remote_candidate_;

webrtc/p2p/base/p2ptransportchannel.cc

@@ -888,7 +888,9 @@ int P2PTransportChannel::SendPacket(const char *data, size_t len,
     error_ = EINVAL;
     return -1;
   }
-  if (selected_connection_ == NULL) {
+  // If we don't think the connection is working yet, return EWOULDBLOCK
+  // instead of sending a packet that will probably be dropped.
+  if (!ReadyToSend()) {
     error_ = EWOULDBLOCK;
     return -1;
   }
@@ -1228,7 +1230,7 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) {
     LOG_J(LS_INFO, this) << "No selected connection";
   }
   SignalSelectedCandidatePairChanged(this, selected_connection_,
-                                     last_sent_packet_id_);
+                                     last_sent_packet_id_, ReadyToSend());
 }
 
 // Warning: UpdateState should eventually be called whenever a connection
@@ -1322,6 +1324,17 @@ bool P2PTransportChannel::weak() const {
   return !selected_connection_ || selected_connection_->weak();
 }
 
+bool P2PTransportChannel::ReadyToSend() const {
+  // Note that we allow sending on an unreliable connection, because it's
+  // possible that it became unreliable simply due to bad chance.
+  // So this shouldn't prevent attempting to send media.
+  return selected_connection_ != nullptr &&
+         (selected_connection_->writable() ||
+          PresumedWritable(selected_connection_) ||
+          selected_connection_->write_state() ==
+              Connection::STATE_WRITE_UNRELIABLE);
+}
+
 // If we have a selected connection, return it, otherwise return top one in the
 // list (later we will mark it best).
 Connection* P2PTransportChannel::GetBestConnectionOnNetwork(

webrtc/p2p/base/p2ptransportchannel.h

@@ -212,6 +212,8 @@ class P2PTransportChannel : public TransportChannelImpl,
   // A transport channel is weak if the current best connection is either
   // not receiving or not writable, or if there is no best connection at all.
   bool weak() const;
+  // Returns true if it's possible to send packets on this channel.
+  bool ReadyToSend() const;
   void UpdateConnectionStates();
   void RequestSort();
 

webrtc/p2p/base/p2ptransportchannel_unittest.cc

@@ -1698,6 +1698,10 @@ TEST_F(P2PTransportChannelTest, TurnToTurnPresumedWritable) {
   // it has a TURN-TURN pair.
   EXPECT_TRUE(ep1_ch1()->writable());
   EXPECT_TRUE(GetEndpoint(0)->ready_to_send_);
+  // Also make sure we can immediately send packets.
+  const char* data = "test";
+  int len = static_cast<int>(strlen(data));
+  EXPECT_EQ(len, SendData(ep1_ch1(), data, len));
 }
 
 // Test that a TURN/peer reflexive candidate pair is also presumed writable.
@@ -2233,7 +2237,8 @@ class P2PTransportChannelPingTest : public testing::Test,
   void OnSelectedCandidatePairChanged(
       TransportChannel* transport_channel,
       CandidatePairInterface* selected_candidate_pair,
-      int last_sent_packet_id) {
+      int last_sent_packet_id,
+      bool ready_to_send) {
     last_selected_candidate_pair_ = selected_candidate_pair;
     last_sent_packet_id_ = last_sent_packet_id;
     ++selected_candidate_pair_switches_;
@@ -2639,7 +2644,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) {
   int last_packet_id = 0;
   const char* data = "ABCDEFGH";
   int len = static_cast<int>(strlen(data));
-  SendData(ch, data, len, ++last_packet_id);
+  EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id));
   // When a higher priority candidate comes in, the new connection is chosen
   // as the selected connection.
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 10));
@@ -2647,13 +2652,13 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) {
   ASSERT_TRUE(conn2 != nullptr);
   EXPECT_EQ(conn2, ch.selected_connection());
   EXPECT_EQ(conn2, last_selected_candidate_pair());
-  EXPECT_EQ(last_packet_id, last_sent_packet_id());
+  EXPECT_EQ(-1, last_sent_packet_id());
   EXPECT_FALSE(channel_ready_to_send());
 
   // If a stun request with use-candidate attribute arrives, the receiving
   // connection will be set as the selected connection, even though
   // its priority is lower.
-  SendData(ch, data, len, ++last_packet_id);
+  EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id));
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "3.3.3.3", 3, 1));
   Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
   ASSERT_TRUE(conn3 != nullptr);
@@ -2666,13 +2671,13 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) {
   conn3->SignalNominated(conn3);
   EXPECT_EQ(conn3, ch.selected_connection());
   EXPECT_EQ(conn3, last_selected_candidate_pair());
-  EXPECT_EQ(last_packet_id, last_sent_packet_id());
+  EXPECT_EQ(-1, last_sent_packet_id());
   EXPECT_TRUE(channel_ready_to_send());
 
   // Even if another higher priority candidate arrives, it will not be set as
   // the selected connection because the selected connection is nominated by
   // the controlling side.
-  SendData(ch, data, len, ++last_packet_id);
+  EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id));
   ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "4.4.4.4", 4, 100));
   Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
   ASSERT_TRUE(conn4 != nullptr);

webrtc/p2p/base/port.cc

@@ -1525,10 +1525,6 @@ ProxyConnection::ProxyConnection(Port* port,
 
 int ProxyConnection::Send(const void* data, size_t size,
                           const rtc::PacketOptions& options) {
-  if (!ReadyToSendMedia()) {
-    error_ = EWOULDBLOCK;
-    return SOCKET_ERROR;
-  }
   stats_.sent_total_packets++;
   int sent = port_->SendTo(data, size, remote_candidate_.address(),
                            options, true);

webrtc/p2p/base/port.h

@@ -461,10 +461,6 @@ class Connection : public CandidatePairInterface,
   bool active() const {
     return write_state_ != STATE_WRITE_TIMEOUT;
   }
-  virtual bool ReadyToSendMedia() const {
-    return write_state_ == STATE_WRITABLE ||
-           write_state_ == STATE_WRITE_UNRELIABLE;
-  }
 
   // A connection is dead if it can be safely deleted.
   bool dead(int64_t now) const;

webrtc/p2p/base/port_unittest.cc

@@ -2424,11 +2424,11 @@ TEST_F(PortTest, TestWritableState) {
   ch1.Ping();
   WAIT(!ch2.remote_address().IsNil(), kTimeout);
 
-  // Data should be unsendable until the connection is accepted.
+  // Data should be sendable before the connection is accepted.
   char data[] = "abcd";
   int data_size = arraysize(data);
   rtc::PacketOptions options;
-  EXPECT_EQ(SOCKET_ERROR, ch1.conn()->Send(data, data_size, options));
+  EXPECT_EQ(data_size, ch1.conn()->Send(data, data_size, options));
 
   // Accept the connection to return the binding response, transition to
   // writable, and allow data to be sent.
@@ -2464,8 +2464,9 @@ TEST_F(PortTest, TestWritableState) {
                           500u);
   EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state());
 
-  // Now that the connection has completely timed out, data send should fail.
-  EXPECT_EQ(SOCKET_ERROR, ch1.conn()->Send(data, data_size, options));
+  // Even if the connection has timed out, the Connection shouldn't block
+  // the sending of data.
+  EXPECT_EQ(data_size, ch1.conn()->Send(data, data_size, options));
 
   ch1.Stop();
   ch2.Stop();

webrtc/p2p/base/transportchannel.h

@@ -160,7 +160,9 @@ class TransportChannel : public sigslot::has_slots<> {
   // The first parameter is the transport channel that signals the event.
   // The second parameter is the new selected candidate pair. The third
   // parameter is the last packet id sent on the previous candidate pair.
-  sigslot::signal3<TransportChannel*, CandidatePairInterface*, int>
+  // The fourth parameter is a boolean which is true if the TransportChannel
+  // is ready to send with this candidate pair.
+  sigslot::signal4<TransportChannel*, CandidatePairInterface*, int, bool>
       SignalSelectedCandidatePairChanged;
 
   // Invoked when the channel is being destroyed.

webrtc/p2p/base/turnport_unittest.cc

@@ -1019,12 +1019,12 @@ TEST_F(TurnPortTest, TestChannelBindGetErrorResponse) {
 
   EXPECT_TRUE_SIMULATED_WAIT(CheckConnectionFailedAndPruned(conn1),
                              kSimulatedRtt, fake_clock_);
-  // Verify that no packet can be sent after a bind request error.
+  // Verify that packets are allowed to be sent after a bind request error.
+  // They'll just use a send indication instead.
   conn2->SignalReadPacket.connect(static_cast<TurnPortTest*>(this),
                                   &TurnPortTest::OnUdpReadPacket);
   conn1->Send(data.data(), data.length(), options);
-  SIMULATED_WAIT(!udp_packets_.empty(), kSimulatedRtt, fake_clock_);
-  EXPECT_TRUE(udp_packets_.empty());
+  EXPECT_TRUE_SIMULATED_WAIT(!udp_packets_.empty(), kSimulatedRtt, fake_clock_);
 }
 
 // Do a TURN allocation, establish a UDP connection, and send some data.

webrtc/p2p/quic/quictransportchannel.cc

@@ -395,10 +395,10 @@ void QuicTransportChannel::OnRouteChange(TransportChannel* channel,
 void QuicTransportChannel::OnSelectedCandidatePairChanged(
     TransportChannel* channel,
     CandidatePairInterface* selected_candidate_pair,
-    int last_sent_packet_id) {
+    int last_sent_packet_id bool ready_to_send) {
   ASSERT(channel == channel_.get());
   SignalSelectedCandidatePairChanged(this, selected_candidate_pair,
-                                     last_sent_packet_id);
+                                     last_sent_packet_id, ready_to_send);
 }
 
 void QuicTransportChannel::OnChannelStateChanged(

webrtc/p2p/quic/quictransportchannel.h

@@ -246,7 +246,8 @@ class QuicTransportChannel : public TransportChannelImpl,
   void OnSelectedCandidatePairChanged(
       TransportChannel* channel,
       CandidatePairInterface* selected_candidate_pair,
-      int last_sent_packet_id);
+      int last_sent_packet_id,
+      bool ready_to_send);
   void OnChannelStateChanged(TransportChannelImpl* channel);
 
   // Callbacks for |quic_|.

webrtc/pc/channel.cc

@@ -601,15 +601,15 @@ void BaseChannel::OnDtlsState(TransportChannel* channel,
 void BaseChannel::OnSelectedCandidatePairChanged(
     TransportChannel* channel,
     CandidatePairInterface* selected_candidate_pair,
-    int last_sent_packet_id) {
+    int last_sent_packet_id,
+    bool ready_to_send) {
   ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
   RTC_DCHECK(network_thread_->IsCurrent());
   std::string transport_name = channel->transport_name();
   rtc::NetworkRoute network_route;
   if (selected_candidate_pair) {
     network_route = rtc::NetworkRoute(
-        selected_candidate_pair->ReadyToSendMedia(),
-        selected_candidate_pair->local_candidate().network_id(),
+        ready_to_send, selected_candidate_pair->local_candidate().network_id(),
         selected_candidate_pair->remote_candidate().network_id(),
         last_sent_packet_id);
   }

webrtc/pc/channel.h

@@ -222,7 +222,8 @@ class BaseChannel
   void OnSelectedCandidatePairChanged(
       TransportChannel* channel,
       CandidatePairInterface* selected_candidate_pair,
-      int last_sent_packet_id);
+      int last_sent_packet_id,
+      bool ready_to_send);
 
   bool PacketIsRtcp(const TransportChannel* channel, const char* data,
                     size_t len);

webrtc/pc/channel_unittest.cc

@@ -915,8 +915,8 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
     media_channel1->set_num_network_route_changes(0);
     network_thread_->Invoke<void>(RTC_FROM_HERE, [transport_channel1] {
       // The transport channel becomes disconnected.
-      transport_channel1->SignalSelectedCandidatePairChanged(transport_channel1,
-                                                             nullptr, -1);
+      transport_channel1->SignalSelectedCandidatePairChanged(
+          transport_channel1, nullptr, -1, false);
     });
     WaitForThreads();
     EXPECT_EQ(1, media_channel1->num_network_route_changes());
@@ -933,7 +933,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
           transport_controller1_->CreateFakeCandidatePair(
               local_address, kLocalNetId, remote_address, kRemoteNetId));
       transport_channel1->SignalSelectedCandidatePairChanged(
-          transport_channel1, candidate_pair.get(), kLastPacketId);
+          transport_channel1, candidate_pair.get(), kLastPacketId, true);
     });
     WaitForThreads();
     EXPECT_EQ(1, media_channel1->num_network_route_changes());