SPDY - Handle incomplete headers during server push.

SPDY - Handle incomplete headers during server push.

If we receive a data frame without a status or without
a version header, we close the stream with a PROTOCOL ERROR.

Small bug fix to HttpNetworkTransaction to access the
ResponseHeaders only if headers are there.

In SpdyStream, retrun a SPDY_PROTOCOL_ERROR if we have pending
data frames, but we haven't received "status" and "version"
headers. (rch: removed the DCHECK for unit tests).

SpdyHttpStream's OnDataReceived used to expect that it would
be called only when all required headers are received. Converted
the DCHECK into an error condition and SpdyStream closes the
stream with PROTOCOL ERROR if OnDataReceived returns a network
error.

BUG=135485
R=rch@chromium.org
TEST=network unittests

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

[ramant (doing other things), 2012-08-03 00:08:30]

Hi Ryan,
  Could you please take a look at this change we had discussed earlier?

thanks much
raman

[Ryan Hamilton, 2012-08-03 19:52:14]

One meta-comment.  This approach is fine, but if you decided you wanted to make
the CL smaller, I think you could remove the changes to the OnDataReceived
function, and add back the DCHECK() in spdy_htp_stream.cc.  It seems like a lot
of changes to deal with a case that you believe should be impossible.  That
being said, I don't object to it.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
net/http/http_network_transaction.cc:264: if (headers && headers->IsKeepAlive()
&&
Is this change related to the SPDY server push bug?  If not, I think it should
be split into a new CL.

Either way, it would be great if there were a test.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
net/http/http_network_transaction.cc:925: HttpResponseHeaders* headers =
GetResponseHeaders();
(Ditto)

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_netwo...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_netwo...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5133:
TEST_P(SpdyNetworkTransactionSpdy2Test, ServerPushWithNoStatusHeaderFrames) {
I want to confirm that if you run this test with the old code, we see a crash.
(DCHECK).  Is that right?

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
File net/spdy/spdy_stream.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:498: // We haven't received all the required headers.
This comment assumes something about the implementation of the delegate that I'm
not sure is valid to assume.  I would simply say some about "the delegate
rejected the data".

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:500: "Data is received but incomplete headers.");
Same point as above for the error description.

[ramant (doing other things), 2012-08-03 20:50:40]

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
net/http/http_network_transaction.cc:264: if (headers && headers->IsKeepAlive()
&&
On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> Is this change related to the SPDY server push bug?  If not, I think it should
> be split into a new CL.
> 
> Either way, it would be great if there were a test.

Deleted this change.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
net/http/http_network_transaction.cc:925: HttpResponseHeaders* headers =
GetResponseHeaders();
On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> (Ditto)

SpdyNetworkTransactionSpdy3Test..ServerPushWithNoStatusHeaderFrames causes this
crash. It looks like CanFindEndOfResponse is being true though there are no
response headers. That may be another bug. Will undo this change when we fix it.
Added a TODO. Will submit a separate CL for unit tests.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_netwo...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_netwo...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5133:
TEST_P(SpdyNetworkTransactionSpdy2Test, ServerPushWithNoStatusHeaderFrames) {
On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> I want to confirm that if you run this test with the old code, we see a crash.
> (DCHECK).  Is that right?

Yes it does. It crashes the code.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
File net/spdy/spdy_stream.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:498: // We haven't received all the required headers.
On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> This comment assumes something about the implementation of the delegate that
I'm
> not sure is valid to assume.  I would simply say some about "the delegate
> rejected the data".

Done.

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:500: "Data is received but incomplete headers.");
On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> Same point as above for the error description.

Done.

[Ryan Hamilton, 2012-08-03 21:10:01]

lgtm

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
File net/http/http_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
net/http/http_network_transaction.cc:925: HttpResponseHeaders* headers =
GetResponseHeaders();
On 2012/08/03 20:50:40, ramant wrote:
> On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> > (Ditto)
> 
> SpdyNetworkTransactionSpdy3Test..ServerPushWithNoStatusHeaderFrames causes
this
> crash. It looks like CanFindEndOfResponse is being true though there are no
> response headers. That may be another bug. Will undo this change when we fix
it.
> Added a TODO. Will submit a separate CL for unit tests.

Does that test cause a crash with your new code, or with the new code.  If your
new test is crashing here with the new code, then I agree with you that this
change is required as part of this CL.

[ramant (doing other things), 2012-08-06 16:51:14]

On 2012/08/03 21:10:01, Ryan Hamilton wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
> File net/http/http_network_transaction.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_netwo...
> net/http/http_network_transaction.cc:925: HttpResponseHeaders* headers =
> GetResponseHeaders();
> On 2012/08/03 20:50:40, ramant wrote:
> > On 2012/08/03 19:52:14, Ryan Hamilton wrote:
> > > (Ditto)
> > 
> > SpdyNetworkTransactionSpdy3Test..ServerPushWithNoStatusHeaderFrames causes
> this
> > crash. It looks like CanFindEndOfResponse is being true though there are no
> > response headers. That may be another bug. Will undo this change when we fix
> it.
> > Added a TODO. Will submit a separate CL for unit tests.
> 
> Does that test cause a crash with your new code, or with the new code.  If
your
> new test is crashing here with the new code, then I agree with you that this
> change is required as part of this CL.

It was crashing with the new code and the new test. 

I was thinking this could have been the reason, how we would have accessed NULL
pointer object, but wasn't able to reproduce it so far on windows with the Dev
build.

[commit-bot: I haz the power, 2012-08-06 16:52:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10836084/15001

[rtenneti, 2012-08-06 17:05:14]

On 2012/08/06 16:52:00, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/rtenneti%40chromium.org/10836084/15001

I was able to duplicate the bug in M21 on Linux. crash id is 7a6eaef1ed3585ff. 

It crashes in 	 -
net/http/http_network_transaction.cc:860]	net::HttpNetworkTransaction::DoReadHeadersComplete

[commit-bot: I haz the power, 2012-08-06 17:07:32]

Try job failure for 10836084-15001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 17:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10836084/15001

[commit-bot: I haz the power, 2012-08-06 18:53:49]

Change committed as 150121