Enable SpdyStream's HALF_CLOSED_REMOTE state.

Enable SpdyStream's HALF_CLOSED_REMOTE state.

Unit-tests cover HTTP case of a response received before post completes,
and websocket case of a CLOSE frame with SPDY FIN set (client should
still send a websocket CLOSE).

Also rename SpdyStream::IsIdleTemporaryRename() back to IsIdle().

TEST=Also tested with Fedor's test case from the ticket.
BUG=330860
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251450

[Johnny, 2014-01-08 22:25:51]

Hi Will & Fred,

For when you're ready, this is the follow-up CL which enables HALF_CLOSED_REMOTE
states in SpdyStream, and allows responses while requests are still in-flight.

This is meant to apply on top of 126923003 (thus the try breaks).

[akalin, 2014-01-14 00:09:12]

On 2014/01/08 22:25:51, Johnny wrote:
> Hi Will & Fred,
> 
> For when you're ready, this is the follow-up CL which enables
HALF_CLOSED_REMOTE
> states in SpdyStream, and allows responses while requests are still in-flight.
> 
> This is meant to apply on top of 126923003 (thus the try breaks).

FWIW, you can do 'git try origin/master' instead of 'git cl try' to send tryjobs
with local diffs.

[akalin, 2014-01-28 23:31:59]

Replacing myself with rch.

[Johnny, 2014-02-14 00:07:08]

Hi Ryan,

This is rebased and ready for a look. Thanks,

-johnny

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc#...
net/spdy/spdy_stream.cc:545: // Notify delegate of write completion. Must not
destroy |this|.
This looks scary on the surface, but I mixed up this comment in the previous
"Remove SpdyStream::DoLoop()" CL. Prior to that, Fred had comments on the calls
to delegate_->OnRequestHeadersSent() and delegate_->OnDataSent(), pointing out
that they _couldn't_ destroy |this|.

[Ryan Hamilton, 2014-02-14 01:47:21]

lgtm

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc#...
net/spdy/spdy_stream.cc:545: // Notify delegate of write completion. Must not
destroy |this|.
On 2014/02/14 00:07:08, Johnny wrote:
> This looks scary on the surface, but I mixed up this comment in the previous
> "Remove SpdyStream::DoLoop()" CL. Prior to that, Fred had comments on the
calls
> to delegate_->OnRequestHeadersSent() and delegate_->OnDataSent(), pointing out
> that they _couldn't_ destroy |this|.

Makes sense.  I wonder if we could have a member variable like:
  bool destruction_allowed_
which defaults to true, and which we set to false around this block of code. 
Then the destructor could CHECK(destruction_allowed_) to ensure that we don't
screw this up.  hopefully a use-after-free would be detected, so perhaps this is
not necessary?

[Johnny, 2014-02-14 16:27:30]

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/129543002/diff/60001/net/spdy/spdy_stream.cc#...
net/spdy/spdy_stream.cc:545: // Notify delegate of write completion. Must not
destroy |this|.
Sure. I've added a |weak_this| guard instead. Same effect without the new
member.

On 2014/02/14 01:47:22, Ryan Hamilton wrote:
> On 2014/02/14 00:07:08, Johnny wrote:
> > This looks scary on the surface, but I mixed up this comment in the previous
> > "Remove SpdyStream::DoLoop()" CL. Prior to that, Fred had comments on the
> calls
> > to delegate_->OnRequestHeadersSent() and delegate_->OnDataSent(), pointing
out
> > that they _couldn't_ destroy |this|.
> 
> Makes sense.  I wonder if we could have a member variable like:
>   bool destruction_allowed_
> which defaults to true, and which we set to false around this block of code. 
> Then the destructor could CHECK(destruction_allowed_) to ensure that we don't
> screw this up.  hopefully a use-after-free would be detected, so perhaps this
is
> not necessary?

[Johnny, 2014-02-14 16:28:23]

The CQ bit was checked by jgraettinger@chromium.org

[commit-bot: I haz the power, 2014-02-14 16:29:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgraettinger@chromium.org/129543002/18...

[Ryan Hamilton, 2014-02-14 17:23:11]

On Fri, Feb 14, 2014 at 8:27 AM, <jgraettinger@chromium.org> wrote:

>
> https://codereview.chromium.org/129543002/diff/60001/net/
> spdy/spdy_stream.cc
> File net/spdy/spdy_stream.cc (right):
>
> https://codereview.chromium.org/129543002/diff/60001/net/
> spdy/spdy_stream.cc#newcode545
> net/spdy/spdy_stream.cc:545: // Notify delegate of write completion.
> Must not destroy |this|.
> Sure. I've added a |weak_this| guard instead. Same effect without the
> new member.


​Perfect.​

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-02-14 18:49:04]

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

[commit-bot: I haz the power, 2014-02-14 18:49:05]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Johnny, 2014-02-14 20:15:44]

The CQ bit was checked by jgraettinger@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:17:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgraettinger@chromium.org/129543002/18...

[commit-bot: I haz the power, 2014-02-15 00:16:12]

Change committed as 251450