SPDY: Fix DeleteStream() being called twice and CloseStream() reporting a wrong stream number.

Fix DeleteStream() being called twice per stream and CloseStream() reporting a wrong stream number.

BUG=49683
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53784

[agayev, 2010-07-21 16:44:04]

This one should be pretty easy to review.

[cbentzel, 2010-07-21 17:34:46]

Are there any unit tests which would break on the old behavior but work
correctly with the new behavior?

http://codereview.chromium.org/3035015/diff/1/2
File net/spdy/spdy_stream.cc (left):

http://codereview.chromium.org/3035015/diff/1/2#oldcode197
net/spdy/spdy_stream.cc:197: stream_id_ = 0;
Are there other parts of the code base which check stream_id_ == 0 to determine
if closed (I seem to recall this happening)

This is probably a good change, but I'm worried about what else breaks.

[erikchen, 2010-07-21 18:45:31]

http://codereview.chromium.org/3035015/diff/1/2
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3035015/diff/1/2#newcode35
net/spdy/spdy_stream.cc:35: DCHECK(response_complete_);
This looks like it will fail if a SpdyStream is constructed, and immediately
destructed.

[agayev, 2010-07-21 19:04:03]

http://codereview.chromium.org/3035015/diff/1/2
File net/spdy/spdy_stream.cc (left):

http://codereview.chromium.org/3035015/diff/1/2#oldcode197
net/spdy/spdy_stream.cc:197: stream_id_ = 0;
On 2010/07/21 17:34:46, cbentzel wrote:
> Are there other parts of the code base which check stream_id_ == 0 to
determine
> if closed (I seem to recall this happening)
> 
> This is probably a good change, but I'm worried about what else breaks.

The only reference to it was in ~SpdyStream above, which didn't make much sense,
since response_complete_ is set to true the moment stream_id_ is set to 0,
therefore, DCHECK would always pass.

http://codereview.chromium.org/3035015/diff/1/2
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3035015/diff/1/2#newcode35
net/spdy/spdy_stream.cc:35: DCHECK(response_complete_);
On 2010/07/21 18:45:32, erikchen1 wrote:
> This looks like it will fail if a SpdyStream is constructed, and immediately
> destructed.

Is this likely?  Shouldn't OnClose() be called on stream before it is destroyed?
 Also, it seems your scenario was possible with the old check as well, since
push streams are generated with stream id of 0 and updated later, so if those
were cancelled, before push stream arrived...  

I should probably get rid of DCHECK or add another flag just for this purpose.