Fix for data channels perpetually stuck in "closing" state.

Fix for data channels perpetually stuck in "closing" state.

If the data transport is destroyed while data is buffered (due to
the PC being closed, or a description set with data rejected), the
data channel was getting stuck in a "closing" state, waiting to
finish sending its buffered data. But since there's no more transport,
it will never get another chance to send buffered data.

It just needs to terminate non-gracefully and discard the buffered data
in this situation.

R=skvlad@webrtc.org, zhihuang@webrtc.org

Committed: https://crrev.com/4cb5b64b16deeb1cb22141ad104ed83ff4899624
Cr-Commit-Position: refs/heads/master@{#13737}

[Taylor Brandstetter, 2016-08-11 03:10:28]

deadbeef@webrtc.org changed reviewers:
+ skvlad@webrtc.org, zhihuang@webrtc.org

[Taylor Brandstetter, 2016-08-11 03:10:28]

Zhi and Vlad, can you take a look? This is just a small bug I noticed while
debugging another issue.

[skvlad, 2016-08-11 17:05:06]

On 2016/08/11 03:10:28, Taylor Brandstetter wrote:
> Zhi and Vlad, can you take a look? This is just a small bug I noticed while
> debugging another issue.

lgtm

Unrelated to this change, but isn't it possible to have a race condition that
leads to message reordering when messages are sent just as the channel gets
unblocked?
DataChannel::SendDataMessage always attempts to send the message to the provider
- even if there are already messages in the queue. The queue gets drained in
DataChannel::OnChannelReady which appears to be delivered asynchronously.

So the following sequence appears to be possible:
 1. The transport becomes blocked
 2. Some messages get queued up
 3. The transport becomes unblocked, MSG_READYTOSENDDATA gets sent to the thread
 4. Before it gets picked up from the thread message queue, the application
sends another message on the data channel - which is able to successfully call
provider_->SendData() and sends the message out of order.
 5. MSG_READYTOSENDDATA is processed and the previously queued messages go out

[Taylor Brandstetter, 2016-08-11 17:19:33]

On 2016/08/11 17:05:06, skvlad wrote:
> On 2016/08/11 03:10:28, Taylor Brandstetter wrote:
> > Zhi and Vlad, can you take a look? This is just a small bug I noticed while
> > debugging another issue.
> 
> lgtm
> 
> Unrelated to this change, but isn't it possible to have a race condition that
> leads to message reordering when messages are sent just as the channel gets
> unblocked?
> DataChannel::SendDataMessage always attempts to send the message to the
provider
> - even if there are already messages in the queue. The queue gets drained in
> DataChannel::OnChannelReady which appears to be delivered asynchronously.
> 
> So the following sequence appears to be possible:
>  1. The transport becomes blocked
>  2. Some messages get queued up
>  3. The transport becomes unblocked, MSG_READYTOSENDDATA gets sent to the
thread
>  4. Before it gets picked up from the thread message queue, the application
> sends another message on the data channel - which is able to successfully call
> provider_->SendData() and sends the message out of order.
>  5. MSG_READYTOSENDDATA is processed and the previously queued messages go out

In step #4, you would hit this condition and the new message would be added to
the end of the queue:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/datachannel.cc?dr...

So I don't think this is an issue.

[Zhi Huang, 2016-08-11 17:29:41]

On 2016/08/11 17:19:33, Taylor Brandstetter wrote:
> On 2016/08/11 17:05:06, skvlad wrote:
> > On 2016/08/11 03:10:28, Taylor Brandstetter wrote:
> > > Zhi and Vlad, can you take a look? This is just a small bug I noticed
while
> > > debugging another issue.
> > 
> > lgtm
> > 
> > Unrelated to this change, but isn't it possible to have a race condition
that
> > leads to message reordering when messages are sent just as the channel gets
> > unblocked?
> > DataChannel::SendDataMessage always attempts to send the message to the
> provider
> > - even if there are already messages in the queue. The queue gets drained in
> > DataChannel::OnChannelReady which appears to be delivered asynchronously.
> > 
> > So the following sequence appears to be possible:
> >  1. The transport becomes blocked
> >  2. Some messages get queued up
> >  3. The transport becomes unblocked, MSG_READYTOSENDDATA gets sent to the
> thread
> >  4. Before it gets picked up from the thread message queue, the application
> > sends another message on the data channel - which is able to successfully
call
> > provider_->SendData() and sends the message out of order.
> >  5. MSG_READYTOSENDDATA is processed and the previously queued messages go
out
> 
> In step #4, you would hit this condition and the new message would be added to
> the end of the queue:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/datachannel.cc?dr...
> 
> So I don't think this is an issue.

lgtm

[Taylor Brandstetter, 2016-08-11 21:56:11]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-08-11 21:56:18]

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-08-11 22:19:21]

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

[commit-bot: I haz the power, 2016-08-11 22:19:22]

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

[Taylor Brandstetter, 2016-08-12 17:10:47]

Description was changed from

==========
Fix for data channels perpetually stuck in "closing" state.

If the data transport is destroyed while data is buffered (due to
the PC being closed, or a description set with data rejected), the
data channel was getting stuck in a "closing" state, waiting to
finish sending its buffered data. But since there's no more transport,
it will never get another chance to send buffered data.

It just needs to terminate non-gracefully and discard the buffered data
in this situation.
==========

to

==========
Fix for data channels perpetually stuck in "closing" state.

If the data transport is destroyed while data is buffered (due to
the PC being closed, or a description set with data rejected), the
data channel was getting stuck in a "closing" state, waiting to
finish sending its buffered data. But since there's no more transport,
it will never get another chance to send buffered data.

It just needs to terminate non-gracefully and discard the buffered data
in this situation.

R=skvlad@webrtc.org, zhihuang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/4cb5b64b16deeb1cb22141ad1...
==========

[Taylor Brandstetter, 2016-08-12 17:10:48]

Committed patchset #1 (id:1) manually as
4cb5b64b16deeb1cb22141ad104ed83ff4899624 (presubmit successful).

[commit-bot: I haz the power, 2016-08-12 17:10:52]

Description was changed from

==========
Fix for data channels perpetually stuck in "closing" state.

If the data transport is destroyed while data is buffered (due to
the PC being closed, or a description set with data rejected), the
data channel was getting stuck in a "closing" state, waiting to
finish sending its buffered data. But since there's no more transport,
it will never get another chance to send buffered data.

It just needs to terminate non-gracefully and discard the buffered data
in this situation.

R=skvlad@webrtc.org, zhihuang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/4cb5b64b16deeb1cb22141ad1...
==========

to

==========
Fix for data channels perpetually stuck in "closing" state.

If the data transport is destroyed while data is buffered (due to
the PC being closed, or a description set with data rejected), the
data channel was getting stuck in a "closing" state, waiting to
finish sending its buffered data. But since there's no more transport,
it will never get another chance to send buffered data.

It just needs to terminate non-gracefully and discard the buffered data
in this situation.

R=skvlad@webrtc.org, zhihuang@webrtc.org

Committed: https://crrev.com/4cb5b64b16deeb1cb22141ad104ed83ff4899624
Cr-Commit-Position: refs/heads/master@{#13737}
==========