Refactor pooling logic into a helper method

Refactor pooling logic into a helper method
Disable pooling when there are cert errors.
Disable pooling when pinning does not match for the new host.

BUG=398925
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289433

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290320

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290497

[Ryan Hamilton, 2014-08-07 17:05:52]

You don't need to review this now since my other CL still needs work, but this
should provide a good sense of how I'm envisioning fixing this.

[Ryan Sleevi, 2014-08-07 18:49:29]

Drive by!

Two high-level comments:
Test wise, you should have a test for each of the conditionals you're testing in
the CanPool logic. It seems that some are missing

Structure wise, it feels weird/wrong to put this in HttpUtil, because it's a
SPDY/QUIC concept, not an HTTP one. Is there a 'higher' shared abstraction you
can put this under?

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:758: return false;
You should check this on 771

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:777: /* pinning_failure_log= */ NULL)) {
1) git-cl-format this (you should be four *additional* spaces indented)
2) It seems like the style from
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
should apply here , which matches a good bit of Chromium

That is
true, /* sni_available */
NULL, /* pinning_failure_log */

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util_unitt...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util_unitt...
net/http/http_util_unittest.cc:1134: TEST(HttpUtilTest,
CanNotPoolAcrossETLDsWithChannelID) {
Need pinning test

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: return
HttpUtil::CanPool(transport_security_state_, ssl_info,
This seems weird to be called an HTTP util. This isn't really an HTTP-level
concept, but seems like it's more tied with the QUIC/SPDY session pools / client
socket pool?

For example, one way I see this sort of backfiring is with respect to how group
names are treated (for example, privacy mode vs non-privacy mode), and the
caller being confused as to whether/how this treats them.

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:201: TEST_P(QuicClientSessionTest,
ConnectionPooledWithTlsChannelId) {
Pinning tests

[Ryan Hamilton, 2014-08-08 19:27:44]

Thanks for the comments! The CL wasn't yet ready for review (since I hadn't yet
figure out how to test pinning because of the #ifdefs) but I'm happy for the
feedback.  I think I've addressed your main points with two caveats:

1) I'll write a QUIC-level test but haven't done so yet.
2) I'm not sure where this should live. As a strawman, I moved it to SpdySession
which is *plausible* but I'm totally open to a better location if you have one.

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:758: return false;
On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> You should check this on 771

Done. (But how come?)

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:777: /* pinning_failure_log= */ NULL)) {
On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> 1) git-cl-format this (you should be four *additional* spaces indented)

Done.

> 2) It seems like the style from
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> should apply here ,

I don't think that part of the style guide applies here, because that is simple
commenting out the name of a parameter in the location it would otherwise
appear. I think the part of the style guide which is more on point is here,
right?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

> which matches a good bit of Chromium
> 
> That is
> true, /* sni_available */
> NULL, /* pinning_failure_log */

I'm ok switching to this (or perhaps to "// SNI is available") but I don't love
it and I'm curious what you think. The trouble comes with:

  false, /* is_server */

When I read that, I'm always confused if that is saying that is_server is false,
or if it is saying that false means that this is a server. (This is particularly
trick if the term is something like "success"). That's why I prefer the style
with the =.

That being said, when in Rome...

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util_unitt...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util_unitt...
net/http/http_util_unittest.cc:1134: TEST(HttpUtilTest,
CanNotPoolAcrossETLDsWithChannelID) {
On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> Need pinning test

Right, but I couldn't write such a test until we got to the bottom of the
mechanism we could use which would allow pinning to be checked in a test :>

Done now.

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: return
HttpUtil::CanPool(transport_security_state_, ssl_info,
On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> This seems weird to be called an HTTP util. This isn't really an HTTP-level
> concept, but seems like it's more tied with the QUIC/SPDY session pools /
client
> socket pool?

*nod* I've stuffed it into spdy_session.h for now, but happy to put it somewhere
if you have a better idea.

> For example, one way I see this sort of backfiring is with respect to how
group
> names are treated (for example, privacy mode vs non-privacy mode), and the
> caller being confused as to whether/how this treats them.

Hm. I'm not sure. There are two steps that, say, the SpdySessionPool has to
undertake in order to send an existing request to an existing session. First it
has to (as you suggest) identify a set of possible candidate session based on
things like IP overlap, privacy mode, secure vs insecure.  Only after a
candidate session has been identified does it perform the this final validation
step on the session. I don't think we can fold the "identification" step into
common code because SpdySessionPool and QuicStreamFactor represent these things
quite differently. I'm totally happy to move this method anywhere you think it
should go; I'm just not sold on folding in privacy mode and friends into it. 

What do you think?

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:201: TEST_P(QuicClientSessionTest,
ConnectionPooledWithTlsChannelId) {
On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> Pinning tests

I would have thought that the new tests written as part of this CL would be
sufficient that we would not need to re-test the logic here at the QUIC level?
I'll work on this now.

[Ryan Hamilton, 2014-08-08 19:52:40]

Added a QUIC pooling + pinning test

[Ryan Sleevi, 2014-08-11 18:45:18]

Mostly nits, except one question on IsCertStatusMinorError

I think this LGTM, but it'd be good if you can answer that question. If you
don't hear back from me right away (or ping me on gchat), feel free to go ahead
and CQ.

https://codereview.chromium.org/425803014/diff/60001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:16: #include "net/http/http_util.h"
No longer needed

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:542: if (IsCertStatusError(ssl_info.cert_status))
Should this be

if (IsCertStatusError(ssl_info.cert_status) &&
!IsCertStatusMinorError(ssl_info.cert_status)) ?


Why or why not?

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session_un...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session_un...
net/spdy/spdy_session_unittest.cc:5075: TEST(CanPoolTest,
CanNotPoolWithDifferentPins) {
This test isn't quite correct, is it?

You're not checking that the pins are identical - just that the pins are
acceptable.

CanNotPoolWithBadPins ? (or Unacceptable)

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.cc
File net/spdy/spdy_test_utils.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.cc:145: HashValue hash_value(HASH_VALUE_SHA1);
Use SHA-256 for these.

SHA-1 is deprecated in HPKP draft, we'll be cleaning them up, best to avoid the
issue now.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.h
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:10: #include "net/base/hash_value.h"
Can forward declare this.

See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

"Forward OrReturnValue()"

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:11: #include "net/http/transport_security_state.h"
Can forward declare this - only used as a pointer.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:39: HashValue GetTestHashValue(uint8 label);
1) These should be using uint8_t, not uint8, in SPDY land.
2) IWYU: You should be including <stdint.h> (uint8_t) OR "base/basictypes.h"
(uint8), rather than relying on spdy_protocol.h to do so.

[Ryan Sleevi, 2014-08-11 19:03:43]

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:758: return false;
On 2014/08/08 19:27:43, Ryan Hamilton wrote:
> On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> > You should check this on 771
> 
> Done. (But how come?)

Forgot to answer this - verify the cert is trusted/trustworthy before acting
upon/processing the parsed data. Avoid hijinks from hostile certs :)

[Ryan Sleevi, 2014-08-11 19:09:17]

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: return
HttpUtil::CanPool(transport_security_state_, ssl_info,
On 2014/08/08 19:27:43, Ryan Hamilton wrote:
> Hm. I'm not sure. There are two steps that, say, the SpdySessionPool has to
> undertake in order to send an existing request to an existing session. First
it
> has to (as you suggest) identify a set of possible candidate session based on
> things like IP overlap, privacy mode, secure vs insecure.  Only after a
> candidate session has been identified does it perform the this final
validation
> step on the session. I don't think we can fold the "identification" step into
> common code because SpdySessionPool and QuicStreamFactor represent these
things
> quite differently. I'm totally happy to move this method anywhere you think it
> should go; I'm just not sold on folding in privacy mode and friends into it. 
> 
> What do you think?

Right, I wasn't trying to suggest folding privacy mode into it - merely that
someone would assume in HttpUtil that it did, or that it was safe to use w/o
checking privacy mode.

I think moving it to Spdy is fine. I guess the argument for HttpUtil is that
this is part of the HTTP/2 spec, and so maybe this really is an HTTP utility
now, even though we support SPDY?

Or are our SPDY classes being used for h2 support now?

[Ryan Hamilton, 2014-08-12 14:39:06]

Thanks for the review. One question.

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:758: return false;
On 2014/08/11 19:03:43, Ryan Sleevi wrote:
> On 2014/08/08 19:27:43, Ryan Hamilton wrote:
> > On 2014/08/07 18:49:29, Ryan Sleevi wrote:
> > > You should check this on 771
> > 
> > Done. (But how come?)
> 
> Forgot to answer this - verify the cert is trusted/trustworthy before acting
> upon/processing the parsed data. Avoid hijinks from hostile certs :)

Oh! Good point.

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: return
HttpUtil::CanPool(transport_security_state_, ssl_info,
On 2014/08/11 19:09:17, Ryan Sleevi wrote:
> On 2014/08/08 19:27:43, Ryan Hamilton wrote:
> > Hm. I'm not sure. There are two steps that, say, the SpdySessionPool has to
> > undertake in order to send an existing request to an existing session. First
> it
> > has to (as you suggest) identify a set of possible candidate session based
on
> > things like IP overlap, privacy mode, secure vs insecure.  Only after a
> > candidate session has been identified does it perform the this final
> validation
> > step on the session. I don't think we can fold the "identification" step
into
> > common code because SpdySessionPool and QuicStreamFactor represent these
> things
> > quite differently. I'm totally happy to move this method anywhere you think
it
> > should go; I'm just not sold on folding in privacy mode and friends into it.

> > 
> > What do you think?
> 
> Right, I wasn't trying to suggest folding privacy mode into it - merely that
> someone would assume in HttpUtil that it did, or that it was safe to use w/o
> checking privacy mode.
> 
> I think moving it to Spdy is fine. I guess the argument for HttpUtil is that
> this is part of the HTTP/2 spec, and so maybe this really is an HTTP utility
> now, even though we support SPDY?

Yeah, that's what I was thinking, but I agree that it felt weird.

> Or are our SPDY classes being used for h2 support now?

Indeed SPDY == HTTP2, which makes SPDY seem like a reasonable place.  Thanks!

https://codereview.chromium.org/425803014/diff/60001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:16: #include "net/http/http_util.h"
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> No longer needed

Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:542: if (IsCertStatusError(ssl_info.cert_status))
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> Should this be
> 
> if (IsCertStatusError(ssl_info.cert_status) &&
> !IsCertStatusMinorError(ssl_info.cert_status)) ?
> 
> 
> Why or why not?

No idea :> I'm happy to defer to your judgement because I'm unfamiliar with the
nuances here. We obviously don't want to pool onto a connection that the use
would have to override an interstitial for. If minor errors don't trigger such
user-visible issues, then your suggestion is clearly right. I've made that
change, but I would like confirmation  :>

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session_un...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session_un...
net/spdy/spdy_session_unittest.cc:5075: TEST(CanPoolTest,
CanNotPoolWithDifferentPins) {
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> This test isn't quite correct, is it?
> 
> You're not checking that the pins are identical - just that the pins are
> acceptable.
> 
> CanNotPoolWithBadPins ? (or Unacceptable)

Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.cc
File net/spdy/spdy_test_utils.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.cc:145: HashValue hash_value(HASH_VALUE_SHA1);
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> Use SHA-256 for these.
> 
> SHA-1 is deprecated in HPKP draft, we'll be cleaning them up, best to avoid
the
> issue now.

Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.h
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:10: #include "net/base/hash_value.h"
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> Can forward declare this.
> 
> See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> 
> "Forward OrReturnValue()"

Whoa! You forward declare if it's used as a return value? Wow. I would not have
expected this to be possible, since i would have expected that the size would
need to be known... But I guess not! I guess it needs to be know if it's
*called* but not if it's declared.

Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:11: #include "net/http/transport_security_state.h"
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> Can forward declare this - only used as a pointer.

Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:39: HashValue GetTestHashValue(uint8 label);
On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> 1) These should be using uint8_t, not uint8, in SPDY land.

I'm not sure I follow? We use basictypes all the time in SPDY code. (Both
internally and externally)

> 2) IWYU: You should be including <stdint.h> (uint8_t) OR "base/basictypes.h"
> (uint8), rather than relying on spdy_protocol.h to do so.

Done.

[Ryan Sleevi, 2014-08-12 14:52:03]

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:542: if (IsCertStatusError(ssl_info.cert_status))
On 2014/08/12 14:39:06, Ryan Hamilton wrote:
> On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> > Should this be
> > 
> > if (IsCertStatusError(ssl_info.cert_status) &&
> > !IsCertStatusMinorError(ssl_info.cert_status)) ?
> > 
> > 
> > Why or why not?
> 
> No idea :> I'm happy to defer to your judgement because I'm unfamiliar with
the
> nuances here. We obviously don't want to pool onto a connection that the use
> would have to override an interstitial for. If minor errors don't trigger such
> user-visible issues, then your suggestion is clearly right. I've made that
> change, but I would like confirmation  :>

If the answer is no idea, let's defer to closed.

Right now the minor errors do trigger UI, but don't affect things like treating
it as mixed content or the like.

Right now the minor errors are just OCSP fail open statuses. We're still working
out the policy as to whether SHA-1 certs will be treated as a minor error, same
with certs that are valid too long. Neither will cause an interstitial, but they
WILL be surfaced to the user (omnibox) and MAY affect processing (e.g. mixed
content)

I'm happy with requiring strongest/best to get pooling, and seem to recall
something in HTTP/2 to that effect?

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.h
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:10: #include "net/base/hash_value.h"
On 2014/08/12 14:39:06, Ryan Hamilton wrote:
> On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> > Can forward declare this.
> > 
> > See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> > 
> > "Forward OrReturnValue()"
> 
> Whoa! You forward declare if it's used as a return value? Wow. I would not
have
> expected this to be possible, since i would have expected that the size would
> need to be known... But I guess not! I guess it needs to be know if it's
> *called* but not if it's declared.
> 
> Done.

Yup. That's exactly it - you don't need to know the size until you attempt to
call it.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:39: HashValue GetTestHashValue(uint8 label);
On 2014/08/12 14:39:06, Ryan Hamilton wrote:
> On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> > 1) These should be using uint8_t, not uint8, in SPDY land.
> 
> I'm not sure I follow? We use basictypes all the time in SPDY code. (Both
> internally and externally)

Basictypes is deprecated in Chrome. It existed because MSVC didn't support the
C99 stdint.h - but now it does.

I can't imagine it being essential on the internal code either - e.g. I expect
stdint to exist.

Hence the uint8_t (standard) over uint8 (ours)

> 
> > 2) IWYU: You should be including <stdint.h> (uint8_t) OR "base/basictypes.h"
> > (uint8), rather than relying on spdy_protocol.h to do so.
> 
> Done.

[Ryan Hamilton, 2014-08-12 15:37:10]

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:542: if (IsCertStatusError(ssl_info.cert_status))
On 2014/08/12 14:52:03, Ryan Sleevi wrote:
> On 2014/08/12 14:39:06, Ryan Hamilton wrote:
> > On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> > > Should this be
> > > 
> > > if (IsCertStatusError(ssl_info.cert_status) &&
> > > !IsCertStatusMinorError(ssl_info.cert_status)) ?
> > > 
> > > 
> > > Why or why not?
> > 
> > No idea :> I'm happy to defer to your judgement because I'm unfamiliar with
> the
> > nuances here. We obviously don't want to pool onto a connection that the use
> > would have to override an interstitial for. If minor errors don't trigger
such
> > user-visible issues, then your suggestion is clearly right. I've made that
> > change, but I would like confirmation  :>
> 
> If the answer is no idea, let's defer to closed.
> 
> Right now the minor errors do trigger UI, but don't affect things like
treating
> it as mixed content or the like.
> 
> Right now the minor errors are just OCSP fail open statuses. We're still
working
> out the policy as to whether SHA-1 certs will be treated as a minor error,
same
> with certs that are valid too long. Neither will cause an interstitial, but
they
> WILL be surfaced to the user (omnibox) and MAY affect processing (e.g. mixed
> content)
> 
> I'm happy with requiring strongest/best to get pooling, and seem to recall
> something in HTTP/2 to that effect?

SGTM. Done.

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils.h
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/425803014/diff/60001/net/spdy/spdy_test_utils...
net/spdy/spdy_test_utils.h:39: HashValue GetTestHashValue(uint8 label);
On 2014/08/12 14:52:03, Ryan Sleevi wrote:
> On 2014/08/12 14:39:06, Ryan Hamilton wrote:
> > On 2014/08/11 18:45:17, Ryan Sleevi wrote:
> > > 1) These should be using uint8_t, not uint8, in SPDY land.
> > 
> > I'm not sure I follow? We use basictypes all the time in SPDY code. (Both
> > internally and externally)
> 
> Basictypes is deprecated in Chrome. It existed because MSVC didn't support the
> C99 stdint.h - but now it does.

*sigh* I "love" it that we deprecate stuff but don't use automated tools to
convert to the new standard. We use uint8 140+ times in net/ .h files, and we
use uint8_t 1 time. *boggle*

> I can't imagine it being essential on the internal code either - e.g. I expect
> stdint to exist.

I'm sure it exists, but the standards have not switched yet. The style guide
says "For consistency, use our existing integer types rather than C++11's
equivalents like int16_t where possible." 

> Hence the uint8_t (standard) over uint8 (ours)

In any case, this code is not shared so I'm happy to use the preferred Chrome
solution. 

Done.

[Ryan Hamilton, 2014-08-13 15:21:33]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-08-13 15:22:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/425803014/120001

[commit-bot: I haz the power, 2014-08-14 01:00:20]

Committed patchset #7 (120001) as 289433

[Ryan Hamilton, 2014-08-15 17:45:56]

A revert of this CL (patchset #7) has been created in
https://codereview.chromium.org/474303002/ by rch@chromium.org.

The reason for reverting is: Causes crashes in canary..

[Ryan Hamilton, 2014-08-15 22:48:28]

*facepalm*

sleevi: can you take a look at patch set 9. The original CL which landed was 7,
so you can look at the diffs between those. In particular, the
transport_security_state_ member of QuicStreamFactory was not being initialized.
*sigh* I also added DCHECK() to confirm that it *is* initialized which required
fixing a couple tests.

[Ryan Hamilton, 2014-08-16 16:37:46]

On 2014/08/15 22:48:28, Ryan Hamilton wrote:
> *facepalm*
> 
> sleevi: can you take a look at patch set 9. The original CL which landed was
7,
> so you can look at the diffs between those. In particular, the
> transport_security_state_ member of QuicStreamFactory was not being
initialized.
> *sigh* I also added DCHECK() to confirm that it *is* initialized which
required
> fixing a couple tests.

Ok, I also added some regression tests in patch set 11. PTAL.

[Ryan Sleevi, 2014-08-18 05:50:27]

Patchset 11 LGTM

[Ryan Hamilton, 2014-08-18 15:57:41]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-08-18 15:58:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/425803014/200001

[commit-bot: I haz the power, 2014-08-18 17:34:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-18 19:16:05]

Committed patchset #11 (200001) as 290320

[viettrungluu, 2014-08-18 21:51:27]

On 2014/08/18 19:16:05, I haz the power (commit-bot) wrote:
> Committed patchset #11 (200001) as 290320

I think this may have caused this failure (not 100% positive):

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/15245

(Weirdly, it says it failed at the end, but I don't see a failure anywhere in
the logs.)

[Ryan Hamilton, 2014-08-19 04:12:10]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-08-19 04:13:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/425803014/240001

[Ryan Sleevi, 2014-08-19 05:20:28]

Not LGTM.

If you're going to TBR-CQ, please publish.

As it is, I'm fairly confident you're reintroducing a bug in the test

https://codereview.chromium.org/425803014/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool_unittest.cc:196:
params.transport_security_state = new TransportSecurityState();
Did you mean to delete this?

[commit-bot: I haz the power, 2014-08-19 05:22:24]

Committed patchset #13 (240001) as 290497

[Ryan Hamilton, 2014-08-19 13:50:19]

https://codereview.chromium.org/425803014/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/425803014/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool_unittest.cc:196:
params.transport_security_state = new TransportSecurityState();
On 2014/08/19 05:20:28, Ryan Sleevi wrote:
> Did you mean to delete this?

Yup. Check out line 187:

 params.transport_security_state = transport_security_state_.get();

Though it turns out transport_security_state_ was not being initialized, which I
discovered when I ran git cl try. So the next patch set initialized it in the
constructor. net_unittests then passed the linux_asan bot.

[Ryan Sleevi, 2014-08-19 15:00:32]

That was... Surprising.

Lgtm for history, and shame on me for not yelling about the duplication the
first time around :)