Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(50)

Issue 10836084: SPDY - Handle incomplete headers during server push. (Closed)

Created:
7 years ago by ramant (doing other things)
Modified:
7 years ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Total comments: 11

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+291 lines, -18 lines) Patch
M net/http/http_network_transaction.cc View 1 2 1 chunk +7 lines, -2 lines 0 comments Download
M net/spdy/spdy_http_stream.h View 1 1 chunk +1 line, -1 line 0 comments Download
M net/spdy/spdy_http_stream.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download
M net/spdy/spdy_network_transaction_spdy2_unittest.cc View 1 1 chunk +123 lines, -0 lines 0 comments Download
M net/spdy/spdy_network_transaction_spdy3_unittest.cc View 1 1 chunk +124 lines, -0 lines 0 comments Download
M net/spdy/spdy_proxy_client_socket.h View 1 1 chunk +1 line, -1 line 0 comments Download
M net/spdy/spdy_proxy_client_socket.cc View 1 2 chunks +2 lines, -1 line 0 comments Download
M net/spdy/spdy_session_spdy2_unittest.cc View 1 2 chunks +5 lines, -2 lines 0 comments Download
M net/spdy/spdy_session_spdy3_unittest.cc View 1 2 chunks +5 lines, -2 lines 0 comments Download
M net/spdy/spdy_stream.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M net/spdy/spdy_stream.cc View 1 2 2 chunks +11 lines, -2 lines 0 comments Download
M net/spdy/spdy_stream_test_util.h View 1 1 chunk +1 line, -1 line 0 comments Download
M net/spdy/spdy_stream_test_util.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M net/spdy/spdy_websocket_stream.h View 1 1 chunk +1 line, -1 line 0 comments Download
M net/spdy/spdy_websocket_stream.cc View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 10 (0 generated)
ramant (doing other things)
Hi Ryan, Could you please take a look at this change we had discussed earlier? ...
7 years ago (2012-08-03 00:08:30 UTC) #1
Ryan Hamilton
One meta-comment. This approach is fine, but if you decided you wanted to make the ...
7 years ago (2012-08-03 19:52:14 UTC) #2
ramant (doing other things)
https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_network_transaction.cc#newcode264 net/http/http_network_transaction.cc:264: if (headers && headers->IsKeepAlive() && On 2012/08/03 19:52:14, Ryan ...
7 years ago (2012-08-03 20:50:40 UTC) #3
Ryan Hamilton
lgtm https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_network_transaction.cc#newcode925 net/http/http_network_transaction.cc:925: HttpResponseHeaders* headers = GetResponseHeaders(); On 2012/08/03 20:50:40, ramant ...
7 years ago (2012-08-03 21:10:01 UTC) #4
ramant (doing other things)
On 2012/08/03 21:10:01, Ryan Hamilton wrote: > lgtm > > https://chromiumcodereview.appspot.com/10836084/diff/3004/net/http/http_network_transaction.cc > File net/http/http_network_transaction.cc (right): ...
7 years ago (2012-08-06 16:51:14 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10836084/15001
7 years ago (2012-08-06 16:52:00 UTC) #6
rtenneti
On 2012/08/06 16:52:00, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
7 years ago (2012-08-06 17:05:14 UTC) #7
commit-bot: I haz the power
Try job failure for 10836084-15001 (retry) on win_rel for step "compile" (clobber build). It's a ...
7 years ago (2012-08-06 17:07:32 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10836084/15001
7 years ago (2012-08-06 17:19:20 UTC) #9
commit-bot: I haz the power
7 years ago (2012-08-06 18:53:49 UTC) #10
Change committed as 150121

Powered by Google App Engine
This is Rietveld 408576698