Make net::BidirectionalStream handle RST_STREAM_NO_ERROR

Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle
RST_STREAM_NO_ERROR, so that future SendData() call will not
produce OnFailed() callback. This CL essentially make
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438
Committed: https://crrev.com/4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a
Cr-Commit-Position: refs/heads/master@{#428858}

[xunjieli, 2016-10-27 21:22:19]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-10-27 21:23:03]

Description was changed from

==========
Make net::BidirectionalStream handle RST_STREAM with no error.

This CL makes net::BidirectionalStream handle RST_STREAM with no error, so that
future SendData() call will not produce OnFailed() callback.
This CL essentially make net::BidirectionalStream blackholes SendData() after
receiving a RST_STREAM with no error.

BUG=
==========

to

==========
Make net::BidirectionalStream handle RST_STREAM with no error.

This CL makes net::BidirectionalStream handle RST_STREAM 
with no error, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackholes SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

[xunjieli, 2016-10-27 21:23:48]

Description was changed from

==========
Make net::BidirectionalStream handle RST_STREAM with no error.

This CL makes net::BidirectionalStream handle RST_STREAM 
with no error, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackholes SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

to

==========
Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle 
RST_STREAM_NO_ERROR, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

[xunjieli, 2016-10-27 21:28:54]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-10-27 21:30:06]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[xunjieli, 2016-10-27 21:30:06]

Andrei, thanks for the suggestions. What do you think about this patch?

[xunjieli, 2016-10-27 21:30:30]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 21:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 22:37:54]

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

[commit-bot: I haz the power, 2016-10-27 22:37:55]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2016-10-28 16:11:00]

https://codereview.chromium.org/2462463002/diff/40001/net/spdy/bidirectional_...
File net/spdy/bidirectional_stream_spdy_impl.cc (right):

https://codereview.chromium.org/2462463002/diff/40001/net/spdy/bidirectional_...
net/spdy/bidirectional_stream_spdy_impl.cc:375: if (stream_closed_ &&
closed_stream_status_ == OK) {
If the stream was closed because all data were sent & received, what will happen
if the client calls SendData()? Previously we were returning an error. As the
result of the change, will it blackhole the data now & call the delegate's
OnDataSent()?

[xunjieli, 2016-10-28 21:43:52]

Patchset #2 (id:60001) has been deleted

[xunjieli, 2016-10-28 21:51:53]

Patchset #2 (id:80001) has been deleted

[xunjieli, 2016-10-28 21:52:42]

Patchset #2 (id:100001) has been deleted

[xunjieli, 2016-10-28 21:54:13]

Thanks for the review! I have uploaded a new CL. Please feel free to delay the
next round to next week since it's pretty late. Have a great weekend!

[kapishnikov, 2016-10-31 17:03:07]

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl.cc (right):

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:114: 
Should we add DCHECK(!written_end_of_stream_) here or will it break some of our
tests?

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:134: 
Same DCHECK(!written_end_of_stream_) here

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:402: if (!written_end_of_stream_ &&
stream_closed_ &&
To make the code easier to understand, I think we should break the 'if' into 2
different cases:

1) When the client calls SendData() with end_stream == true second time, we
should return an error with the corresponding corresponding log message.
2) When SendData() is executed on a closed stream (as before).

Some can assume that if the client called SendData() with end_stream == true,
written_end_of_stream_ is always et to true. However, it is not always the case.

[xunjieli, 2016-10-31 17:18:03]

Thanks for the review! I think after making the suggested change, the code looks
less confusing. PTAL?

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl.cc (right):

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:114: 
On 2016/10/31 17:03:07, kapishnikov wrote:
> Should we add DCHECK(!written_end_of_stream_) here or will it break some of
our
> tests?

The newly added test will crash instead of having an error.

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:134: 
On 2016/10/31 17:03:07, kapishnikov wrote:
> Same DCHECK(!written_end_of_stream_) here

Same as above. The newly added test will crash. We either have a DCHECk crash or
an OnFailed() failure. I am fine with going either way.

https://codereview.chromium.org/2462463002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.cc:402: if (!written_end_of_stream_ &&
stream_closed_ &&
On 2016/10/31 17:03:07, kapishnikov wrote:
> To make the code easier to understand, I think we should break the 'if' into 2
> different cases:
> 
> 1) When the client calls SendData() with end_stream == true second time, we
> should return an error with the corresponding corresponding log message.
> 2) When SendData() is executed on a closed stream (as before).
> 
> Some can assume that if the client called SendData() with end_stream == true,
> written_end_of_stream_ is always et to true. However, it is not always the
case.

Done.

[kapishnikov, 2016-10-31 17:21:47]

https://codereview.chromium.org/2462463002/diff/140001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl_unittest.cc (right):

https://codereview.chromium.org/2462463002/diff/140001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl_unittest.cc:437:
TEST_F(BidirectionalStreamSpdyImplTest, RstWithNoErrorBeforeSendIsComplete) {
We need a similar test for SendvData

[xunjieli, 2016-10-31 18:41:55]

Patchset #4 (id:160001) has been deleted

[xunjieli, 2016-10-31 18:42:03]

Patchset #4 (id:180001) has been deleted

[xunjieli, 2016-10-31 18:42:19]

Thanks for the review! PTAL

https://codereview.chromium.org/2462463002/diff/140001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl_unittest.cc (right):

https://codereview.chromium.org/2462463002/diff/140001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl_unittest.cc:437:
TEST_F(BidirectionalStreamSpdyImplTest, RstWithNoErrorBeforeSendIsComplete) {
On 2016/10/31 17:21:47, kapishnikov wrote:
> We need a similar test for SendvData

Done.

[kapishnikov, 2016-10-31 18:56:22]

lgtm

[xunjieli, 2016-10-31 18:56:34]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-10-31 18:57:05]

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

[commit-bot: I haz the power, 2016-10-31 19:24:38]

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

[commit-bot: I haz the power, 2016-10-31 19:24:39]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xunjieli, 2016-10-31 19:32:02]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-31 19:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2462463002/#ps220001
(title: "Fix compile")

[commit-bot: I haz the power, 2016-10-31 19:33:02]

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

[commit-bot: I haz the power, 2016-10-31 20:03:04]

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

[commit-bot: I haz the power, 2016-10-31 20:03:05]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[xunjieli, 2016-10-31 21:58:31]

Thanks, Andrei! The problem was that |write_pending_| isn't set back to false
when OnDataSent() is invoked by SpdyStream. Could you take a look at diffs
between PS#5 and PS#6?

[xunjieli, 2016-10-31 21:58:38]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 21:59:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kapishnikov, 2016-10-31 22:05:02]

Looks good. LGTM

[xunjieli, 2016-10-31 22:05:34]

On 2016/10/31 22:05:02, kapishnikov wrote:
> Looks good. LGTM

Thanks!

[xunjieli, 2016-10-31 22:05:56]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-10-31 22:06:00]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-10-31 22:06:30]

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

[commit-bot: I haz the power, 2016-10-31 23:16:25]

Description was changed from

==========
Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle 
RST_STREAM_NO_ERROR, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

to

==========
Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle 
RST_STREAM_NO_ERROR, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

[commit-bot: I haz the power, 2016-10-31 23:16:26]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2016-10-31 23:21:04]

Description was changed from

==========
Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle 
RST_STREAM_NO_ERROR, so that future SendData() call will not 
produce OnFailed() callback. This CL essentially make 
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438
==========

to

==========
Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle
RST_STREAM_NO_ERROR, so that future SendData() call will not
produce OnFailed() callback. This CL essentially make
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG=650438

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

[commit-bot: I haz the power, 2016-10-31 23:21:05]

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