Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503243003:
Wire up CertificateReportingService to handle report uploads (Closed)
DescriptionWire up CertificateReportingService to handle report uploads
This CL is a follow up to crrev.com/2543523002. It routes certificate reports
through CertificateReportingService instead of SafeBrowsingPingManager.
Reporting code in SafeBrowsingPingManager isn't removed yet. This is done
in https://crrev.com/2578873003
Background:
The main feature is to retry uploads of invalid SSL certificate reports.
Currently, the reports are uploaded only once when the user sees an SSL error.
This isn't always sufficient though, as the user might be behind a captive
portal which blocks all connections to the internet. For that reason, this
feature queues failed uploads and retries them at a later time. All of this
logic is in CertificateReportingService.
However, CertificateReportingService doesn't do any scheduling of retries itself,
it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries
queued (pending) report uploads whenever it's asked to provide metrics. This logic is
very much like HttpsEngagementMetricsProvider which also relies on the metrics service
to do the scheduling.
BUG=554323
TEST=CertificateReportingServiceBrowserTest.*
Committed: https://crrev.com/f43117a3430e9ec23d2d2329175257fa4d6059f0
Cr-Commit-Position: refs/heads/master@{#440788}
Patch Set 1 #Patch Set 2 : Move cert reporting service to safebrowsing #Patch Set 3 : CertificateReportingService unit tests #Patch Set 4 : Rebase onto crrev/2543523002 #Patch Set 5 : Lint2 #Patch Set 6 : Rebase #Patch Set 7 : Add missing file #Patch Set 8 : Don't leak SafeBrowsingService in unit tests #Patch Set 9 : Add histogram tests #Patch Set 10 : Fix browser tests and histograms #Patch Set 11 : Fix histogram unit tests #Patch Set 12 : Rebase #
Total comments: 50
Patch Set 13 : jialiul, estark comments #
Total comments: 13
Patch Set 14 : estark comments and remove PreferenceObserver #Patch Set 15 : Fix metrics provider - based on crrev.com/2531023002 #
Total comments: 6
Patch Set 16 : estark comments #
Total comments: 7
Patch Set 17 : More documentation for CertificateReportingMetricsProvider #
Total comments: 4
Patch Set 18 : isherman comments #Patch Set 19 : Rebase #Messages
Total messages: 85 (62 generated)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement certificate reporting service. This service handles reporting of invalid certificate reports and retries sending any unsuccessful reports. BUG=554323 ========== to ========== Wire up CertificateReportingService to handle report uploads. This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Wire up CertificateReportingService to handle report uploads. This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ========== to ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ========== to ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. I'll do that in a separate CL. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ==========
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
meacer@chromium.org changed reviewers: + estark@chromium.org, jialiul@chromium.org
estark, jialiul: PTAL? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some initial comments. I haven't looked at tests very carefully. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:24: if (!service) nit, if (service) service->SendPending(); https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:62: safe_browsing::IsExtendedReportingEnabled(prefs_); You probably want to check kSafeBrowsingExtendedReportingOptInAllowed as well in case enterprise setting overrides extended reporting preference. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:48: // does this by subscribing to SafeBrowsing service shut downs when it's nit s/"SafeBrowsingService"/"SafeBrowsing service" multiple places https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_factory.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_factory.h:28: void SetReportEncryptionParamsForTesting(uint8_t* server_public_key_, extra "_" https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:169: std::unique_ptr<ShutdownSubscription> RegisterShutdownCallback( Could you add some comments on what thread these will be called and roughly what kind if task these callbacks perform (either here or where shutdown_callback_list_ is defined.)
Looks good, some nits and suggestions for test refactoring and questions. I focused on the tests because my brain was fried after reading those, will look at the rest more tomorrow. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:77: #include "chrome/browser/safe_browsing/safe_browsing_service.h" can be removed now? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:78: #include "chrome/browser/safe_browsing/ui_manager.h" ditto https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:676: class CertificateReportingServiceCertReporter : public SSLCertReporter { Hrmmmmm. There is not much reason for this SSLCertReporter interface to exist anymore. CertReportHelper in chrome/browser/ssl could just using the CertificateReportingService directly. We originally did this SSLCertReporter interface to avoid introducing a c/b/ssl -> c/b/safe_browsing dependency... but now c/b/ssl depends on c/b/safe_browsing all over the place. So we could get rid of this interface. Or we could leave it as you have it and file a bug to clean up the c/b/ssl => c/b/safe_browsing dependency. (I don't think it would be that hard to clean it up.) I have a slight preference for the latter, since it is kinda weird that c/b/ssl depends on safebrowsing. Do you have any preference? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:152: virtual ~PreferenceObserver() {} nit: looks weird to me to have the destructor at the bottom https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:53: class CertificateReportingServiceBrowserTest It would be helpful to have a comment here explaining the overall testing strategy, saying that the tests generally want to wait for reports to send successfully or fail, or they want to wait to check that no reports have been sent, and how each of those things is implemented. (For example, checking that no reports have been sent is done by posting a dummy task to the IO thread and waiting for it, because if a report had been sent it would have been seen by the IO thread before that dummy task.) https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:56: CertificateReportingServiceTestBase { Sorry, I missed the multiple inheritance of CertificateReportingServiceTestBase when you introduced it in an earlier CL. I mentioned this about the histogram test base too -- I think these should be composition instead of inheritance. i.e. so CertificateReportingServiceBrowserTest has a CertificateReportingServiceTestBase on which it calls SetUpInterceptor, etc. Could be a future follow-up if it's too messy to change it in this CL. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:62: expect_delayed_report_on_teardown_ = false; any reason to do this here instead of initializing in the constructor? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:123: // events: 1 event for send completion or cancellation, the rest for How are these events observed? I see you call for WaitIOThread after the interstitial goes away... but I can't figure out from here how you're observing what happens to the report. ... oh wait, could this be a comment leftover from EventObserver days of yore? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:142: void ChangeOptInAndWait(certificate_reporting_test_utils::OptIn opt_in) { Might want to explain this a bit in a comment -- I guess setting a pref synchronously fires an observer, and CertificateReportingService observes the observer firing and posts a task to the IO thread to reset? Is that right? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:172: // Resumes the delayed request and waits for the actual resume to finish. Doesn't parse; is "actual resume" supposed to be "actual request"? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:196: // to wait for the SafeBrowsingService to finish loading/stopping. Is this comment accurate/up-to-date? Looks a little odd to me because I'm not sure when we wait for SBService to load/stop. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:246: int num_expected_failed_report_ = 0; unsigned int https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:262: SendReport("no-report"); Is this missing a WaitForReports() call? I can't figure out where the test would fail if a report was actually sent. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:320: SendPendingReports(); ditto here and below: I can't remember/figure out where the test fails if there is a pending report that gets sent https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:73: // Sets the expected histogram value to be checked during teardown. nit: maybe make this SetExpectedHistogramFailedCountOnTeardown or something with "failed" in it somewhere. It's a little confusing down below where it says SetExpectedHistogramCount(0) but then reports get sent -- not clear that the histogram only counts failed reports. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:89: int num_expected_failed_report_ = 0; unsigned int https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:136: public HistogramTestBase { Instead of the multiple inheritance, could the test fixtures have a HistogramTestBase? (i.e. so they call histogram_test_->CheckHistogram() where you're currently calling CheckHistogram()) https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:319: public HistogramTestBase { same here https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:369: base::Unretained(this))); This looks a little sketchy to me. I'd think we need to wait, otherwise `this` could be destroyed before TearDownOnIOThread() runs. Needs a WaitForIOThread afterwards, maybe?
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:77: #include "chrome/browser/safe_browsing/safe_browsing_service.h" On 2016/12/16 01:57:58, estark wrote: > can be removed now? Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:78: #include "chrome/browser/safe_browsing/ui_manager.h" On 2016/12/16 01:57:58, estark wrote: > ditto Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:676: class CertificateReportingServiceCertReporter : public SSLCertReporter { On 2016/12/16 01:57:58, estark wrote: > Hrmmmmm. There is not much reason for this SSLCertReporter interface to exist > anymore. CertReportHelper in chrome/browser/ssl could just using the > CertificateReportingService directly. > > We originally did this SSLCertReporter interface to avoid introducing a c/b/ssl > -> c/b/safe_browsing dependency... but now c/b/ssl depends on c/b/safe_browsing > all over the place. > > So we could get rid of this interface. Or we could leave it as you have it and > file a bug to clean up the c/b/ssl => c/b/safe_browsing dependency. (I don't > think it would be that hard to clean it up.) I have a slight preference for the > latter, since it is kinda weird that c/b/ssl depends on safebrowsing. Do you > have any preference? I've been avoiding adding more c/b/sb dependency to ssl so I'd prefer cleaning out the dependencies instead. As far as I can tell, the only dependencies are in tests and one other place which I removed in the next CL (chrome/browser/ssl/certificate_reporting_test_utils.cc). The rest of the code in ssl depends on components/safe_browsing_db instead. But maybe I'm missing something? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:24: if (!service) On 2016/12/16 01:55:24, Jialiu Lin wrote: > nit, > if (service) > service->SendPending(); Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:62: safe_browsing::IsExtendedReportingEnabled(prefs_); On 2016/12/16 01:55:24, Jialiu Lin wrote: > You probably want to check kSafeBrowsingExtendedReportingOptInAllowed as well in > case enterprise setting overrides extended reporting preference. Would it make sense to add kSafeBrowsingExtendedReportingOptInAllowed check to IsExtendedReportingEnabled instead of here? Or do we want to keep them separate? https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:48: // does this by subscribing to SafeBrowsing service shut downs when it's On 2016/12/16 01:55:24, Jialiu Lin wrote: > nit s/"SafeBrowsingService"/"SafeBrowsing service" multiple places Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:152: virtual ~PreferenceObserver() {} On 2016/12/16 01:57:58, estark wrote: > nit: looks weird to me to have the destructor at the bottom Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:53: class CertificateReportingServiceBrowserTest On 2016/12/16 01:57:58, estark wrote: > It would be helpful to have a comment here explaining the overall testing > strategy, saying that the tests generally want to wait for reports to send > successfully or fail, or they want to wait to check that no reports have been > sent, and how each of those things is implemented. (For example, checking that > no reports have been sent is done by posting a dummy task to the IO thread and > waiting for it, because if a report had been sent it would have been seen by the > IO thread before that dummy task.) Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:56: CertificateReportingServiceTestBase { On 2016/12/16 01:57:59, estark wrote: > Sorry, I missed the multiple inheritance of CertificateReportingServiceTestBase > when you introduced it in an earlier CL. I mentioned this about the histogram > test base too -- I think these should be composition instead of inheritance. > i.e. so CertificateReportingServiceBrowserTest has a > CertificateReportingServiceTestBase on which it calls SetUpInterceptor, etc. > > Could be a future follow-up if it's too messy to change it in this CL. Good point, done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:62: expect_delayed_report_on_teardown_ = false; On 2016/12/16 01:57:58, estark wrote: > any reason to do this here instead of initializing in the constructor? Nope :) https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:123: // events: 1 event for send completion or cancellation, the rest for On 2016/12/16 01:57:59, estark wrote: > How are these events observed? I see you call for WaitIOThread after the > interstitial goes away... but I can't figure out from here how you're observing > what happens to the report. > > ... > > oh wait, could this be a comment leftover from EventObserver days of yore? Indeed. Removed. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:142: void ChangeOptInAndWait(certificate_reporting_test_utils::OptIn opt_in) { On 2016/12/16 01:57:59, estark wrote: > Might want to explain this a bit in a comment -- I guess setting a pref > synchronously fires an observer, and CertificateReportingService observes the > observer firing and posts a task to the IO thread to reset? Is that right? Correct, clarified. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:172: // Resumes the delayed request and waits for the actual resume to finish. On 2016/12/16 01:57:59, estark wrote: > Doesn't parse; is "actual resume" supposed to be "actual request"? Well, it does indeed wait for the "Resume" task to finish, which causes the response to start, but reworded. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:196: // to wait for the SafeBrowsingService to finish loading/stopping. On 2016/12/16 01:57:59, estark wrote: > Is this comment accurate/up-to-date? Looks a little odd to me because I'm not > sure when we wait for SBService to load/stop. It's because it's taken from SafeBrowsingService. Fixed. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:246: int num_expected_failed_report_ = 0; On 2016/12/16 01:57:59, estark wrote: > unsigned int Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:262: SendReport("no-report"); On 2016/12/16 01:57:58, estark wrote: > Is this missing a WaitForReports() call? I can't figure out where the test would > fail if a report was actually sent. The test teardown checks for created and in-flight requests. If a report was incorrectly sent, it should fail there. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:320: SendPendingReports(); On 2016/12/16 01:57:59, estark wrote: > ditto here and below: I can't remember/figure out where the test fails if there > is a pending report that gets sent Same as NotOptedIn_ShouldNotSendReports, the tests will fail in test TearDown. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_factory.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_factory.h:28: void SetReportEncryptionParamsForTesting(uint8_t* server_public_key_, On 2016/12/16 01:55:24, Jialiu Lin wrote: > extra "_" Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:73: // Sets the expected histogram value to be checked during teardown. On 2016/12/16 01:57:59, estark wrote: > nit: maybe make this SetExpectedHistogramFailedCountOnTeardown or something with > "failed" in it somewhere. It's a little confusing down below where it says > SetExpectedHistogramCount(0) but then reports get sent -- not clear that the > histogram only counts failed reports. Changed it to SetExpectedFailedReportCount, added a helper method to the tests with OnTearDown suffix. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:89: int num_expected_failed_report_ = 0; On 2016/12/16 01:57:59, estark wrote: > unsigned int Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:136: public HistogramTestBase { On 2016/12/16 01:57:59, estark wrote: > Instead of the multiple inheritance, could the test fixtures have a > HistogramTestBase? (i.e. so they call histogram_test_->CheckHistogram() where > you're currently calling CheckHistogram()) Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:319: public HistogramTestBase { On 2016/12/16 01:57:59, estark wrote: > same here Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:369: base::Unretained(this))); On 2016/12/16 01:57:59, estark wrote: > This looks a little sketchy to me. I'd think we need to wait, otherwise `this` > could be destroyed before TearDownOnIOThread() runs. > > Needs a WaitForIOThread afterwards, maybe? Done. https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:169: std::unique_ptr<ShutdownSubscription> RegisterShutdownCallback( On 2016/12/16 01:55:24, Jialiu Lin wrote: > Could you add some comments on what thread these will be called and roughly what > kind if task these callbacks perform (either here or where > shutdown_callback_list_ is defined.) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:676: class CertificateReportingServiceCertReporter : public SSLCertReporter { On 2016/12/16 20:26:35, Mustafa Emre Acer wrote: > On 2016/12/16 01:57:58, estark wrote: > > Hrmmmmm. There is not much reason for this SSLCertReporter interface to exist > > anymore. CertReportHelper in chrome/browser/ssl could just using the > > CertificateReportingService directly. > > > > We originally did this SSLCertReporter interface to avoid introducing a > c/b/ssl > > -> c/b/safe_browsing dependency... but now c/b/ssl depends on > c/b/safe_browsing > > all over the place. > > > > So we could get rid of this interface. Or we could leave it as you have it and > > file a bug to clean up the c/b/ssl => c/b/safe_browsing dependency. (I don't > > think it would be that hard to clean it up.) I have a slight preference for > the > > latter, since it is kinda weird that c/b/ssl depends on safebrowsing. Do you > > have any preference? > > I've been avoiding adding more c/b/sb dependency to ssl so I'd prefer cleaning > out the dependencies instead. > > As far as I can tell, the only dependencies are in tests and one other place > which I removed in the next CL > (chrome/browser/ssl/certificate_reporting_test_utils.cc). The rest of the code > in ssl depends on components/safe_browsing_db instead. But maybe I'm missing > something? I think removing the safe_browsing_db ones would be a reasonable thing to do too, if possible. I filed a bug to look into it: https://bugs.chromium.org/p/chromium/issues/detail?id=675245 https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:688: CertificateReportingService* service_; nit: Can you add DISALLOW_COPY_AND_ASSIGN while you're here? https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:22: CertificateReportingServiceFactory::GetForBrowserContext( I am very stupid for not realizing this earlier, but I just realized that this is likely going to be crashy for the same reason that HttpsEngagementMetricsProvider was: https://crbug.com/671579 https://bugs.chromium.org/p/chromium/issues/detail?id=671579#c21 suggests that we should be listening to when profile initialization is complete. Do you have any idea how to do that...? https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:42: : public CertificateReportingService::PreferenceObserver { I'm wondering if these abstractions are needed (the PreferenceObserver interface and the SBPreferenceObserver implementation). Could you instead just give the CertificateReportingService a safe_browsing_state_subscription_ member that calls a CertificateReportingService method as its callback? https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:44: // service. SafeBrowsingService is created before CertificateReportingService, Who/what enforces this? https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:124: scoped_refptr<net::URLRequestContextGetter> url_request_context(); why is this change needed?
no more comments from me. I'll defer to estark@. LGTM https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:62: safe_browsing::IsExtendedReportingEnabled(prefs_); On 2016/12/16 20:26:35, Mustafa Emre Acer wrote: > On 2016/12/16 01:55:24, Jialiu Lin wrote: > > You probably want to check kSafeBrowsingExtendedReportingOptInAllowed as well > in > > case enterprise setting overrides extended reporting preference. > > Would it make sense to add kSafeBrowsingExtendedReportingOptInAllowed check to > IsExtendedReportingEnabled instead of here? Or do we want to keep them separate? Agree. I'll sync up with lpz@ to make the change.
Thanks Jialiu. Some quick comments with no changes yet. https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:22: CertificateReportingServiceFactory::GetForBrowserContext( On 2016/12/16 23:39:04, estark wrote: > I am very stupid for not realizing this earlier, but I just realized that this > is likely going to be crashy for the same reason that > HttpsEngagementMetricsProvider was: https://crbug.com/671579 > > https://bugs.chromium.org/p/chromium/issues/detail?id=671579#c21 suggests that > we should be listening to when profile initialization is complete. Do you have > any idea how to do that...? That's a scary bug. It looks like NOTIFICATION_PROFILE_CREATED is sent after a profile is initialized, but I can't immediately tell if it's sent on a new profile creation or when an existing one is loaded. https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:124: scoped_refptr<net::URLRequestContextGetter> url_request_context(); On 2016/12/16 23:39:04, estark wrote: > why is this change needed? This is so that we can pass the request context between threads when initializing CertificateReportingService. The service is created in the UI thread but its reporter (which uses the request context) is initialized on the IO thread. The alternative is to defer initialization of CertificateReportingService, add an Initialize method to it and pass the URL request context to that method. The Initialize method will need to be called on the IO thread which seemed ugly to me, as the public interface of CertificateReportingService is only called on the UI thread. Also, I should note that CertificateReportingService doesn't hold on to this getter. It gets the URLRequestContext from this getter as a raw pointer and uses it in the IO thread, without adding a reference to url_request_context (which is actually a URLRequestContextGetter and not a URLRequestContext).
https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:688: CertificateReportingService* service_; On 2016/12/16 23:39:04, estark wrote: > nit: Can you add DISALLOW_COPY_AND_ASSIGN while you're here? Done. https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:42: : public CertificateReportingService::PreferenceObserver { On 2016/12/16 23:39:04, estark wrote: > I'm wondering if these abstractions are needed (the PreferenceObserver interface > and the SBPreferenceObserver implementation). Could you instead just give the > CertificateReportingService a safe_browsing_state_subscription_ member that > calls a CertificateReportingService method as its callback? Good point, removed. This used to be created by ChromeContentBrowserClient but since there is no need to do that anymore, we don't need this abstraction. https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:44: // service. SafeBrowsingService is created before CertificateReportingService, On 2016/12/16 23:39:04, estark wrote: > Who/what enforces this? SafeBrowsing is created on first use by the browser main loop through ChromeContentBrowserClient::ResourceDispatcherHostCreated. CertificateReportingService also gets created on first use and that happens when the metrics service uploads a report, or the user sees an SSL error. Both of them happen after ResourceDispatcherHostCreated gets called. https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:124: scoped_refptr<net::URLRequestContextGetter> url_request_context(); On 2016/12/17 01:23:21, Mustafa Emre Acer wrote: > On 2016/12/16 23:39:04, estark wrote: > > why is this change needed? > > This is so that we can pass the request context between threads when > initializing CertificateReportingService. The service is created in the UI > thread but its reporter (which uses the request context) is initialized on the > IO thread. > > The alternative is to defer initialization of CertificateReportingService, add > an Initialize method to it and pass the URL request context to that method. The > Initialize method will need to be called on the IO thread which seemed ugly to > me, as the public interface of CertificateReportingService is only called on the > UI thread. > > Also, I should note that CertificateReportingService doesn't hold on to this > getter. It gets the URLRequestContext from this getter as a raw pointer and uses > it in the IO thread, without adding a reference to url_request_context (which is > actually a URLRequestContextGetter and not a URLRequestContext). I think the current implementation is buggy: There's existing code that's keeping scoped_refptr to his url_request_context. There is a small chance that those could drop all references which would free url_request_context even though it shouldn't be freed. E.g. https://cs.chromium.org/chromium/src/chrome/browser/extensions/blacklist_stat... |url_request_context_getter_| is a scoped_refptr and can end up freeing SafeBrowsingService's url_request_context_getter.
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
estark: PTAL? https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:22: CertificateReportingServiceFactory::GetForBrowserContext( On 2016/12/17 01:23:21, Mustafa Emre Acer wrote: > On 2016/12/16 23:39:04, estark wrote: > > I am very stupid for not realizing this earlier, but I just realized that this > > is likely going to be crashy for the same reason that > > HttpsEngagementMetricsProvider was: https://crbug.com/671579 > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=671579#c21 suggests that > > we should be listening to when profile initialization is complete. Do you have > > any idea how to do that...? > > That's a scary bug. It looks like NOTIFICATION_PROFILE_CREATED is sent after a > profile is initialized, but I can't immediately tell if it's sent on a new > profile creation or when an existing one is loaded. I believe this is fixed now. The fix is based on https://codereview.chromium.org/2531023002
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, just a few comment nits! https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2503243003/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.h:124: scoped_refptr<net::URLRequestContextGetter> url_request_context(); On 2016/12/19 23:16:45, Mustafa Emre Acer wrote: > On 2016/12/17 01:23:21, Mustafa Emre Acer wrote: > > On 2016/12/16 23:39:04, estark wrote: > > > why is this change needed? > > > > This is so that we can pass the request context between threads when > > initializing CertificateReportingService. The service is created in the UI > > thread but its reporter (which uses the request context) is initialized on the > > IO thread. > > > > The alternative is to defer initialization of CertificateReportingService, add > > an Initialize method to it and pass the URL request context to that method. > The > > Initialize method will need to be called on the IO thread which seemed ugly to > > me, as the public interface of CertificateReportingService is only called on > the > > UI thread. > > > > Also, I should note that CertificateReportingService doesn't hold on to this > > getter. It gets the URLRequestContext from this getter as a raw pointer and > uses > > it in the IO thread, without adding a reference to url_request_context (which > is > > actually a URLRequestContextGetter and not a URLRequestContext). > > I think the current implementation is buggy: There's existing code that's > keeping scoped_refptr to his url_request_context. There is a small chance that > those could drop all references which would free url_request_context even though > it shouldn't be freed. > > E.g. > https://cs.chromium.org/chromium/src/chrome/browser/extensions/blacklist_stat... > > |url_request_context_getter_| is a scoped_refptr and can end up freeing > SafeBrowsingService's url_request_context_getter. Acknowledged. https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc (right): https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:99: histogram_tester_.ExpectUniqueSample(kFailedReportHistogram, nit: I think you can get rid of the conditional? My reading of the ExpectUniqueSample() documentation is that ExpectUniqueSample(histogram, blah, 0) is the same as ExpectTotalCount(histogram, blah, 0) https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:245: WaitForIOThread(); nit: this could use a line of explanation, e.g. // Wait for a dummy task on the IO thread to ensure that any // report-sending tasks previously posted to the IO thread have // run (and thus been observed by the interceptor). https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:637: // service should clear its in-flight reports list. Resuming nit: extra word or unfinished sentence?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
meacer@chromium.org changed reviewers: + anthonyvd@chromium.org, isherman@chromium.org
Thanks! isherman: Can you PTAL at chrome/browser/metrics/chrome_metrics_service_client.cc ? anthonyvd: Can you PTAL at chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc ? https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc (right): https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:99: histogram_tester_.ExpectUniqueSample(kFailedReportHistogram, On 2016/12/22 00:11:44, estark wrote: > nit: I think you can get rid of the conditional? > My reading of the ExpectUniqueSample() documentation is that > ExpectUniqueSample(histogram, blah, 0) > is the same as ExpectTotalCount(histogram, blah, 0) Unfortunately no. The histogram isn't created until it gets a sample, so this hits the null check in ExpectUniqueSample. https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:245: WaitForIOThread(); On 2016/12/22 00:11:44, estark wrote: > nit: this could use a line of explanation, e.g. > // Wait for a dummy task on the IO thread to ensure that any > // report-sending tasks previously posted to the IO thread have > // run (and thus been observed by the interceptor). Done. https://codereview.chromium.org/2503243003/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:637: // service should clear its in-flight reports list. Resuming On 2016/12/22 00:11:44, estark wrote: > nit: extra word or unfinished sentence? Done.
I might be missing some context for this CL. Could you please fill me in a bit about what you're trying to do, and how it relates to UMA? https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:32: service->SendPending(); Hmm, this doesn't look like it's actually related to UMA metrics. Am I misunderstanding something here? Why is this being hooked up as a metrics provider? https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:35: UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.CertificateErrorReportFailure", -net_error); Is this a new histogram? Should it be added to histograms.xml?
c/b/profiles/ lgtm
Description was changed from ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes ceritificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. I'll do that in a separate CL. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ========== to ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes certificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. This is done in https://crrev.com/2578873003 Background: The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling of retries itself, it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries queued (pending) report uploads whenever it's asked to provide metrics. This logic is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ==========
https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:32: service->SendPending(); On 2016/12/22 01:41:57, Ilya Sherman (Away De.29-Ja.4) wrote: > Hmm, this doesn't look like it's actually related to UMA metrics. Am I > misunderstanding something here? Why is this being hooked up as a metrics > provider? Sorry, I split this feature into multiple CLs and that caused loss of context. The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling. We wanted to re-use the scheduling logic of MetricsService instead. What happens here is that whenever this metrics provider is asked to provide metrics, the CertificateReportingService tries uploading queued (i.e. pending) reports. This is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. It's not related to UMA at all, the histogram mentioned in the CL is an existing histogram moved from SafeBrowsingPingManager. Hope that helps, I can explain further if needed. https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:35: UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.CertificateErrorReportFailure", -net_error); On 2016/12/22 01:41:57, Ilya Sherman (Away De.29-Ja.4) wrote: > Is this a new histogram? Should it be added to histograms.xml? It's an existing histogram being moved from SafeBrowsingPingManager. Oddly, cs.chromium.org doesn't show its histograms.xml entry but I swear it's there :)
https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:32: service->SendPending(); On 2016/12/22 18:50:04, Mustafa Emre Acer wrote: > On 2016/12/22 01:41:57, Ilya Sherman (Away De.29-Ja.4) wrote: > > Hmm, this doesn't look like it's actually related to UMA metrics. Am I > > misunderstanding something here? Why is this being hooked up as a metrics > > provider? > > Sorry, I split this feature into multiple CLs and that caused loss of context. > > The main feature is to retry uploads of invalid SSL certificate reports. > Currently, the reports are uploaded only once when the user sees an SSL error. > This isn't always sufficient though, as the user might be behind a captive > portal which blocks all connections to the internet. For that reason, this > feature queues failed uploads and retries them at a later time. All of this > logic is in CertificateReportingService. > > However, CertificateReportingService doesn't do any scheduling. We wanted to > re-use the scheduling logic of MetricsService instead. What happens here is that > whenever this metrics provider is asked to provide metrics, the > CertificateReportingService tries uploading queued (i.e. pending) reports. > > This is very much like HttpsEngagementMetricsProvider which also relies on the > metrics service to do the scheduling. It's not related to UMA at all, the > histogram mentioned in the CL is an existing histogram moved from > SafeBrowsingPingManager. > > Hope that helps, I can explain further if needed. Okay, that kind of makes sense. I'm not 100% sure that it's a good idea to piggyback off of the UMA scheduler, but I also can't think of any specific reason why it's a bad idea, so maybe it's okay. Please add documentation to the code, though, to explain that this is what's happening. Otherwise, I could imagine a future reader of this code being quite confused =) https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.cc:35: UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.CertificateErrorReportFailure", -net_error); On 2016/12/22 18:50:04, Mustafa Emre Acer wrote: > On 2016/12/22 01:41:57, Ilya Sherman (Away De.29-Ja.4) wrote: > > Is this a new histogram? Should it be added to histograms.xml? > > It's an existing histogram being moved from SafeBrowsingPingManager. Oddly, > http://cs.chromium.org doesn't show its histograms.xml entry but I swear it's there :) Okay, thanks for the reassurance. And, yeah, codesearch is currently not indexing histograms.xml because the file size is too big :(
https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/290018/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:32: service->SendPending(); On 2016/12/22 22:00:08, Ilya Sherman (Away De.29-Ja.4) wrote: > On 2016/12/22 18:50:04, Mustafa Emre Acer wrote: > > On 2016/12/22 01:41:57, Ilya Sherman (Away De.29-Ja.4) wrote: > > > Hmm, this doesn't look like it's actually related to UMA metrics. Am I > > > misunderstanding something here? Why is this being hooked up as a metrics > > > provider? > > > > Sorry, I split this feature into multiple CLs and that caused loss of context. > > > > The main feature is to retry uploads of invalid SSL certificate reports. > > Currently, the reports are uploaded only once when the user sees an SSL error. > > This isn't always sufficient though, as the user might be behind a captive > > portal which blocks all connections to the internet. For that reason, this > > feature queues failed uploads and retries them at a later time. All of this > > logic is in CertificateReportingService. > > > > However, CertificateReportingService doesn't do any scheduling. We wanted to > > re-use the scheduling logic of MetricsService instead. What happens here is > that > > whenever this metrics provider is asked to provide metrics, the > > CertificateReportingService tries uploading queued (i.e. pending) reports. > > > > This is very much like HttpsEngagementMetricsProvider which also relies on the > > metrics service to do the scheduling. It's not related to UMA at all, the > > histogram mentioned in the CL is an existing histogram moved from > > SafeBrowsingPingManager. > > > > Hope that helps, I can explain further if needed. > > Okay, that kind of makes sense. I'm not 100% sure that it's a good idea to > piggyback off of the UMA scheduler, but I also can't think of any specific > reason why it's a bad idea, so maybe it's okay. Please add documentation to the > code, though, to explain that this is what's happening. Otherwise, I could > imagine a future reader of this code being quite confused =) Sure, expanded the documentation a bit. Hopefully it's clearer now.
Metrics changes LGTM, thanks. https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:19: // metrisc to the metrics service. It doesn't provide any metrics though, nit: s/metrisc/metrics https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.h (right): https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.h:12: // uploads of failed reports. Instead, it piggybacks off on the metrics service nit: s/off on/off of
Thanks everyone! https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc (right): https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.cc:19: // metrisc to the metrics service. It doesn't provide any metrics though, On 2016/12/23 00:56:48, Ilya Sherman (Away De.29-Ja.4) wrote: > nit: s/metrisc/metrics Done. https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_metrics_provider.h (right): https://codereview.chromium.org/2503243003/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_metrics_provider.h:12: // uploads of failed reports. Instead, it piggybacks off on the metrics service On 2016/12/23 00:56:48, Ilya Sherman (Away De.29-Ja.4) wrote: > nit: s/off on/off of Done.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, estark@chromium.org, anthonyvd@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2503243003/#ps330001 (title: "isherman comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, anthonyvd@chromium.org, jialiul@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2503243003/#ps350001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1482867272709320,
"parent_rev": "5b9d19777b56d763aeda94b7f89b796008df616e", "commit_rev":
"aa76c99a97192ee571a4fa9b88f5cff63ba7949c"}
Message was sent while issue was closed.
Description was changed from ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes certificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. This is done in https://crrev.com/2578873003 Background: The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling of retries itself, it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries queued (pending) report uploads whenever it's asked to provide metrics. This logic is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* ========== to ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes certificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. This is done in https://crrev.com/2578873003 Background: The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling of retries itself, it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries queued (pending) report uploads whenever it's asked to provide metrics. This logic is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* Review-Url: https://codereview.chromium.org/2503243003 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes certificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. This is done in https://crrev.com/2578873003 Background: The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling of retries itself, it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries queued (pending) report uploads whenever it's asked to provide metrics. This logic is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* Review-Url: https://codereview.chromium.org/2503243003 ========== to ========== Wire up CertificateReportingService to handle report uploads This CL is a follow up to crrev.com/2543523002. It routes certificate reports through CertificateReportingService instead of SafeBrowsingPingManager. Reporting code in SafeBrowsingPingManager isn't removed yet. This is done in https://crrev.com/2578873003 Background: The main feature is to retry uploads of invalid SSL certificate reports. Currently, the reports are uploaded only once when the user sees an SSL error. This isn't always sufficient though, as the user might be behind a captive portal which blocks all connections to the internet. For that reason, this feature queues failed uploads and retries them at a later time. All of this logic is in CertificateReportingService. However, CertificateReportingService doesn't do any scheduling of retries itself, it relies on MetricsService instead. CertificateReportingServiceMetricsProvider retries queued (pending) report uploads whenever it's asked to provide metrics. This logic is very much like HttpsEngagementMetricsProvider which also relies on the metrics service to do the scheduling. BUG=554323 TEST=CertificateReportingServiceBrowserTest.* Committed: https://crrev.com/f43117a3430e9ec23d2d2329175257fa4d6059f0 Cr-Commit-Position: refs/heads/master@{#440788} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/f43117a3430e9ec23d2d2329175257fa4d6059f0 Cr-Commit-Position: refs/heads/master@{#440788} |
