Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361963002:
Adding NetLog support to SafeBrowsingPingManager. (Closed)
DescriptionAdding NetLog support to SafeBrowsingPingManager.
This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins
when a ping is queued up, and ends when the result callback is fired. Covers
both the SafeBrowsingHit and ThreatDetails pings.
BUG=648323
Committed: https://crrev.com/f69809c4d3c28423bf1c11fce30dfd0360fdd2df
Cr-Commit-Position: refs/heads/master@{#422849}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Segfault fix: call netlog before std::moving the fetcher #
Total comments: 6
Patch Set 3 : Added unit tests. #Patch Set 4 : Addressing review comments. #
Total comments: 2
Patch Set 5 : Base64 encoding the payloads. #
Total comments: 6
Patch Set 6 : Tidy up after final reviews. #
Messages
Total messages: 50 (33 generated)
lpz@chromium.org changed reviewers: + jialiul@chromium.org, nparker@chromium.org, sadrul@chromium.org
Early snapshot of a CL to add NetLog support for ThreatDetails. Just a quick pass to confirm that this change is what we're aiming for. Looking for feedback on whether the start of the ping is an interesting event (as opposed to just the result), whether the SBHit ping is interesting, and whether we want more/different data in the Log. I'll also add unit tests. Thanks for the early feedback!
The CQ bit was checked by lpz@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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/ping_manager.cc:155: hit_report.post_data)); It seems hit_report.post_data could be empty if report is sent via GET. If that's the case, should we at least send something to the netlog? Seems only download_protection_service uses this post_data field of HitReport. https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr...
The CQ bit was checked by lpz@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
With the standard caveat that I am not really familiar with this code: looks reasonable to me. I have a couple of nits, and a query: https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:39: // Return a dictionary with "url"=|url-spec| and "data"=|payload| for *Returns https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:54: // Return a dictionary with "url"=|url-spec|, "status"=|status| and ditto https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:173: report)); Are these callbacks always expected to run synchronously? (because if not, then there may be some lifetime issues here)
The CQ bit was checked by lpz@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...
https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/ping_manager.cc:155: hit_report.post_data)); On 2016/09/22 21:19:13, Jialiu Lin wrote: > It seems hit_report.post_data could be empty if report is sent via GET. If > that's the case, should we at least send something to the netlog? > > Seems only download_protection_service uses this post_data field of HitReport. > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... > Ahh good point. It looks like all the other data in HitReport is used to generate the ping URL so it's represented there (of course in a much less readable form). I could certainly pretty-print that struct into the payload, but wondering if the URL params are good enough for what we need? https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:39: // Return a dictionary with "url"=|url-spec| and "data"=|payload| for On 2016/09/23 16:38:21, sadrul wrote: > *Returns Done. https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:54: // Return a dictionary with "url"=|url-spec|, "status"=|status| and On 2016/09/23 16:38:21, sadrul wrote: > ditto Done. https://codereview.chromium.org/2361963002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:173: report)); On 2016/09/23 16:38:21, sadrul wrote: > Are these callbacks always expected to run synchronously? (because if not, then > there may be some lifetime issues here) Yes the docs suggest that this is guaranteed - the callback will finish before BeginEvent/EndEvent returns (http://dev.chromium.org/developers/design-documents/network-stack/netlog, see "How custom parameters are attached to events").
https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/ping_manager.cc:155: hit_report.post_data)); On 2016/09/23 at 18:11:58, lpz wrote: > On 2016/09/22 21:19:13, Jialiu Lin wrote: > > It seems hit_report.post_data could be empty if report is sent via GET. If > > that's the case, should we at least send something to the netlog? > > > > Seems only download_protection_service uses this post_data field of HitReport. > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... > > > > Ahh good point. It looks like all the other data in HitReport is used to generate the ping URL so it's represented there (of course in a much less readable form). I could certainly pretty-print that struct into the payload, but wondering if the URL params are good enough for what we need? either way is fine as long as human can understand it when displayed in chrome://net-internal
Looks great! LGTM https://codereview.chromium.org/2361963002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:152: report_ptr->GetOriginalURL(), hit_report.post_data)); Is the post data already base64 encoded? It'd be good to see what this looks like. Have you tried it out?: Open chrome://net-internals in one tab, then trigger an interstitial in another, then look at Events in net-internals. You can also use chrome://tracing/ and select the "net-log" category, and then the same info will show up on a timeline plot there.
Thanks for the feedback so far. Note that something about this change is broken (see any of the trybot failures...the error is ERROR:interface_registry.cc(93)] Capability spec prevented service: exe:content_browser from binding interface: visitedlink::mojom::VisitedLinkNotificationSink). I'm digging into it but thought I'd broadcast this in case anybody has an obvious fix in mind.
The CQ bit was checked by lpz@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: This issue passed the CQ dry run.
The CQ bit was checked by lpz@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...
lpz@chromium.org changed reviewers: + rtenneti@chromium.org
rtenneti@chromium.org: Please review changes in net_log_event_type_list.h https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/ping_manager.cc:155: hit_report.post_data)); On 2016/09/23 18:18:25, Jialiu Lin wrote: > On 2016/09/23 at 18:11:58, lpz wrote: > > On 2016/09/22 21:19:13, Jialiu Lin wrote: > > > It seems hit_report.post_data could be empty if report is sent via GET. If > > > that's the case, should we at least send something to the netlog? > > > > > > Seems only download_protection_service uses this post_data field of > HitReport. > > > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... > > > > > > > Ahh good point. It looks like all the other data in HitReport is used to > generate the ping URL so it's represented there (of course in a much less > readable form). I could certainly pretty-print that struct into the payload, but > wondering if the URL params are good enough for what we need? > > either way is fine as long as human can understand it when displayed in > chrome://net-internal It's somewhat usable since you can search within the net-internals window for the url params (from SafeBrowsingHitURL). If it turns out to be too annoying I think we can do a follow-up change to make it nicer for this use case. https://codereview.chromium.org/2361963002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:152: report_ptr->GetOriginalURL(), hit_report.post_data)); On 2016/09/23 18:43:09, Nathan Parker wrote: > Is the post data already base64 encoded? > > It'd be good to see what this looks like. Have you tried it out?: > Open chrome://net-internals in one tab, then trigger an interstitial in another, > then look at Events in net-internals. You can also use chrome://tracing/ and > select the "net-log" category, and then the same info will show up on a timeline > plot there. Good call, encoded both payloads.
lgtm https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/ping_manager.cc:155: hit_report.post_data)); On 2016/09/26 at 19:55:39, lpz wrote: > On 2016/09/23 18:18:25, Jialiu Lin wrote: > > On 2016/09/23 at 18:11:58, lpz wrote: > > > On 2016/09/22 21:19:13, Jialiu Lin wrote: > > > > It seems hit_report.post_data could be empty if report is sent via GET. If > > > > that's the case, should we at least send something to the netlog? > > > > > > > > Seems only download_protection_service uses this post_data field of > > HitReport. > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... > > > > > > > > > > Ahh good point. It looks like all the other data in HitReport is used to > > generate the ping URL so it's represented there (of course in a much less > > readable form). I could certainly pretty-print that struct into the payload, but > > wondering if the URL params are good enough for what we need? > > > > either way is fine as long as human can understand it when displayed in > > chrome://net-internal > > It's somewhat usable since you can search within the net-internals window for the url params (from SafeBrowsingHitURL). If it turns out to be too annoying I think we can do a follow-up change to make it nicer for this use case. SG.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 ========== to ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 ==========
lpz@chromium.org changed reviewers: - rtenneti@chromium.org
lpz@chromium.org changed reviewers: + mef@chromium.org
mef@chromium.org: Please review changes in net_log_event_type_list.h
lgtm
lgtm Drive-by review. Thanks for the good work! https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:147: std::string post_data_base64 = ""; you may be able to leave out the explicit initialization since the default constructor creates an empty string. https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:173: std::string report_base64 = ""; same here
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Optional drive by comment, don't wait for a signoff from me on this CL. https://codereview.chromium.org/2361963002/diff/80001/net/log/net_log_event_t... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2361963002/diff/80001/net/log/net_log_event_t... net/log/net_log_event_type_list.h:3034: // that is being sent (eg: ThreatReport, SafeBrowsingHit)> Is this completely redundant with the corresponding begin phase event? If so, should probably remove it.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
The CQ bit was checked by lpz@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: This issue passed the CQ dry run.
The CQ bit was checked by lpz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, vakh@chromium.org, mef@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2361963002/#ps100001 (title: "Tidy up after final reviews.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 ========== to ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 ========== to ========== Adding NetLog support to SafeBrowsingPingManager. This includes defining a new NetLog event (SAFE_BROWSING_PING), which begins when a ping is queued up, and ends when the result callback is fired. Covers both the SafeBrowsingHit and ThreatDetails pings. BUG=648323 Committed: https://crrev.com/f69809c4d3c28423bf1c11fce30dfd0360fdd2df Cr-Commit-Position: refs/heads/master@{#422849} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f69809c4d3c28423bf1c11fce30dfd0360fdd2df Cr-Commit-Position: refs/heads/master@{#422849}
Message was sent while issue was closed.
https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:147: std::string post_data_base64 = ""; On 2016/09/30 16:05:59, vakh (Varun Khaneja) wrote: > you may be able to leave out the explicit initialization since the default > constructor creates an empty string. Done. https://codereview.chromium.org/2361963002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:173: std::string report_base64 = ""; On 2016/09/30 16:05:59, vakh (Varun Khaneja) wrote: > same here Done. https://codereview.chromium.org/2361963002/diff/80001/net/log/net_log_event_t... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2361963002/diff/80001/net/log/net_log_event_t... net/log/net_log_event_type_list.h:3034: // that is being sent (eg: ThreatReport, SafeBrowsingHit)> On 2016/10/03 15:11:46, mmenke wrote: > Is this completely redundant with the corresponding begin phase event? If so, > should probably remove it. Yes, redundant. Removed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
