Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543523002:
Implement main CertificateReportingService code and add unit tests. (Closed)
DescriptionImplement main CertificateReportingService code and add unit tests.
This CL adds the actual CertificateReportingService code that's going to be
used to send reports. The service is still not wired to any other service, so is
off by default.
BUG=554323
Committed: https://crrev.com/dd133eb62c32783f97cfd7b2b6f83af768461268
Cr-Commit-Position: refs/heads/master@{#438017}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Sacrifice signed integers for Trygods #Patch Set 3 : jialiul comments #
Total comments: 16
Patch Set 4 : Remove EventObserver, check URL request destroyed events #Patch Set 5 : Cleanup #
Total comments: 46
Patch Set 6 : estark comments #Patch Set 7 : Fix unit tests #
Total comments: 10
Patch Set 8 : estark comments #
Messages
Total messages: 48 (28 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...
meacer@chromium.org changed reviewers: + estark@chromium.org, jialiul@chromium.org
jialiul, estark: PTAL? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
(In case you haven't started reviewing, I just uploaded patchset #2 for signed integer conversion, but otherwise you can keep reviewing patchset #1)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm in general with a couple of nits (feel free to ignore). Though several bots are not so happy. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service.cc:178: nullptr /* error_reporter */), Maybe add a TODO here to indicate that "nullptr" need to be changed to wire the actual reporter when everything is ready. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service.h:41: // cannot depend on SafeBrowsing's url request being nit: "available at all times" probably can fit in line 41. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service_factory.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service_factory.cc:32: return nullptr; may be add a TODO? https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:320: EXPECT_EQ(0, report_observer->num_resets()); nit: maybe move the expected value checking part out as a single function such that it can be reused in the next function? https://codereview.chromium.org/2543523002/diff/1/chrome/browser/ssl/cert_rep... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/ssl/cert_rep... chrome/browser/ssl/cert_report_helper.cc:133: LOG(ERROR) << "Failed to serialize certificate report."; maybe remove this LOG(ERROR)?
Thanks! The bots are unhappy because of a null deref. I'm fixing it in https://codereview.chromium.org/2540853005/ since it's an existing problem that's only manifested by this CL. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service.cc:178: nullptr /* error_reporter */), On 2016/11/30 22:18:06, Jialiu Lin wrote: > Maybe add a TODO here to indicate that "nullptr" need to be changed to wire the > actual reporter when everything is ready. Looks like the parameter was just wrong, it should be event_observer_ instead. The tests still pass though, because: - The unittest creates a CertificateReportingService by passing it an observer. - This observer waits for a single Reset event when the service is created. It doesn't wait for Send events. - Since the Reset event is posted by the reply callback below (line 179), passing nullptr to InitializeOnIOThread here doesn't cause any problems (at least for the current test setup) because this parameter is passed down to Reporter (which handles Send events). - Finally, individual test cases that observe Send events set their own event observers, overwriting this one. I'm now passing the proper value. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service.h:41: // cannot depend on SafeBrowsing's url request being On 2016/11/30 22:18:06, Jialiu Lin wrote: > nit: "available at all times" probably can fit in line 41. Done. Also removed last paragraph as this CL doesn't introduce PreferenceObserver class yet (that's coming up in the follow up CL). https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service_factory.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service_factory.cc:32: return nullptr; On 2016/11/30 22:18:06, Jialiu Lin wrote: > may be add a TODO? Done. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:320: EXPECT_EQ(0, report_observer->num_resets()); On 2016/11/30 22:18:06, Jialiu Lin wrote: > nit: maybe move the expected value checking part out as a single function such > that it can be reused in the next function? I actually did this in the follow up CL which I'll send out soon: https://codereview.chromium.org/2503243003/diff/60001/chrome/browser/safe_bro... Would it be okay to go with that? (There is also some refactoring of this code in patchset #2) https://codereview.chromium.org/2543523002/diff/1/chrome/browser/ssl/cert_rep... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/ssl/cert_rep... chrome/browser/ssl/cert_report_helper.cc:133: LOG(ERROR) << "Failed to serialize certificate report."; On 2016/11/30 22:18:07, Jialiu Lin wrote: > maybe remove this LOG(ERROR)? estark: I think this is your LOG. Is it okay to remove it?
lgtm defer to estark@ for additional comments. https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2543523002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:320: EXPECT_EQ(0, report_observer->num_resets()); On 2016/11/30 at 23:39:40, Mustafa Emre Acer wrote: > On 2016/11/30 22:18:06, Jialiu Lin wrote: > > nit: maybe move the expected value checking part out as a single function such > > that it can be reused in the next function? > > I actually did this in the follow up CL which I'll send out soon: https://codereview.chromium.org/2503243003/diff/60001/chrome/browser/safe_bro... > > Would it be okay to go with that? (There is also some refactoring of this code in patchset #2) Sure. I didn't review patchset#2. Sorry about that.
(Couldn't digest the whole thing at end of day with tired brain, so I mostly focused on the .h and tests) I'm still kinda confused about the EventObserver thing, it really feels like it's testing the implementation rather than the interface, and also prone to breakage. Particularly for the report-sending parts of it, why can't we use a URLRequestInterceptor and test that it a request is intercepted or not-intercepted? When you had this in an earlier CL you said that it was useful for matching up reports to report ids... if you maintain a guarantee that report ids start at 0 and increase from there, could you match reports to report ids from their content (i.e. report with id 0 has body "report0", etc.)? https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:110: class Reporter { Why does this need to be its own class? https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:144: // the next |SendPending| call. very tiny nit: |blah| is only supposed to be used for variables IIRC, so |SendPending| => SendPending() https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:148: // If true, retries are enabled. nit: I think you could remove this comment, doesn't add much beyond the variable name https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:173: // retried in a future time. nit: in => at https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:196: // URL to upload invalid certificate chain reports. An HTTP URL is nit: I don't think this detail about the HTTP URL is necessary for the public header. Maybe drop the |Insecure| from the name, and move this comment into the .cc where you set it to an http:// URL? https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:228: // scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; should be deleted? https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:237: safe_browsing_service_shutdown_subscription_; this is not hooked up yet, correct? (no change needed, just wanting to make sure I understand) https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_cert_reporter.h (right): https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_cert_reporter.h:22: virtual void OnDidNotSendReport() = 0; It looks like this only has empty overrides, are you planning to use this later?
On 2016/12/01 01:46:54, estark wrote: > (Couldn't digest the whole thing at end of day with tired brain, so I mostly > focused on the .h and tests) > > I'm still kinda confused about the EventObserver thing, it really feels like > it's testing the implementation rather than the interface, and also prone to > breakage. Particularly for the report-sending parts of it, why can't we use a > URLRequestInterceptor and test that it a request is intercepted or > not-intercepted? When you had this in an earlier CL you said that it was useful > for matching up reports to report ids... if you maintain a guarantee that report > ids start at 0 and increase from there, could you match reports to report ids > from their content (i.e. report with id 0 has body "report0", etc.)? You are right, EventObserver indeed partially tests the implementation. There are a couple of reasons for this: - Most of the operations are done across threads, so we need to wait for them to complete before testing them. E.g. we need to wait for service initialization to complete before sending events (this is the main reason I think) - When opt-in is toggled, the service should reset itself and clean the queue. The interceptor doesn't get any requests when that happens, so it can't observe unexpected reports. As an example: If a report with id = 0 is in the retry queue and the user opts out and then opts in, the next SendPending call will send a report with id = 0. But it's not possible to know this is/isn't the same report before the opt in toggle by just keeping a count of the report ids. Observing for a reset event assures that the service properly cleans up the queue. (there might be more reasons that I can't remember at the end of the day. I'll take another look tomorrow)
On 2016/12/01 02:08:23, Mustafa Emre Acer wrote: > On 2016/12/01 01:46:54, estark wrote: > > (Couldn't digest the whole thing at end of day with tired brain, so I mostly > > focused on the .h and tests) > > > > I'm still kinda confused about the EventObserver thing, it really feels like > > it's testing the implementation rather than the interface, and also prone to > > breakage. Particularly for the report-sending parts of it, why can't we use a > > URLRequestInterceptor and test that it a request is intercepted or > > not-intercepted? When you had this in an earlier CL you said that it was > useful > > for matching up reports to report ids... if you maintain a guarantee that > report > > ids start at 0 and increase from there, could you match reports to report ids > > from their content (i.e. report with id 0 has body "report0", etc.)? > > You are right, EventObserver indeed partially tests the implementation. There > are > a couple of reasons for this: > > - Most of the operations are done across threads, so we need to wait for them to > complete > before testing them. E.g. we need to wait for service initialization to complete > before > sending events (this is the main reason I think) > > - When opt-in is toggled, the service should reset itself and clean the queue. > The > interceptor doesn't get any requests when that happens, so it can't observe > unexpected > reports. As an example: If a report with id = 0 is in the retry queue and the > user > opts out and then opts in, the next SendPending call will send a report with id > = 0. > But it's not possible to know this is/isn't the same report before the opt in > toggle > by just keeping a count of the report ids. Observing for a reset event assures > that > the service properly cleans up the queue. > > (there might be more reasons that I can't remember at the end of the day. I'll > take > another look tomorrow) Btw, I'm looking at removing EventObserver. I'll report back here.
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...
estark: I removed event observed. Can you PTAL and see if this approach works? (I'll resolve your remaining comments in a follow up CL) https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:110: class Reporter { On 2016/12/01 01:46:53, estark wrote: > Why does this need to be its own class? I think there is better separation this way. In particular: - I can unit test the Reporter class independent of the Service itself. The previous CL only implemented the reporter and had a unittest for it without having the service. - The reporter gets recreated each time SB preferences change, clearing the queue and pending requests. - The reporter lives in the IO thread so it's a bit easier to reason about. The service just calls reporter methods on the right thread. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_cert_reporter.h (right): https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_cert_reporter.h:22: virtual void OnDidNotSendReport() = 0; On 2016/12/01 01:46:54, estark wrote: > It looks like this only has empty overrides, are you planning to use this later? Removed.
Yaaaaaaaay no more EventObserver!!! Thanks for doing this, the unit tests are really easy to read. I left some comments in addition to my previous ones, but it's mostly nits. The main thing is that there are some test assertions that are commented out, not sure if they're supposed to be deleted or uncommented. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.cc:160: base::Unretained(this), enabled_, url_request_context_getter, Are we sure it's safe to pass this as Unretained? https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.cc:182: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); redundant with line 177 https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:44: // initialization, and this service shuts down when SafeBrowsing shuts down. nit: "this service" gets a little confusing with all these services floating around, maybe replace "this service" with CertificateReportingService here and the line above https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:209: // scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; don't forget to delete https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:61: CertificateReportingServiceTestNetworkDelegate:: nit: I think the order of things here doesn't match the declaration order in the header file https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:75: base::Bind(url_request_destroyed_callback_, request)); Is it safe to pass URLRequests to the UI thread? ... after reading below, it looks like you don't even use it, so maybe you can just get rid of the URLRequest argument https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:104: net::HttpResponseInfo* info) {} nit: maybe put a comment here explaining that the report server sends empty responses? https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:290: CertificateReportingServiceTestBase::ReportExpectation::Successful( Oh, cool, I've never seen this pattern in chrome before. I guess it's so you can write `expectation.Successful(successful).Failed(failed)` or something like that? https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:329: NOTREACHED() << "Observed unexpected report"; nit: maybe make this an ASSERT_LE above line 319 https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:28: REPORTS_SUCCESSFUL = 0, nit: explicit values aren't necessary (unless you're histogramming or serializing the enum or something, which I'm guessing you're not) https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:35: // A URLRequestJob that serves valid time server responses, but delays time server => report server I can tell where you got this code from :P https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:44: base::WeakPtr<DelayableCertReportURLRequestJob> GetWeakPtr() { nit: maybe don't inline this since nothing else is inlined https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:69: // class is used by the tests; one for failed reports and one for successful nit: is => are https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:117: class CertificateReportingServiceTestNetworkDelegate document https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:132: class CertificateReportingServiceTestBase { document https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:138: struct ReportExpectation { document plz (In particular I can't figure out from this declaration what the Successful()/Failed()/Delayed() methods do) https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:163: void WaitForRequestDeletions(const ReportExpectation& expectation); nit: document that this handles the case where all the expected requests have already been deleted before this is called https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:29: void ClearURLHandlers() { nit: blank line above https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:35: const size_t kMaxReportCountInQueue = 3; nit: I think constants are supposed to go first, above ClearURLHandlers() https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:306: scoped_refptr<base::ThreadTestHelper> io_helper( Oh, so this is a pre-existing thing that does the dummy task thing we were talking about? Cool! https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:350: WaitForRequestDeletions(ReportExpectation().Failed({"report0"})); Nice, this is super readable https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:386: // EXPECT_TRUE(observed_reports().empty()); delete? or uncomment? https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:459: // EXPECT_TRUE(observed_reports().empty()); same
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...
Resolved all comments and had to do a rebase. jialiul, estark: PTAL both? https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:144: // the next |SendPending| call. On 2016/12/01 01:46:53, estark wrote: > very tiny nit: |blah| is only supposed to be used for variables IIRC, so > |SendPending| => SendPending() Done. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:148: // If true, retries are enabled. On 2016/12/01 01:46:53, estark wrote: > nit: I think you could remove this comment, doesn't add much beyond the variable > name Done. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:173: // retried in a future time. On 2016/12/01 01:46:53, estark wrote: > nit: in => at Done. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:196: // URL to upload invalid certificate chain reports. An HTTP URL is On 2016/12/01 01:46:53, estark wrote: > nit: I don't think this detail about the HTTP URL is necessary for the public > header. Maybe drop the |Insecure| from the name, and move this comment into the > .cc where you set it to an http:// URL? Done. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:228: // scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; On 2016/12/01 01:46:53, estark wrote: > should be deleted? Done. https://codereview.chromium.org/2543523002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:237: safe_browsing_service_shutdown_subscription_; On 2016/12/01 01:46:53, estark wrote: > this is not hooked up yet, correct? (no change needed, just wanting to make sure > I understand) Yes, removed from this CL. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.cc:160: base::Unretained(this), enabled_, url_request_context_getter, On 2016/12/07 00:42:55, estark wrote: > Are we sure it's safe to pass this as Unretained? CertificateReportingService is only destructed during browser shutdown, so in practice this should be safe. The metrics service that causes this constructor to be called also shuts down during browser shutdown. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.cc:182: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/12/07 00:42:55, estark wrote: > redundant with line 177 Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:44: // initialization, and this service shuts down when SafeBrowsing shuts down. On 2016/12/07 00:42:55, estark wrote: > nit: "this service" gets a little confusing with all these services floating > around, maybe replace "this service" with CertificateReportingService here and > the line above Done, and removed the last sentence for now. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service.h:209: // scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; On 2016/12/07 00:42:55, estark wrote: > don't forget to delete Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:61: CertificateReportingServiceTestNetworkDelegate:: On 2016/12/07 00:42:55, estark wrote: > nit: I think the order of things here doesn't match the declaration order in the > header file Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:75: base::Bind(url_request_destroyed_callback_, request)); On 2016/12/07 00:42:55, estark wrote: > Is it safe to pass URLRequests to the UI thread? > > ... > > > after reading below, it looks like you don't even use it, so maybe you can just > get rid of the URLRequest argument Removed the request. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:104: net::HttpResponseInfo* info) {} On 2016/12/07 00:42:55, estark wrote: > nit: maybe put a comment here explaining that the report server sends empty > responses? Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:290: CertificateReportingServiceTestBase::ReportExpectation::Successful( On 2016/12/07 00:42:55, estark wrote: > Oh, cool, I've never seen this pattern in chrome before. I guess it's so you can > write `expectation.Successful(successful).Failed(failed)` or something like > that? Yes, but the unit tests only check for a single type so I made these static constructors instead. I'll bring this pattern back if I need it in the browser tests. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:329: NOTREACHED() << "Observed unexpected report"; On 2016/12/07 00:42:55, estark wrote: > nit: maybe make this an ASSERT_LE above line 319 Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:28: REPORTS_SUCCESSFUL = 0, On 2016/12/07 00:42:55, estark wrote: > nit: explicit values aren't necessary (unless you're histogramming or > serializing the enum or something, which I'm guessing you're not) Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:35: // A URLRequestJob that serves valid time server responses, but delays On 2016/12/07 00:42:55, estark wrote: > time server => report server > > I can tell where you got this code from :P I tried to erase all traces but apparently failed :p https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:44: base::WeakPtr<DelayableCertReportURLRequestJob> GetWeakPtr() { On 2016/12/07 00:42:55, estark wrote: > nit: maybe don't inline this since nothing else is inlined Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:69: // class is used by the tests; one for failed reports and one for successful On 2016/12/07 00:42:55, estark wrote: > nit: is => are Old comment, removed. There is a single instance of this class. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:117: class CertificateReportingServiceTestNetworkDelegate On 2016/12/07 00:42:55, estark wrote: > document Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:132: class CertificateReportingServiceTestBase { On 2016/12/07 00:42:55, estark wrote: > document Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:138: struct ReportExpectation { On 2016/12/07 00:42:55, estark wrote: > document plz > (In particular I can't figure out from this declaration what the > Successful()/Failed()/Delayed() methods do) Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:163: void WaitForRequestDeletions(const ReportExpectation& expectation); On 2016/12/07 00:42:55, estark wrote: > nit: document that this handles the case where all the expected requests have > already been deleted before this is called Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc (right): https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:29: void ClearURLHandlers() { On 2016/12/07 00:42:56, estark wrote: > nit: blank line above Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:35: const size_t kMaxReportCountInQueue = 3; On 2016/12/07 00:42:55, estark wrote: > nit: I think constants are supposed to go first, above ClearURLHandlers() Done. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:306: scoped_refptr<base::ThreadTestHelper> io_helper( On 2016/12/07 00:42:56, estark wrote: > Oh, so this is a pre-existing thing that does the dummy task thing we were > talking about? Cool! Yep! Not yet sure if it works in browser tests though. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:350: WaitForRequestDeletions(ReportExpectation().Failed({"report0"})); On 2016/12/07 00:42:56, estark wrote: > Nice, this is super readable Great! https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:386: // EXPECT_TRUE(observed_reports().empty()); On 2016/12/07 00:42:56, estark wrote: > delete? or uncomment? Removed, not needed. https://codereview.chromium.org/2543523002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc:459: // EXPECT_TRUE(observed_reports().empty()); On 2016/12/07 00:42:55, estark wrote: > same Removed. TearDown already checks this.
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_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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm with a few more nits (sorry I didn't notice them earlier) https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:28: class URLRequestContextGetter; not needed, you're #include'ing it above https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:161: void DidAttemptSend(bool sent); not used/implemented? https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:164: Reporter* get_reporter_for_testing() const; GetReporterForTesting https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:172: static const char kExtendedReportingUploadUrl[]; I think this is only used in tests; if so, could you move it into the .cc and only access it via the GetReportingURLForTesting() method above? https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h (right): https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:127: // Base class for CertificateReportingService tests. Sets up an intercepto to typo: interceptor
Thanks! Jialiu: Still lgty? https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service.h (right): https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:28: class URLRequestContextGetter; On 2016/12/09 00:49:16, estark wrote: > not needed, you're #include'ing it above Done. https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:161: void DidAttemptSend(bool sent); On 2016/12/09 00:49:16, estark wrote: > not used/implemented? Done. https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:164: Reporter* get_reporter_for_testing() const; On 2016/12/09 00:49:16, estark wrote: > GetReporterForTesting Done. https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service.h:172: static const char kExtendedReportingUploadUrl[]; On 2016/12/09 00:49:16, estark wrote: > I think this is only used in tests; if so, could you move it into the .cc and > only access it via the GetReportingURLForTesting() method above? Done. https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h (right): https://codereview.chromium.org/2543523002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:127: // Base class for CertificateReportingService tests. Sets up an intercepto to On 2016/12/09 00:49:16, estark wrote: > typo: interceptor Done.
LGTM. And sorry for my late reply.
No worries, thanks!
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2543523002/#ps140001 (title: "estark 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: 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
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": 140001, "attempt_start_ts": 1481588325613450,
"parent_rev": "8d9e3f90b74aed24ded610790c1024d38d6435a8", "commit_rev":
"368044b385aeb5c4a8f3d8ff6325447dcf248a7b"}
Message was sent while issue was closed.
Description was changed from ========== Implement main CertificateReportingService code and add unit tests. This CL adds the actual CertificateReportingService code that's going to be used to send reports. The service is still not wired to any other service, so is off by default. BUG=554323 ========== to ========== Implement main CertificateReportingService code and add unit tests. This CL adds the actual CertificateReportingService code that's going to be used to send reports. The service is still not wired to any other service, so is off by default. BUG=554323 Review-Url: https://codereview.chromium.org/2543523002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Implement main CertificateReportingService code and add unit tests. This CL adds the actual CertificateReportingService code that's going to be used to send reports. The service is still not wired to any other service, so is off by default. BUG=554323 Review-Url: https://codereview.chromium.org/2543523002 ========== to ========== Implement main CertificateReportingService code and add unit tests. This CL adds the actual CertificateReportingService code that's going to be used to send reports. The service is still not wired to any other service, so is off by default. BUG=554323 Committed: https://crrev.com/dd133eb62c32783f97cfd7b2b6f83af768461268 Cr-Commit-Position: refs/heads/master@{#438017} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dd133eb62c32783f97cfd7b2b6f83af768461268 Cr-Commit-Position: refs/heads/master@{#438017} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
