Net: Stop treating partial HTTP headers as a valid response.

Net:  Stop treating partial HTTP headers as a valid response.

For now, we return ERR_RESPONSE_HEADERS_TRUNCATED in this case.  Longer
term, we probably want to get rid of the extra code to detect when this
happens.

Also do a bit of cleanup in HttpStreamParser's error handling code.

BUG=472762
Committed: https://crrev.com/854d9c86cc897fdf55ea917d2aed2070e9235dac
Cr-Commit-Position: refs/heads/master@{#341744}

[mmenke, 2015-07-31 18:11:47]

Patchset #4 (id:60001) has been deleted

[mmenke, 2015-07-31 19:10:59]

mmenke@chromium.org changed reviewers:
+ davidben@chromium.org

[mmenke, 2015-07-31 19:11:00]

Should we have a pool for how many bug reports this gets us, where people
threaten to switch browsers?

https://codereview.chromium.org/1266713007/diff/100001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1266713007/diff/100001/net/http/http_stream_p...
net/http/http_stream_parser.cc:795: return ERR_RESPONSE_HEADERS_TRUNCATED;
This code, in the HTTP case, is the only behavior that should change. 
Everything else is just a refactor, as the old code was rather silly.

[davidben, 2015-07-31 21:32:50]

lgtm

https://codereview.chromium.org/1266713007/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_service_worker_browsertest.cc (left):

https://codereview.chromium.org/1266713007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_service_worker_browsertest.cc:83: "HTTP/1.1 200
OK\nContent-Type: text/javascript");
Ick. Why didn't they check in these files?

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:1230: for (const TestCase&
test_case : kTestCases) {
SCOPED_TRACE(test_case.response)?

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
net/http/http_stream_parser.cc:774: }
If you want, you could further simplify checks by moving this block underneath
the ERR_CONNECTION_CLOSED codepath.

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
File net/http/http_stream_parser_unittest.cc (left):

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
net/http/http_stream_parser_unittest.cc:611: NUM_PROTOCOLS,
[Confirmed there are no other interesting HTTP vs HTTPS distinctions in the
parser.]

[mmenke, 2015-07-31 21:39:25]

Patchset #7 (id:140001) has been deleted

[mmenke, 2015-07-31 21:45:35]

mmenke@chromium.org changed reviewers:
+ thestig@chromium.org

[mmenke, 2015-07-31 21:45:36]

[+thestig]:  Please review chrome/browser/chrome_service_worker_browsertest.cc
changes.  They were accidentally relying on our parsing of partial HTTP
responses.

Changes are pretty TBR-able, but I'm always reluctant to do that, and I've found
people generally look at TBRs, anyways.

https://codereview.chromium.org/1266713007/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_service_worker_browsertest.cc (left):

https://codereview.chromium.org/1266713007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_service_worker_browsertest.cc:83: "HTTP/1.1 200
OK\nContent-Type: text/javascript");
On 2015/07/31 21:32:50, David Benjamin wrote:
> Ick. Why didn't they check in these files?

I have no idea.

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:1230: for (const TestCase&
test_case : kTestCases) {
On 2015/07/31 21:32:50, David Benjamin wrote:
> SCOPED_TRACE(test_case.response)?

Done.

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1266713007/diff/120001/net/http/http_stream_p...
net/http/http_stream_parser.cc:774: }
On 2015/07/31 21:32:50, David Benjamin wrote:
> If you want, you could further simplify checks by moving this block underneath
> the ERR_CONNECTION_CLOSED codepath.

Good idea, done.

[Lei Zhang, 2015-07-31 21:54:03]

thestig@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Lei Zhang, 2015-07-31 21:54:04]

jyasskin@ wrote the test, so lgtm if he's ok with it. Looks ok to me.

[mmenke, 2015-08-03 15:20:08]

On 2015/07/31 21:54:04, Lei Zhang wrote:
> jyasskin@ wrote the test, so lgtm if he's ok with it. Looks ok to me.

jyasskin:  PTAL.

A little more context:  The test server expects mock-header files to include a
terminal line break.  Not having it meant the server was sending headers without
the double CRLF.  Switched to HTTP/1.0 as well, since the test server doesn't
support keep-alive, and to prevent any problems with net/ trying to reuse
sockets.

[Jeffrey Yasskin, 2015-08-04 15:57:37]

LGTM

Would checking in the .mock-http-headers files have made this change easier to
read or write? I put them in the test so people reading the test didn't have to
go find them in order to understand what the test was doing.

[mmenke, 2015-08-04 16:01:24]

On 2015/08/04 15:57:37, Jeffrey Yasskin wrote:
> LGTM
> 
> Would checking in the .mock-http-headers files have made this change easier to
> read or write? I put them in the test so people reading the test didn't have
to
> go find them in order to understand what the test was doing.

It would have meant no fooling with scoped temp files and the like, but I don't
think it would have made the tests simpler, otherwise.  More an issue of
preferred style.

[mmenke, 2015-08-04 16:01:41]

Thanks for the reviews!

[mmenke, 2015-08-04 16:01:44]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-08-04 16:01:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1266713007/#ps160001
(title: "Response to comments")

[commit-bot: I haz the power, 2015-08-04 16:02:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1266713007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1266713007/160001

[commit-bot: I haz the power, 2015-08-04 17:04:07]

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

[commit-bot: I haz the power, 2015-08-04 17:04:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mmenke, 2015-08-04 17:07:13]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-08-04 17:07:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1266713007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1266713007/160001

[commit-bot: I haz the power, 2015-08-04 17:44:02]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-08-04 17:44:51]

Patchset 7 (id:??) landed as
https://crrev.com/854d9c86cc897fdf55ea917d2aed2070e9235dac
Cr-Commit-Position: refs/heads/master@{#341744}

[Zhenyao Mo, 2015-08-04 22:07:46]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1255123006/ by zmo@chromium.org.

The reason for reverting is: Might broke
http/tests/security/XFrameOptions/x-frame-options-cached.html
http/tests/security/contentSecurityPolicy/cached-frame-csp.html
http/tests/security/xssAuditor/cached-frame.html on windows blink bots.