Fixing bug where Connection drops packets when presumed writable.

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

Committed: https://crrev.com/6bb1ef2b868fef645fdddb41599d6a6693b634d8
Cr-Commit-Position: refs/heads/master@{#13307}

[Taylor Brandstetter, 2016-06-24 19:17:28]

deadbeef@webrtc.org changed reviewers:
+ honghaiz@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-06-24 19:17:28]

This fixes the issue Honghai noticed earlier. It's also now covered by the test
(I thought I covered it by checking if SignalReadyToSend was fired, but I was
wrong).

[pthatcher1, 2016-06-25 00:15:51]

lgtm

[honghaiz3, 2016-06-27 16:12:55]

https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest.cc
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest...
webrtc/p2p/base/port_unittest.cc:2469: EXPECT_EQ(data_size,
ch1.conn()->Send(data, data_size, options));
I am really not sure about the decision we have made here that the connection
will just send down anything regardless its states. It will send even if it has
timed out, or even failed. This essentially makes Connection not an independent
class. It can only be used by the p2ptransportchannel. If any other class want
to use it, it has to repeat the same logic we have done in the
p2ptransportchannel. 

I am fine if you want to fix this issue quickly although I suggest we should
revisit the whole decision later.

[Taylor Brandstetter, 2016-06-27 17:35:42]

https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest.cc
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest...
webrtc/p2p/base/port_unittest.cc:2469: EXPECT_EQ(data_size,
ch1.conn()->Send(data, data_size, options));
On 2016/06/27 16:12:54, honghaiz3 wrote:
> I am really not sure about the decision we have made here that the connection
> will just send down anything regardless its states. It will send even if it
has
> timed out, or even failed. This essentially makes Connection not an
independent
> class. It can only be used by the p2ptransportchannel. If any other class want
> to use it, it has to repeat the same logic we have done in the
> p2ptransportchannel. 
> 
> I am fine if you want to fix this issue quickly although I suggest we should
> revisit the whole decision later. 

I guess I have a different opinion. I feel that making it less restrictive makes
it more generally usable. If another class calls Send while it's in the timeout
or init state, it seems like we should assume that class knows what it's doing
and attempt to send. Maybe some optimization in P2PTransportChannel will do that
in the future.  We may even want to push the decision of whether or not to send
further up the chain in the future; maybe the audio/video engines would like to
decide whether or not to send when the state is "timeout".

[honghaiz3, 2016-06-27 17:48:11]

lgtm

We should go ahead to land the CL. 
I think it is a good idea to push the state information to the upper part of the
sending chain although we should not remove the necessary sanity checking on the
connection state before we push it to the wire, just in case it was mis-used
anywhere on the sending chain.

[pthatcher1, 2016-06-27 18:05:19]

On 2016/06/27 16:12:55, honghaiz3 wrote:
>
https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest.cc
> File webrtc/p2p/base/port_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2099783002/diff/1/webrtc/p2p/base/port_unittest...
> webrtc/p2p/base/port_unittest.cc:2469: EXPECT_EQ(data_size,
> ch1.conn()->Send(data, data_size, options));
> I am really not sure about the decision we have made here that the connection
> will just send down anything regardless its states. It will send even if it
has
> timed out, or even failed. This essentially makes Connection not an
independent
> class. It can only be used by the p2ptransportchannel. 

I don't expect anything other than P2pTransportChannel will ever use Connection.
 Connections, and even Ports, are really just implementation details of what we
call in the future IceTransport and IceGatherer (currently called, roughly,
P2pTransportChannel and PortAllocator).  We should clean up the relationship of
these classes as we go along, but I think the trend toward moving logic into
IceTransport out out of Connection is a good one.  

> If any other class want
> to use it, it has to repeat the same logic we have done in the
> p2ptransportchannel. 
> 
> I am fine if you want to fix this issue quickly although I suggest we should
> revisit the whole decision later.

[Taylor Brandstetter, 2016-06-28 00:28:58]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-06-28 00:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2099783002/#ps20001 (title:
"Merge with master.")

[commit-bot: I haz the power, 2016-06-28 00:29:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-06-28 00:36:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 00:36:45]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14533)

[commit-bot: I haz the power, 2016-06-28 01:09:15]

Description was changed from

==========
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.
==========

to

==========
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

Committed: https://crrev.com/6bb1ef2b868fef645fdddb41599d6a6693b634d8
Cr-Commit-Position: refs/heads/master@{#13307}
==========

[Taylor Brandstetter, 2016-06-28 01:09:16]

Description was changed from

==========
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

Committed: https://crrev.com/6bb1ef2b868fef645fdddb41599d6a6693b634d8
Cr-Commit-Position: refs/heads/master@{#13307}
==========

to

==========
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

Committed:
https://chromium.googlesource.com/external/webrtc/+/6bb1ef2b868fef645fdddb415...
==========

[commit-bot: I haz the power, 2016-06-28 01:09:16]

Patchset 2 (id:??) landed as
https://crrev.com/6bb1ef2b868fef645fdddb41599d6a6693b634d8
Cr-Commit-Position: refs/heads/master@{#13307}

[Taylor Brandstetter, 2016-06-28 01:09:17]

Committed patchset #2 (id:20001) manually as
6bb1ef2b868fef645fdddb41599d6a6693b634d8 (presubmit successful).