Construct CertificateReportingServiceCertReporter in SSLErrorNavigationThrottle? |
||||
Issue description
I was trying to write a unit test that navigates a WebContents, and was getting a crash.
It *seems* that this is because ChromeContentBrowserClient [1] unconditionally creates a CertificateReportingServiceCertReporter for the SSLErrorNavigationThrottle (when --committed-interstitials is enabled). This in turn relies on a safe browsing service that is present for unit tests.
In addition, meacer@ tells me that constructing a CertificateReportingServiceCertReporter is a little expensive for every havigation.
estark@, what do you think of doing this?
- Move CertificateReportingServiceCertReporter from chrome_content_browser_client.cc to chrome/browser/ssl/ssl_cert_reporter_impl.{h, cc}
- In the committed interstitials code path, construct the SSLCertReporterImpl just to pass into SSLErrorHandler?
[1] https://crrev.com/c/621236 as of patch set 29.
,
Nov 9 2017
,
Nov 9 2017
Actually, that solution doesn't work at all for fixing the crash. I'll look into why the old path doesn't crash; maybe that can help us figure out what to do.
,
Nov 9 2017
After looking into it a bit, I believe that existing unit tests are not triggering ChromeContentBrowserClient::AllowCertificateError() – i.e. the old place where CertificateReportingServiceCertReporter is constructed. And browser tests that trigger the function have the requisite Safe Browsing service. The plan I described above should work, since it will continue to avoid constructing CertificateReportingServiceCertReporter for existing unit tests. However, there are two caveats: - We still need to be able to tell the throttle to use a mock cert reporter for testing. - We need to continue avoiding the actual interstitial construction path in unit tests, or create create the Safe Browsing service beforehand. I think we can handle the former by taking a function that returns a cert reporter – essentially a thunk for the actual SSLErrorReporter. (Alternatively, we could have a SSLCertReporterFactory and a MockSSLCertReporterFactory, although the former would itself be a thin wrapper around CertificateReportingServiceFactory.) estark@, how does that sound?
,
Nov 10 2017
,
Nov 10 2017
> In addition, meacer@ tells me that constructing a CertificateReportingServiceCertReporter is a little expensive for every havigation. Actually this isn't correct. I was thinking about the CertificateReportingService. CertificateReportingServiceCertReporter constructor is pretty simple.
,
Nov 13 2017
> In general, I would recommend against pre-loading a hard Okay, then let's work around it and WontFix for now. I'll revive this if it turns to be needed again. |
||||
►
Sign in to add a comment |
||||
Comment 1 by lgar...@chromium.org
, Nov 9 2017