QuicChromiumClientStream should only do OnStream after notifying delegate about trailers

QuicChromiumClientStream should only do OnStream after notifying delegate about trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through 
QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), 
which can led to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=586207

[xunjieli, 2016-03-08 02:52:18]

Description was changed from

==========
QuicChromiumClientStream should do OnStream after notifying delegate about
trailers

OnStream does delegate_->OnDataAvailable to force delegate to read FIN in
 QuicStreamSequencer::MaybeCloseStream. When trailers are received, we should
 only force delegate to read FIN only after delegate is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream does delegate_->OnDataAvailable to force delegate to read FIN in
 QuicStreamSequencer::MaybeCloseStream. When trailers are received, we should
 only force delegate to read FIN only after delegate is notified of trailers.

BUG=584338
==========

[xunjieli, 2016-03-08 02:54:09]

Description was changed from

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream does delegate_->OnDataAvailable to force delegate to read FIN in
 QuicStreamSequencer::MaybeCloseStream. When trailers are received, we should
 only force delegate to read FIN only after delegate is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since client will receive OnDataAvailable
callback before OnHeadersReceived (for trailers).
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

[xunjieli, 2016-03-08 02:54:37]

Description was changed from

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since client will receive OnDataAvailable
callback before OnHeadersReceived (for trailers).
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since client will receive OnDataAvailable
callback before OnHeadersReceived (for trailers).
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

[xunjieli, 2016-03-08 02:55:39]

Description was changed from

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since client will receive OnDataAvailable
callback before OnHeadersReceived (for trailers).
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), which can led
to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

[xunjieli, 2016-03-08 02:55:54]

Description was changed from

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), which can led
to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through 
QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), 
which can led to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

[xunjieli, 2016-03-08 02:57:16]

Description was changed from

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through 
QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), 
which can led to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=584338
==========

to

==========
QuicChromiumClientStream should only do OnStream after notifying delegate about
trailers

OnStream with a FIN flag indirectly calls delegate_->OnDataAvailable  
to force delegate to read FIN through 
QuicStreamSequencer::MaybeCloseStream. 
However this is not desirable, since delegate will receive 
OnDataAvailable callback before OnHeadersReceived (for trailers), 
which can led to delegate missing the trailers received event.
Therefore, we should only force delegate to read FIN only after delegate 
is notified of trailers.

BUG=586207
==========

[xunjieli, 2016-03-08 02:59:09]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-03-08 03:13:20]

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

[xunjieli, 2016-03-08 03:13:20]

Ryan, PTAL. My previous CL isn't quite right. I didn't realize that OnStream
with a FIN flag will call back into delegate as well (through OnDataAvailable).
In this case, we should delay OnStream after we notify delegate of trailers,
otherwise delegate will miss the trailers event since it will assume that
there's nothing after FIN flag.

Robbie (cc-ed), FYI. Please help me do a sanity check if you have time.

[Ryan Hamilton, 2016-03-08 04:12:43]

https://codereview.chromium.org/1774703003/diff/20001/net/quic/quic_spdy_stre...
File net/quic/quic_spdy_stream.cc (left):

https://codereview.chromium.org/1774703003/diff/20001/net/quic/quic_spdy_stre...
net/quic/quic_spdy_stream.cc:216: OnStreamFrame(QuicStreamFrame(id(), fin,
stream_bytes_read(), StringPiece()));
I thought that you and robbie agreed that this was the right place for this
change?

Did you ever merge this back to google3?

[xunjieli, 2016-03-08 14:46:02]

Thanks, PTAL

https://codereview.chromium.org/1774703003/diff/20001/net/quic/quic_spdy_stre...
File net/quic/quic_spdy_stream.cc (left):

https://codereview.chromium.org/1774703003/diff/20001/net/quic/quic_spdy_stre...
net/quic/quic_spdy_stream.cc:216: OnStreamFrame(QuicStreamFrame(id(), fin,
stream_bytes_read(), StringPiece()));
On 2016/03/08 04:12:43, Ryan Hamilton wrote:
> I thought that you and robbie agreed that this was the right place for this
> change?
> 
> Did you ever merge this back to google3?

Robbie and I agreed that we need to add a OnStreamFrame. However, if we put
OnStreamFrame here, it will call OnDataAvailable with Fin flag before we notify
delegate of trailers. This isn't correct, because once delegate consumes FIN
flag, it thinks that everything is done. (The code is fine for initial headers,
because there's some logic to block the sequencer, so we delay OnDataAvailable
until the initial headers). 

This is discovered when I did more testing.

Sorry, I thought you'd help me with merging with Google3, so I didn't do
anything. Could you help me?

[xunjieli, 2016-03-09 20:40:06]

I found a less invasive way to achieve this by simply delaying
MarkTrailersConsumed until we are about to notify delegate not when the task is
posted. Closing this CL now.