Make HRH::IsKeepAlive() look past the first header

Make HRH::IsKeepAlive() look past the first header

Up until now, HttpResponseHeaders::IsKeepAlive() only looked at the
first value of the Connection header. Since for a WebSocket response a
header like

Connection: Upgrade, close

is valid we should check other values past the first value.

BUG=371759
TEST=net_unittests

Committed: https://crrev.com/5db2f4ea9ac85ef4c8221c533e61b26b69c15ab7
Cr-Commit-Position: refs/heads/master@{#300087}

[Adam Rice, 2014-10-09 10:56:58]

ricea@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2014-10-15 22:55:22]

lgtm after nits.

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
net/http/http_response_headers.cc:1187: for (const char* header :
kConnectionHeaders) {
nit: We need a comment somewhere stating that we grab the first value that we
recognize and go with it, ignoring anything after that (I don't know if that
behavior is defined anywhere)

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
net/http/http_response_headers_unittest.cc:1586: void PrintTo(const
KeepAliveTestData& keep_alive_test_data,
nit: add a comment about this being gtest related.

[Adam Rice, 2014-10-16 11:29:39]

I re-read RFC 7230 section 6.1 http://tools.ietf.org/html/rfc7230#section-6.1
and thought about it a bit more and I'm uncomfortable with this change.

The old behaviour was unintentionally non-standard and strange. The new
behaviour is intentionally non-standard and strange.

Is there an additional person with good knowledge of cache weirdness you could
get to take a look at this?

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
net/http/http_response_headers.cc:1187: for (const char* header :
kConnectionHeaders) {
On 2014/10/15 22:55:21, rvargas wrote:
> nit: We need a comment somewhere stating that we grab the first value that we
> recognize and go with it, ignoring anything after that (I don't know if that
> behavior is defined anywhere)

Done.

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/640213002/diff/1/net/http/http_response_heade...
net/http/http_response_headers_unittest.cc:1586: void PrintTo(const
KeepAliveTestData& keep_alive_test_data,
On 2014/10/15 22:55:21, rvargas wrote:
> nit: add a comment about this being gtest related.

Done.

[rvargas (doing something else), 2014-10-16 18:11:12]

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

[rvargas (doing something else), 2014-10-16 18:11:13]

On 2014/10/16 11:29:39, Adam Rice wrote:
> I re-read RFC 7230 section 6.1 http://tools.ietf.org/html/rfc7230#section-6.1
> and thought about it a bit more and I'm uncomfortable with this change.
> 
> The old behaviour was unintentionally non-standard and strange. The new
> behaviour is intentionally non-standard and strange.
> 
> Is there an additional person with good knowledge of cache weirdness you could
> get to take a look at this?
> 

I spent some time yesterday reading different parts of httpbis RFCs and didn't
find a clear description of what would be the correct behavior (hence the nit
for a comment). On the other hand, the change seems to make our current behavior
more consistent (even though we actually acknowledge it is not really standard).

I'm adding Matt since he suggested the change in the first place. Maybe willchan
has an opinion here, but he's on leave... we could always go to Darin.

[mmenke, 2014-10-16 19:29:39]

On 2014/10/16 18:11:13, rvargas wrote:
> On 2014/10/16 11:29:39, Adam Rice wrote:
> > I re-read RFC 7230 section 6.1
http://tools.ietf.org/html/rfc7230#section-6.1
> > and thought about it a bit more and I'm uncomfortable with this change.
> > 
> > The old behaviour was unintentionally non-standard and strange. The new
> > behaviour is intentionally non-standard and strange.
> > 
> > Is there an additional person with good knowledge of cache weirdness you
could
> > get to take a look at this?
> > 
> 
> I spent some time yesterday reading different parts of httpbis RFCs and didn't
> find a clear description of what would be the correct behavior (hence the nit
> for a comment). On the other hand, the change seems to make our current
behavior
> more consistent (even though we actually acknowledge it is not really
standard).
> 
> I'm adding Matt since he suggested the change in the first place. Maybe
willchan
> has an opinion here, but he's on leave... we could always go to Darin.

My general feeling is that the spec allows for multiple connection headers so,
we should handle them (Admittedly,they are kinda weird and pointless in the
websockets case, but better to support them now, then when some other connection
value is added and only fix that when we run into bugs).

The weird cases are when we get contradictory values, either in the same header,
or in the two headers - do we ignore the one that's not applicable to the
current protocol, or obey it, and if the latter, in what order do we obey them. 
And when we get a header that is the default for the current protocol, do we
ignore it, or not?

I'm fine with the choice this code takes, I'm also fine with ignoring the one
with no meaning for the protocol in use...Or we could just always treat it as
Connection: close when we're confused.  I really don't have a preference here,
though should definitely not just fail the request.  I think in all the weird
cases, the new behavior is not clearly better or worse.

[Adam Rice, 2014-10-17 09:55:38]

On 2014/10/16 19:29:39, mmenke wrote:
> On 2014/10/16 18:11:13, rvargas wrote:
> > I spent some time yesterday reading different parts of httpbis RFCs and
didn't
> > find a clear description of what would be the correct behavior (hence the
nit
> > for a comment). On the other hand, the change seems to make our current
> behavior
> > more consistent (even though we actually acknowledge it is not really
> standard).
> > 
> > I'm adding Matt since he suggested the change in the first place. Maybe
> willchan
> > has an opinion here, but he's on leave... we could always go to Darin.
> 
> My general feeling is that the spec allows for multiple connection headers so,
> we should handle them (Admittedly,they are kinda weird and pointless in the
> websockets case, but better to support them now, then when some other
connection
> value is added and only fix that when we run into bugs).
> 
> The weird cases are when we get contradictory values, either in the same
header,
> or in the two headers - do we ignore the one that's not applicable to the
> current protocol, or obey it, and if the latter, in what order do we obey
them. 
> And when we get a header that is the default for the current protocol, do we
> ignore it, or not?
> 
> I'm fine with the choice this code takes, I'm also fine with ignoring the one
> with no meaning for the protocol in use...Or we could just always treat it as
> Connection: close when we're confused.  I really don't have a preference here,
> though should definitely not just fail the request.  I think in all the weird
> cases, the new behavior is not clearly better or worse.

Thank you for your feedback. I have regained the confidence to submit.

Bonus url: curl -I mezo.me

It's like they foresaw my unit tests.

[Adam Rice, 2014-10-17 09:55:50]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-10-17 09:56:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640213002/20001

[commit-bot: I haz the power, 2014-10-17 10:49:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-17 10:50:19]

Patchset 2 (id:??) landed as
https://crrev.com/5db2f4ea9ac85ef4c8221c533e61b26b69c15ab7
Cr-Commit-Position: refs/heads/master@{#300087}