Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
869393005:
Perform ClientHello padding if the field trial is enabled (Closed)
DescriptionPerform ClientHello padding if the field trial is enabled for
google.com and its subdomains.
BUG=440443
Committed: https://crrev.com/8d44fadd4eab003ceb58048a660f524b8d07f247
Cr-Commit-Position: refs/heads/master@{#315612}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Re-implementation #
Total comments: 2
Patch Set 3 : CR updates #
Total comments: 7
Patch Set 4 : #Patch Set 5 : Rebase #
Total comments: 12
Patch Set 6 : CR comment updates #
Total comments: 4
Patch Set 7 : CR updates #
Total comments: 6
Patch Set 8 : CR update #
Messages
Total messages: 38 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
jeremyim@chromium.org changed reviewers: + agl@chromium.org, asvitkine@chromium.org, rsleevi@chromium.org
agl/rsleevi => net/* and chrome/browser/* asvitkine => histograms.xml PTAL, and thank you!
LGTM https://codereview.chromium.org/869393005/diff/60001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/60001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:1385: ssl_socket_config_service_->IsGoogle(host_and_port_)) Nit: {}
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
Overall, I think this Not LGTM. I'm not keen of introducing the SSLSocketConfigService, nor is it clear from reading how this fundamentally or functionally differs from SSLConfigService. I'm also not a fan of new optional parameters that may be nullptr, may not be - I think it adds to the combinatorial complexity for testing and reasoning about the code. The question of "IsGoogle" and histogramming also has a layering smell to it. If you do pursue this route of dependency injection (and you really can't find an existing dependency - like SSLConfigService - to inject into), then it seems more suitable to let the //chrome layer make this decision and just treat this class as a simple bool of "Should I do this" From an SSLSocket layering perspective, you're pushing all of the logic into the SSLSocket, but I'm not sure it belongs there. Doesn't this make more sense as a simple bool to the construction of the SSLSocket (and perhaps even the ConnectJob), with the logic living at a higher layer? It's not clear what benefits we get (performance or layering) from delaying that decision as long as possible - it feels like as soon as we know the host, we should query whatever service and then use a bool from there on. https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... File net/ssl/ssl_socket_config_service.h (right): https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... net/ssl/ssl_socket_config_service.h:23: void DisableFastRadioPadding(); Why isn't this a single function with a bool arg? https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... net/ssl/ssl_socket_config_service.h:30: // logic in ssl_client_socket_pool.cc This comment seems very much a layering violation. Talk about what it does, not about what something else unrelated does. https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... net/ssl/ssl_socket_config_service.h:31: virtual bool IsGoogle(const HostPortPair& host_and_port); Why is this virtual? "For Testing"? https://codereview.chromium.org/869393005/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/869393005/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19156: + <owner>agl@chromium.org</owner> You wrote this code, you own it.
Thanks for the feedback; this is definitely an area of the code base with which I'm not very familiar, so I will continue to iterate on it. On 2015/01/30 22:11:32, Ryan Sleevi wrote: > Overall, I think this Not LGTM. I'm not keen of introducing the > SSLSocketConfigService, nor is it clear from reading how this fundamentally or > functionally differs from SSLConfigService. I'm also not a fan of new optional > parameters that may be nullptr, may not be - I think it adds to the > combinatorial complexity for testing and reasoning about the code. > > The question of "IsGoogle" and histogramming also has a layering smell to it. If > you do pursue this route of dependency injection (and you really can't find an > existing dependency - like SSLConfigService - to inject into), then it seems > more suitable to let the //chrome layer make this decision and just treat this > class as a simple bool of "Should I do this" > > From an SSLSocket layering perspective, you're pushing all of the logic into the > SSLSocket, but I'm not sure it belongs there. Doesn't this make more sense as a > simple bool to the construction of the SSLSocket (and perhaps even the > ConnectJob), with the logic living at a higher layer? It's not clear what > benefits we get (performance or layering) from delaying that decision as long as > possible - it feels like as soon as we know the host, we should query whatever > service and then use a bool from there on. > > https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... > File net/ssl/ssl_socket_config_service.h (right): > > https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... > net/ssl/ssl_socket_config_service.h:23: void DisableFastRadioPadding(); > Why isn't this a single function with a bool arg? > > https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... > net/ssl/ssl_socket_config_service.h:30: // logic in ssl_client_socket_pool.cc > This comment seems very much a layering violation. Talk about what it does, not > about what something else unrelated does. > > https://codereview.chromium.org/869393005/diff/60001/net/ssl/ssl_socket_confi... > net/ssl/ssl_socket_config_service.h:31: virtual bool IsGoogle(const > HostPortPair& host_and_port); > Why is this virtual? "For Testing"? > > https://codereview.chromium.org/869393005/diff/60001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/869393005/diff/60001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:19156: + > <mailto:owner>agl@chromium.org</owner> > You wrote this code, you own it.
On 2015/01/30 22:42:39, jeremyim wrote: > Thanks for the feedback; this is definitely an area of the code base with which > I'm not very familiar, so I will continue to iterate on it. Great. David or I should be able to hopefully provide guidance if you have questions - and clarifications if any of the above comments weren't clear.
If I end up using a bool set in //chrome to configure fastradio padding in //net, what is an appropriate vehicle for sending this information? It looks like it should be possible to update the URLRequest from NetworkDelegate::NotifyBeforeURLRequest to accomplish this. On 2015/01/30 22:46:38, Ryan Sleevi wrote: > On 2015/01/30 22:42:39, jeremyim wrote: > > Thanks for the feedback; this is definitely an area of the code base with > which > > I'm not very familiar, so I will continue to iterate on it. > > Great. David or I should be able to hopefully provide guidance if you have > questions - and clarifications if any of the above comments weren't clear.
No. Anything at a socket layer has no relation to URLRequests (because of how socket pools work, pre-connects and pre-fetches may have already pre-warmed a socket that is then bound to a new URLRequest. The same applies for HTTP keepalives - an existing socket may be reused to satisfy a new URLRequest) You're on the right track - that is, that you need to pass it through to the socket pools, which means it has to go through the HttpNetworkTransaction (which owns the pools). I suspect a logical splice point is somewhere around the socket pool logic - that's where you can take the concrete object (which might be the SSLConfigService?) - ask about settings for a host, and translate that into a bool. Then, through the SSLConnectJob and SSLClientSocket, they're just 'dumb' with respect what's going on. On Fri, Jan 30, 2015 at 4:01 PM, <jeremyim@chromium.org> wrote: > If I end up using a bool set in //chrome to configure fastradio padding in > //net, what is an appropriate vehicle for sending this information? It > looks > like it should be possible to update the URLRequest from > NetworkDelegate::NotifyBeforeURLRequest to accomplish this. > > > On 2015/01/30 22:46:38, Ryan Sleevi wrote: > >> On 2015/01/30 22:42:39, jeremyim wrote: >> > Thanks for the feedback; this is definitely an area of the code base >> with >> which >> > I'm not very familiar, so I will continue to iterate on it. >> > > Great. David or I should be able to hopefully provide guidance if you have >> questions - and clarifications if any of the above comments weren't clear. >> > > > https://codereview.chromium.org/869393005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #2 (id:80001) has been deleted
The code has been rewritten, and should address the high level concerns of the previous attempt. The logic for determining whether padding should be enabled or not lives completely in //chrome, and a couple of flags in SSL config control whether to enable it and whether to add it to the histogram (for measuring control vs experiment). On 2015/01/31 00:10:32, Ryan Sleevi wrote: > No. Anything at a socket layer has no relation to URLRequests (because of > how socket pools work, pre-connects and pre-fetches may have already > pre-warmed a socket that is then bound to a new URLRequest. The same > applies for HTTP keepalives - an existing socket may be reused to satisfy a > new URLRequest) > > You're on the right track - that is, that you need to pass it through to > the socket pools, which means it has to go through the > HttpNetworkTransaction (which owns the pools). I suspect a logical splice > point is somewhere around the socket pool logic - that's where you can take > the concrete object (which might be the SSLConfigService?) - ask about > settings for a host, and translate that into a bool. Then, through the > SSLConnectJob and SSLClientSocket, they're just 'dumb' with respect what's > going on. > > On Fri, Jan 30, 2015 at 4:01 PM, <mailto:jeremyim@chromium.org> wrote: > > > If I end up using a bool set in //chrome to configure fastradio padding in > > //net, what is an appropriate vehicle for sending this information? It > > looks > > like it should be possible to update the URLRequest from > > NetworkDelegate::NotifyBeforeURLRequest to accomplish this. > > > > > > On 2015/01/30 22:46:38, Ryan Sleevi wrote: > > > >> On 2015/01/30 22:42:39, jeremyim wrote: > >> > Thanks for the feedback; this is definitely an area of the code base > >> with > >> which > >> > I'm not very familiar, so I will continue to iterate on it. > >> > > > > Great. David or I should be able to hopefully provide guidance if you have > >> questions - and clarifications if any of the above comments weren't clear. > >> > > > > > > https://codereview.chromium.org/869393005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Much better! One last comment about how high level we can push this decision, and a question about the double bool. Overall though, looks like the right approach. https://codereview.chromium.org/869393005/diff/100001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/869393005/diff/100001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1278: ssl_config->fastradio_padding_eligible = true; Why isn't this passed in to the construction of the HttpStreamFactoryJobImpl::Job The flow is (ignoring preconnect), https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre... being called by https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre... This is called by https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... , which we get from the HttpNetworkTransaction at https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... It seems like https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... is perhaps a more logical place to put things, examining |request_info.url| https://codereview.chromium.org/869393005/diff/100001/net/ssl/ssl_config.h File net/ssl/ssl_config.h (right): https://codereview.chromium.org/869393005/diff/100001/net/ssl/ssl_config.h#ne... net/ssl/ssl_config.h:169: bool fastradio_padding_eligible; Why would/should |enable_fastradio_padding| be needed? Shouldn't _eligible suffice?
https://codereview.chromium.org/869393005/diff/120001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/869393005/diff/120001/net/http/http_network_t... net/http/http_network_transaction.cc:186: } It's possible to move the fastradio config setting to this location pretty easily, and moves it up from the previous location in HttpStreamFactoryImpl::Job https://codereview.chromium.org/869393005/diff/120001/net/ssl/ssl_config.h File net/ssl/ssl_config.h (right): https://codereview.chromium.org/869393005/diff/120001/net/ssl/ssl_config.h#ne... net/ssl/ssl_config.h:167: // false - in that case, fastradio padding will not be used. There are two bools here to capture 3 states we care about (for measuring the error rate in the experiment) in SSLClientSocketOpenSSL: - Use fast radio padding (and log to Net.SSL_Connection_Error_ClientPadding) - Don't use fast radio padding (and log to Net.SSL_Connection_Error_ClientPadding since it was "eligible" but not "enabled") - Don't use fast radio padding (don't log to Net.SSL_Connection_Error_ClientPadding since it wasn't eligible) In this case, eligible means passing the google.com domain check, while enabled means whether the session was part of the field trial.
https://codereview.chromium.org/869393005/diff/120001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/120001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:130: host == kGoogleDomain || ".google.com" isn't a valid hostname. I assume you wanted "google.com". https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:479: std::abs(rv)); It looks like SSL_Connection_Error metrics were added specifically for this experiment. We already have a pile of connection metrics here: https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... The ones here apply only to OpenSSL, but some of our ports still ship NSS. They're also duplicated a lot. If I recall, the histogram macros expand out to a decent amount of code with a static thing to cache them and everything. We don't actually want to be duplicating them in lots of places like that. This also is more directly comparable with the _Latency metrics. Not every SSLClientSocket goes through SSLConnectJob. We also already have error metrics at a higher level here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... Both sets are also already keyed on Google, so that might avoid your extra boolean. Though it would also somewhat assume that the higher-level check is compatible with the lower-level check. I'll defer to Ryan there.
https://codereview.chromium.org/869393005/diff/120001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/120001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:130: host == kGoogleDomain || On 2015/02/04 01:04:47, David Benjamin wrote: > ".google.com" isn't a valid hostname. I assume you wanted "google.com". Done. https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:479: std::abs(rv)); On 2015/02/04 01:04:47, David Benjamin wrote: > It looks like SSL_Connection_Error metrics were added specifically for this > experiment. We already have a pile of connection metrics here: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... > > The ones here apply only to OpenSSL, but some of our ports still ship NSS. > They're also duplicated a lot. If I recall, the histogram macros expand out to a > decent amount of code with a static thing to cache them and everything. We don't > actually want to be duplicating them in lots of places like that. > > This also is more directly comparable with the _Latency metrics. Not every > SSLClientSocket goes through SSLConnectJob. > > We also already have error metrics at a higher level here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > Both sets are also already keyed on Google, so that might avoid your extra > boolean. Though it would also somewhat assume that the higher-level check is > compatible with the lower-level check. I'll defer to Ryan there. We were planning on using the latency metrics from ssl_client_socket_pool.cc and we were only adding these metrics to measure error rates. We didn't have confidence that the higher level main/sub frame error rates would give us sufficient visibility into whether the padding introduced higher error rates. I think we are ok with the metrics only being in OpenSSL since NSS doesn't support fastradio padding.
https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:479: std::abs(rv)); On 2015/02/04 17:26:32, jeremyim wrote: > On 2015/02/04 01:04:47, David Benjamin wrote: > > It looks like SSL_Connection_Error metrics were added specifically for this > > experiment. We already have a pile of connection metrics here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... > > > > The ones here apply only to OpenSSL, but some of our ports still ship NSS. > > They're also duplicated a lot. If I recall, the histogram macros expand out to > a > > decent amount of code with a static thing to cache them and everything. We > don't > > actually want to be duplicating them in lots of places like that. > > > > This also is more directly comparable with the _Latency metrics. Not every > > SSLClientSocket goes through SSLConnectJob. > > > > We also already have error metrics at a higher level here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > > Both sets are also already keyed on Google, so that might avoid your extra > > boolean. Though it would also somewhat assume that the higher-level check is > > compatible with the lower-level check. I'll defer to Ryan there. > > We were planning on using the latency metrics from ssl_client_socket_pool.cc and > we were only adding these metrics to measure error rates. We didn't have > confidence that the higher level main/sub frame error rates would give us > sufficient visibility into whether the padding introduced higher error rates. > > I think we are ok with the metrics only being in OpenSSL since NSS doesn't > support fastradio padding. SSL_Connection_Error is extremely generic and isn't specific to OpenSSL/BoringSSL. It's very confusing to have metrics (unless they're so specific so as to be measuring implementation quirks like Net.SSLSessionVersionMatch) only be available on one side and not the other. In fact, the immediate cause for my bringing this up is I noticed that metric and immediately went to compare it before and after the NSS -> BoringSSL switch across platforms. (It turns out the metric was newly added, so it wouldn't have been helpful anyway. But there are still NSS -> BoringSSL switches to come.) There's also still the other concerns like binary size from expanding the histogram macro in so many places and being able to directly compare SSL_Connection_Error and SSL_Connection_Latency.
On 2015/02/04 17:44:36, David Benjamin wrote: > https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_openssl.cc (right): > > https://codereview.chromium.org/869393005/diff/120001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.cc:479: std::abs(rv)); > On 2015/02/04 17:26:32, jeremyim wrote: > > On 2015/02/04 01:04:47, David Benjamin wrote: > > > It looks like SSL_Connection_Error metrics were added specifically for this > > > experiment. We already have a pile of connection metrics here: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... > > > > > > The ones here apply only to OpenSSL, but some of our ports still ship NSS. > > > They're also duplicated a lot. If I recall, the histogram macros expand out > to > > a > > > decent amount of code with a static thing to cache them and everything. We > > don't > > > actually want to be duplicating them in lots of places like that. > > > > > > This also is more directly comparable with the _Latency metrics. Not every > > > SSLClientSocket goes through SSLConnectJob. > > > > > > We also already have error metrics at a higher level here: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > > > > Both sets are also already keyed on Google, so that might avoid your extra > > > boolean. Though it would also somewhat assume that the higher-level check is > > > compatible with the lower-level check. I'll defer to Ryan there. > > > > We were planning on using the latency metrics from ssl_client_socket_pool.cc > and > > we were only adding these metrics to measure error rates. We didn't have > > confidence that the higher level main/sub frame error rates would give us > > sufficient visibility into whether the padding introduced higher error rates. > > > > I think we are ok with the metrics only being in OpenSSL since NSS doesn't > > support fastradio padding. > > SSL_Connection_Error is extremely generic and isn't specific to > OpenSSL/BoringSSL. It's very confusing to have metrics (unless they're so > specific so as to be measuring implementation quirks like > Net.SSLSessionVersionMatch) only be available on one side and not the other. > > In fact, the immediate cause for my bringing this up is I noticed that metric > and immediately went to compare it before and after the NSS -> BoringSSL switch > across platforms. (It turns out the metric was newly added, so it wouldn't have > been helpful anyway. But there are still NSS -> BoringSSL switches to come.) > > There's also still the other concerns like binary size from expanding the > histogram macro in so many places and being able to directly compare > SSL_Connection_Error and SSL_Connection_Latency. Is it reasonable then to move the Error metrics to live alongside the Latency metrics? It won't address the binary size issue, but would permit direct comparing of Latency and Error metrics (and would remove the dual boolean taking place at the "cost" of having the logic live in 2 places for determining whether Google was used or not).
not lgtm, since the underwent large changes since my previous one. Happy to re-approve after comments addressed. https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:35: const char* kGoogleDomain = "google.com"; Nit: Change to the same syntax as the ones above, which is more efficient. https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:131: (host.size() > 10 && host.rfind(kGoogleDomain) == host.size() - 10); Can this use some of the existing utils we have in Chrome, such as IsGoogleHostname()? https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:477: if (ssl_config_.fastradio_padding_eligible) Nit: {} https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:498: if (ssl_config_.fastradio_padding_eligible) Nit: {} https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1381: if (ssl_config_.fastradio_padding_eligible) DItto.
lgtm https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:131: (host.size() > 10 && host.rfind(kGoogleDomain) == host.size() - 10); On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > Can this use some of the existing utils we have in Chrome, such as > IsGoogleHostname()? +1
https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:131: (host.size() > 10 && host.rfind(kGoogleDomain) == host.size() - 10); On 2015/02/06 22:12:59, Ryan Sleevi wrote: > On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > > Can this use some of the existing utils we have in Chrome, such as > > IsGoogleHostname()? > > +1 If not switching it, note the current logic will trip on "https://notgoogle.com". The suffix case should include the ., the equality case should not.
New patchsets have been uploaded after l-g-t-m from rsleevi@chromium.org
https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:35: const char* kGoogleDomain = "google.com"; On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > Nit: Change to the same syntax as the ones above, which is more efficient. Done. https://codereview.chromium.org/869393005/diff/160001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:131: (host.size() > 10 && host.rfind(kGoogleDomain) == host.size() - 10); On 2015/02/06 22:22:54, David Benjamin wrote: > On 2015/02/06 22:12:59, Ryan Sleevi wrote: > > On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > > > Can this use some of the existing utils we have in Chrome, such as > > > IsGoogleHostname()? > > > > +1 > > If not switching it, note the current logic will trip on > "https://notgoogle.com". The suffix case should include the ., the equality case > should not. Done. https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:477: if (ssl_config_.fastradio_padding_eligible) On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > Nit: {} Done. https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:498: if (ssl_config_.fastradio_padding_eligible) On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > Nit: {} Done. https://codereview.chromium.org/869393005/diff/160001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1381: if (ssl_config_.fastradio_padding_eligible) On 2015/02/06 20:26:34, asvitkine (slow this week) wrote: > DItto. Done.
https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:482: (host.size() > 11 && host.rfind(".google.com") == host.size() - 11); Can this use google_util::IsGoogleHostname() too?
https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:482: (host.size() > 11 && host.rfind(".google.com") == host.size() - 11); On 2015/02/06 22:44:01, asvitkine (slow this week) wrote: > Can this use google_util::IsGoogleHostname() too? Unfortunately, I believe that's a layering violation (since google_util lives in components). =\
https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:482: (host.size() > 11 && host.rfind(".google.com") == host.size() - 11); On 2015/02/06 22:47:22, jeremyim wrote: > On 2015/02/06 22:44:01, asvitkine (slow this week) wrote: > > Can this use google_util::IsGoogleHostname() too? > > Unfortunately, I believe that's a layering violation (since google_util lives in > components). =\ Correct. I mean, this is a layering violation as it is (//net shouldn't have Google-specific logic), so the new dependency on it Not LGTM.
https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:482: (host.size() > 11 && host.rfind(".google.com") == host.size() - 11); On 2015/02/07 00:20:56, Ryan Sleevi wrote: > On 2015/02/06 22:47:22, jeremyim wrote: > > On 2015/02/06 22:44:01, asvitkine (slow this week) wrote: > > > Can this use google_util::IsGoogleHostname() too? > > > > Unfortunately, I believe that's a layering violation (since google_util lives > in > > components). =\ > > Correct. > > I mean, this is a layering violation as it is (//net shouldn't have > Google-specific logic), so the new dependency on it Not LGTM. This change has been reverted. Back to using 2 booleans in SSL_Config (with comments) indicating when fastradio_padding is turned on, and UMA should be logged (but fastradio_padding is not turned on).
lgtm
LGTM % nits https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:127: bool SSLConfigServicePref::SupportsFastradioPadding(const GURL& url) { Should this be FastRadioPadding? "fastradio" isn't one word, is it? https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:305: if (group.starts_with(kClientHelloFieldTrialEnabledGroupName)) { Why is this .starts_with and not ==? https://codereview.chromium.org/869393005/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:564: } For consistency with the rest of the code, seems like this should be moved to ~line 550 (after the closing brace)
New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org,rsleevi@chromium.org
https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:127: bool SSLConfigServicePref::SupportsFastradioPadding(const GURL& url) { On 2015/02/09 19:42:34, Ryan Sleevi wrote: > Should this be FastRadioPadding? "fastradio" isn't one word, is it? The BoringSSL function is SSL_enable_fastradio_padding, which implies that it is one word. https://codereview.chromium.org/869393005/diff/200001/chrome/browser/net/ssl_... chrome/browser/net/ssl_config_service_manager_pref.cc:305: if (group.starts_with(kClientHelloFieldTrialEnabledGroupName)) { On 2015/02/09 19:42:34, Ryan Sleevi wrote: > Why is this .starts_with and not ==? In case there is a reason to have multiple "Enabled" experiment groups; at least in io_thread.cc the usage seems to be split between starts_with and ==. https://codereview.chromium.org/869393005/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/869393005/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:564: } On 2015/02/09 19:42:34, Ryan Sleevi wrote: > For consistency with the rest of the code, seems like this should be moved to > ~line 550 (after the closing brace) Done.
The CQ bit was checked by jeremyim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869393005/220001
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8d44fadd4eab003ceb58048a660f524b8d07f247 Cr-Commit-Position: refs/heads/master@{#315612} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
