Plumb certificate error information to navigation code through NavigationURLLoaderNetworkService |
|||
Issue descriptionWhen a request fails due to a certificate error, we need to plumb the SSLInfo to NavigationRequest. Here's the plan for doing that: Add an optional SSLInfo to ResourceRequestCompletionStatus, and set it in URLLoader::NotifyCompleted when there is a certificate error. From there the SSLInfo will be passed in to NavigationURLLoaderNetworkService::URLLoaderRequestControllerURLLoaderClient::OnComplete, in to NavigationURLLoaderNetworkService::OnComplete, and we can set it when we pass it in to NavigationURLLoader::Delegate::OnRequestFailed there.
,
Nov 10 2017
,
Nov 21 2017
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883 commit b09f19ac5bdc4621cb564fc8c0b94e9a23a28883 Author: Emily Stark <estark@google.com> Date: Wed Nov 22 22:41:47 2017 Send SSLInfo to URLLoaderClient::OnComplete when needed For committed interstitials (go/chrome-interstitial-refactor), requests that fail with certificate errors need SSLInfo attached, so that the appropriate error page can be shown. To make this design work with the network service, we include SSLInfo in URLLoaderClient::OnComplete when needed: that is, for main-frame requests that have certificate errors. Sending the certificate chain over IPC twice for a single request (once in OnSSLCertificateError, once in OnComplete) is not ideal. This CL includes UMA histograms for how often it's sent and the size of the certificate chain to gauge whether this is likely to be a noticeable performance impact. If it is, we will consider alternative, more complicated designs that allow the NavigationRequest to receive the SSLInfo via OnSSLCertificateError so that it doesn't have to be sent again in OnComplete. Bug: 783265 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: Icf8bfc7c9fd1e85158811bcd7df3736d1ba93504 Reviewed-on: https://chromium-review.googlesource.com/783874 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Emily Stark <estark@chromium.org> Cr-Commit-Position: refs/heads/master@{#518776} [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/browser/download/resource_downloader.cc [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/browser/loader/navigation_url_loader_delegate.h [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/browser/loader/navigation_url_loader_network_service.cc [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/common/resource_messages.h [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/network/url_loader.cc [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/network/url_loader_unittest.cc [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/public/common/url_loader.mojom [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/content/public/common/url_loader_factory.mojom [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/services/network/public/cpp/url_loader_completion_status.h [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter [modify] https://crrev.com/b09f19ac5bdc4621cb564fc8c0b94e9a23a28883/tools/metrics/histograms/histograms.xml
,
Nov 22 2017
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e70c4a04ca34dcfa38c26c016f2c164c2e68d0c2 commit e70c4a04ca34dcfa38c26c016f2c164c2e68d0c2 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Wed May 02 15:49:10 2018 Fix non matching histogram name. The histogram is used in NavigationURLLoaderNetworkService as: Navigation.URLLoaderNetworkService.OnCompleteHadSSLInfo But it is declared in histograms.xml as: Navigation.URLLoaderNetworkService.OnCompleteHasSSLInfo The names don't match ("Had" vs "Has"). This CL fixes it. Bug: 783265 Change-Id: I33ea9ae1ba92aaa20dab4b80f3e731b4229f3efa Reviewed-on: https://chromium-review.googlesource.com/1039200 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#555404} [modify] https://crrev.com/e70c4a04ca34dcfa38c26c016f2c164c2e68d0c2/content/browser/loader/navigation_url_loader_network_service.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by est...@chromium.org
, Nov 9 2017