|
|
Created:
4 years ago by Jialiu Lin Modified:
4 years ago CC:
asvitkine+watch_chromium.org, chromium-reviews, grt+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd management related code to SafeBrowsingNavigationObserverManager
class:
(1) Schedule clean up stale navigation footprint
(2) Backtrace navigation events up to 2 user gesture for a given
download, and add this info into ClientDownloadRequest proto.
(3) Add UMA metrices to track attribution results
BUG=639467
Committed: https://crrev.com/4e2ebcc9087c02598a638aa86cba21c043069b76
Cr-Commit-Position: refs/heads/master@{#438745}
Patch Set 1 #Patch Set 2 : rebase update #Patch Set 3 : rebase-update #Patch Set 4 : filter out browser side navigation browser tests #
Total comments: 51
Patch Set 5 : address nparker's comments #Patch Set 6 : rebase-update #
Total comments: 82
Patch Set 7 : address comments from creis@ and holte@ + rebase #
Total comments: 12
Patch Set 8 : address nits #Dependent Patchsets: Messages
Total messages: 70 (46 generated)
The CQ bit was checked by jialiul@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 jialiul@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...
jialiul@chromium.org changed reviewers: + nparker@chromium.org
Hi nparker@, Here is the second part of SafeBrowsingNavigationObserverManager code. PTAL. Thanks!
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_...)
The CQ bit was checked by jialiul@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_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 jialiul@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.
Sorry for the delay -- I shoulda been on this earlier. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:198: for (auto it = navigation_map()->begin(); it != navigation_map()->end(); nit: Can you do for (auto it : navigation_map()) ? https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:299: ASSERT_EQ(std::size_t(2), nav_map->size()); nit: 2ull might work if std::size(..) gets annoying https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:619: auto referrer_chain = IdentifyReferrerChain(GetDownload()); Nit: for readability, maybe put a blank line between the VerifyNavigationEntry() section and VerifyReferrerChainEntry()'s section, and same below. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:48: UMA_HISTOGRAM_COUNTS_100(metric_name, size); Does this work? I thought most of the UMA histogram macros couldn't accept non-literal histogram strings. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:73: static const double kNavigationFootPrintTTLInSecond = 120.0; Add a comment describing how this is intended to be used. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:166: std::string attribution_type(kDownloadAttributionUMASuffix); const. Or even just const char* attribution_type = kDownloadAttributionUMASuffix; to avoid a temporary. Maybe the compiler will do the right thing either way. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:173: download_url, download_tab_id, 2, &attribution_chain); What is the 2? Add a comment, or make it a constant. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:177: (*request->add_referrer_chain_entry()) = entry; You shouldn't need the parens. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:240: // Remove any stale NavigationEnvent, if it lasts longer than nit: NavigationEvent. And do you mean "lasts longer" or "is older than?" https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:251: it = navigation_map_.erase(it); This is clever -- avoids needing a second pass. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:302: // Since navigation events are recorded in chronological order, we traverse Is there any way someone could confuse this by adding a redundant navigation _after_ the bad navigation that would mask it? I'm not sure I know what I'm talking about, so tell me if this doesn't make sense. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:305: // If tab id is not valid, we only compare url, otherwise we compare both. I'm curious: When/why would tab_id be invalid? https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:32: enum AttributionResult { This can be private. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:33: SUCCESS, // Identified referrer chain is not empty. Assign numbers to these so you can verify they match those in UMA. I'd start with 1 to detect when it's accidentally set to the default. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:66: void CleanUpStaleNavigationFootPrints(); nit: I'd say Footprint (lowercase P) since it's one word https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:111: void CleanUpNavigationEvents(); Add some comments as to what these do. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:123: void AddToReferrerChain( An idea: Would it be cleaner to have the code that fills in the ClientDownloadRequest be external to this class, and have it just use public methods? That splits it apart into, a) code the tracks attributions (this class) and, b) code that maps it to other structs. We could then add more functions for filling in ClientSafeBrowsingReqportRequest, without adding complexity here. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:171: base::Time::FromDoubleT(now.ToDoubleT() - 60.0 * 60.0); // Stale What's the difference between "Stale" and "Expired?" https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:385: message ReferrerChainEntry { This doesn't look like a backward-compatible change. Is that the intention? Another idea: we could add "ReferrerChainEntry referrer_chain_entry=37" and keep "URLChainEntry urlchain=36" and continue to populate the latter until the backend code no longer looks at it. Or just ensure it's backward compatible, and plan to do a big (mechanical) renaming of things in Google3 so it matches. https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:4: // Click on a link by id to star a test case. "to start" https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:7: if (node != null) { nit: Is there any case when this could be null? If not, you could skip this check. https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... File chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:122: <a id="complete_referer_chain" href="redirect_to_landing.html"> nit: complete_referrer_chain (unless you're trying to match the header name) https://codereview.chromium.org/2538483002/diff/60001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/60001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.NewTabDownload Why do these need to be disabled? Should we add a bug to reenable them? https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54496: +<histogram name="SB2.ReferrerAttributionResult" I'm trying to not put anything new+substantial in the "SB2" prefix, and instead using "SafeBrowsing" prefix. https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54500: + The result of referrer attribution, including different types of success or Add some notes to both on _when_ this is incremented -- e.g. when a download is completed, when a SB interstitial is shown (or is it just when a ping is generated?). Maybe these extra desc's should go down in the <suffix>'s... I think you can add summaries to those (?).
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Thanks, nparker@! Let me know if you have further comments. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:198: for (auto it = navigation_map()->begin(); it != navigation_map()->end(); On 2016/12/05 22:21:23, Nathan Parker wrote: > nit: Can you do > for (auto it : navigation_map()) ? Ack. That needs a copy constructor for NavigationEvent. But since NavigationEvent has a move constructor, copy constructor is implicitly deleted. Thus, it is easier to use explicit iterator instead. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:299: ASSERT_EQ(std::size_t(2), nav_map->size()); On 2016/12/05 22:21:23, Nathan Parker wrote: > nit: 2ull might work if std::size(..) gets annoying Good to know. Thanks! https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:48: UMA_HISTOGRAM_COUNTS_100(metric_name, size); On 2016/12/05 22:21:23, Nathan Parker wrote: > Does this work? I thought most of the UMA histogram macros couldn't accept > non-literal histogram strings. You're right... this would not work. Change to literals instead. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:73: static const double kNavigationFootPrintTTLInSecond = 120.0; On 2016/12/05 22:21:24, Nathan Parker wrote: > Add a comment describing how this is intended to be used. Done. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:166: std::string attribution_type(kDownloadAttributionUMASuffix); On 2016/12/05 22:21:23, Nathan Parker wrote: > const. > > Or even just > const char* attribution_type = kDownloadAttributionUMASuffix; > to avoid a temporary. Maybe the compiler will do the right thing either way. no longer using this variable. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:173: download_url, download_tab_id, 2, &attribution_chain); On 2016/12/05 22:21:23, Nathan Parker wrote: > What is the 2? Add a comment, or make it a constant. Added a const https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:177: (*request->add_referrer_chain_entry()) = entry; On 2016/12/05 22:21:23, Nathan Parker wrote: > You shouldn't need the parens. Done. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:240: // Remove any stale NavigationEnvent, if it lasts longer than On 2016/12/05 22:21:24, Nathan Parker wrote: > nit: NavigationEvent. > > And do you mean "lasts longer" or "is older than?" is older than... https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:251: it = navigation_map_.erase(it); On 2016/12/05 22:21:24, Nathan Parker wrote: > This is clever -- avoids needing a second pass. Acknowledged. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:302: // Since navigation events are recorded in chronological order, we traverse On 2016/12/05 22:21:23, Nathan Parker wrote: > Is there any way someone could confuse this by adding a redundant navigation > _after_ the bad navigation that would mask it? I'm not sure I know what I'm > talking about, so tell me if this doesn't make sense. creis@ and I talked about this before. a -> b->c->download -> e -> f -> c When we search backward, we might get download-c-f-e-a. But assuming the bad guy cannot spoof a user gesture, we can still get a as the landing page, b or e,f are not so important. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:305: // If tab id is not valid, we only compare url, otherwise we compare both. On 2016/12/05 22:21:23, Nathan Parker wrote: > I'm curious: When/why would tab_id be invalid? I never see this happens in real settings, but it happens sometimes during tests (flakiness? maybe) Not sure why. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:32: enum AttributionResult { On 2016/12/05 22:21:24, Nathan Parker wrote: > This can be private. Since I'm going to make AttributionResult IdentifyReferrerChain(...) public, I'll probably keep this enum public too.(such that other classes e.g. SafeBrowsingService can access this enum). https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:33: SUCCESS, // Identified referrer chain is not empty. On 2016/12/05 22:21:24, Nathan Parker wrote: > Assign numbers to these so you can verify they match those in UMA. I'd start > with 1 to detect when it's accidentally set to the default. Done. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:66: void CleanUpStaleNavigationFootPrints(); On 2016/12/05 22:21:24, Nathan Parker wrote: > nit: I'd say Footprint (lowercase P) since it's one word Oops. my spelling... Thanks for catching this. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:111: void CleanUpNavigationEvents(); On 2016/12/05 22:21:24, Nathan Parker wrote: > Add some comments as to what these do. Done. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:123: void AddToReferrerChain( On 2016/12/05 22:21:24, Nathan Parker wrote: > An idea: Would it be cleaner to have the code that fills in the > ClientDownloadRequest be external to this class, and have it just use public > methods? That splits it apart into, a) code the tracks attributions (this > class) and, b) code that maps it to other structs. We could then add more > functions for filling in ClientSafeBrowsingReqportRequest, without adding > complexity here. I'm thinking about move ReferrerChainEntry message out of ClientDownloadRequest and directly put it under safe_browsing namespace, such that ClientSafeBrowsingReportRequest can also reuse this message more easily later. Also make IdentifyReferrerChain() public, and add a todo to AddReferrerChainToClientDownloadRequest(..) function. The later one will be moved to download protection service when it is ready. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:171: base::Time::FromDoubleT(now.ToDoubleT() - 60.0 * 60.0); // Stale On 2016/12/05 22:21:24, Nathan Parker wrote: > What's the difference between "Stale" and "Expired?" oops, I should kept them consistent. https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:385: message ReferrerChainEntry { On 2016/12/05 22:21:24, Nathan Parker wrote: > This doesn't look like a backward-compatible change. Is that the intention? > > Another idea: we could add "ReferrerChainEntry referrer_chain_entry=37" and keep > "URLChainEntry urlchain=36" and continue to populate the latter until the > backend code no longer looks at it. > > Or just ensure it's backward compatible, and plan to do a big (mechanical) > renaming of things in Google3 so it matches. So far, backend is not using field 36. So I guess it is still safe to make these changes. I'll make sure google3 proto updated before I land this CL. https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:4: // Click on a link by id to star a test case. On 2016/12/05 22:21:24, Nathan Parker wrote: > "to start" Done. https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:7: if (node != null) { On 2016/12/05 22:21:24, Nathan Parker wrote: > nit: Is there any case when this could be null? If not, you could skip this > check. Sure. sounds good. https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... File chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/test/data/safe_b... chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:122: <a id="complete_referer_chain" href="redirect_to_landing.html"> On 2016/12/05 22:21:24, Nathan Parker wrote: > nit: complete_referrer_chain (unless you're trying to match the header name) Thanks for catching this. Done. https://codereview.chromium.org/2538483002/diff/60001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/60001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.NewTabDownload On 2016/12/05 22:21:24, Nathan Parker wrote: > Why do these need to be disabled? Should we add a bug to reenable them? As we discussed in person, I'll remove these tests if IsRendererInitiated() function gets fixed. https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54496: +<histogram name="SB2.ReferrerAttributionResult" On 2016/12/05 22:21:24, Nathan Parker wrote: > I'm trying to not put anything new+substantial in the "SB2" prefix, and instead > using "SafeBrowsing" prefix. Done. https://codereview.chromium.org/2538483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54500: + The result of referrer attribution, including different types of success or On 2016/12/05 22:21:24, Nathan Parker wrote: > Add some notes to both on _when_ this is incremented -- e.g. when a download is > completed, when a SB interstitial is shown (or is it just when a ping is > generated?). Maybe these extra desc's should go down in the <suffix>'s... I > think you can add summaries to those (?). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jialiul@chromium.org changed reviewers: + creis@chromium.org
Hi creis@, Here is the follow-up CL of SafeBrowsingNavigationObserverManager. Let me know if you have any comments. Thanks!
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
LGTM from my point of view. Handing off to you, Charlie. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:123: void AddToReferrerChain( On 2016/12/06 23:10:06, Jialiu Lin wrote: > On 2016/12/05 22:21:24, Nathan Parker wrote: > > An idea: Would it be cleaner to have the code that fills in the > > ClientDownloadRequest be external to this class, and have it just use public > > methods? That splits it apart into, a) code the tracks attributions (this > > class) and, b) code that maps it to other structs. We could then add more > > functions for filling in ClientSafeBrowsingReqportRequest, without adding > > complexity here. > > I'm thinking about move ReferrerChainEntry message out of ClientDownloadRequest > and directly put it under safe_browsing namespace, such that > ClientSafeBrowsingReportRequest can also reuse this message more easily later. > Also make IdentifyReferrerChain() public, and add a todo to > AddReferrerChainToClientDownloadRequest(..) function. The later one will be > moved to download protection service when it is ready. SGTM https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/csd.proto:385: message ReferrerChainEntry { On 2016/12/06 23:10:06, Jialiu Lin wrote: > On 2016/12/05 22:21:24, Nathan Parker wrote: > > This doesn't look like a backward-compatible change. Is that the intention? > > > > Another idea: we could add "ReferrerChainEntry referrer_chain_entry=37" and > keep > > "URLChainEntry urlchain=36" and continue to populate the latter until the > > backend code no longer looks at it. > > > > Or just ensure it's backward compatible, and plan to do a big (mechanical) > > renaming of things in Google3 so it matches. > So far, backend is not using field 36. So I guess it is still safe to make these > changes. > I'll make sure google3 proto updated before I land this CL. > Ah great then if it's not being used there's no issue.
The CQ bit was checked by jialiul@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...)
jialiul@chromium.org changed reviewers: + charlie reis,holte@chromium.org - creis@chromium.org
+holte@chromium.org for histograms.xml
Description was changed from ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 ========== to ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 ==========
creis@chromium.org changed reviewers: + creis@chromium.org, holte@chromium.org - charlie reis,holte@chromium.org
Sure, I'll look soon-- I was OOO for a team offsite, but catching up now.
Thanks--generally looks in line with what we discussed. I continue to be a bit concerned about the performance impact when this gets switched on (both memory consumption and cost of periodic cleanup). Is this something you could do a Finch trial for to see if it will lead to performance differences for users in the wild? Finch Chirps are pretty good at noticing such differences. Also, I'm not familiar with proto files, so I'll rely on you to make sure that's reviewed. Side note: Looks like there may be some overlap between my questions and earlier discussions. In general, it can often help to add comments to the CL if there's something unclear to a reviewer, since other people may have the same question. :) https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:140: // For browser initiated navigation (e.g. from address bar or bookmark), we Sounds right. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:216: if (!details.url.host().empty()) Style nit: Needs braces, since body is more than one line. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:174: // This function needs javascript support from test pages. Let's elaborate a bit on what it needs, so that others can add tests using it. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:256: observer_manager_->IdentifyReferrerChain( Please include an EXPECT_EQ for the return value. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:257: download->GetURL(), download_tab_id, 2, &referrer_chain); Let's document that 2 is the user gesture limit. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:266: ASSERT_EQ(1ULL, actual_host_ip_map->size()); I'm curious, is there a reason to use ULL here? I've seen U in a bunch of places, but ULL seems rarer to me. Maybe I just haven't seen the cases that need it? https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:319: auto referrer_chain = IdentifyReferrerChain(GetDownload()); nit: Blank line before (same in all tests). This is a separate block verifying the referrer chain. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:1353: // Click three links before reach download. nit: reaching https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:29: const char kDownloadAttributionResultUMA[] = These shouldn't be constants. Just use the strings directly in the histogram macro. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:54: // navigation related records that happened 2 minutes ago is considered as nit: s/is/are/ https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { This will be running every 2 minutes, right? Please check in with rschoen@ or someone from the performance team to see if there are best practices for this, to avoid unnecessary idle wakeups, etc. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:166: // Back trace to the next nav_event that initiated by user. nit: that was initiated by the user https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:178: user_gesture_count++; nit: Blank line before and after. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:180: // is empty, and is_user_initiated is true, this is a browser initiated nit: are empty https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:188: nav_event = nit: Blank line before. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:196: : ReferrerChainEntry::LANDING_REFERRER); This is a bit unclear from reading the code. I'd suggest declaring a local variable with this value and explaining in a comment what it means to be the landing page or landing referrer. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:210: UMA_HISTOGRAM_BOOLEAN(kDownloadHasInvalidTabIDUMA, true); nit: Replace this whole block with: UMA_HISTOGRAM_BOOLEAN("SafeBrowsing.ReferrerHasInvalidTabID.DownloadAttribution", download_tab_id == -1); https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:339: DCHECK(interval >= base::TimeDelta()); Would DCHECK_GE work? Also, presumably we don't want to allow an empty interval? (Thus DCHECK_GT.) https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: // the vector in reverse order to get the latest match. I think we discussed earlier that this is imperfect, right? A more recent but unrelated navigation to the same URL can cause an incorrect attribution. I seem to remember that we decided it wasn't easily exploitable since the attacker would generally end up in the chain either way? Let's elaborate on that in the FindNavigationEvent declaration comment. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:364: !rit->is_user_initiated) nit: This if/else should probably have braces. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:39: // Always at the end nit: End with period. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:65: // Remove all the observed NavigationEvents, user gestures, and resolved IP nit: These will be easier to read with blank lines between the related blocks (e.g., before lines 63, 65, 68, 74). https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: // Identify referrer chain within user_gesture_count_limit user gestures. Can you elaborate on this comment? This is kind of the heart of the analysis, right? It would help to describe that this looks through connected navigation events that have been recorded to find which page was responsible. Also worth mentioning that it includes only user initiated events. Please also document that |referrer_chain| is an out parameter. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:74: // Identify and add referrer chain to ClientDownloadRequest proto. Is |request| an output parameter? Where is the proto? Let's elaborate in the comment. Also mention that this records UMA stats on the results. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:88: FRIEND_TEST_ALL_PREFIXES(SBNavigationObserverTest, This is already listed as a friend class above. Is there a reason these tests need to be listed here as well? https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:144: navigation_map()->insert( nit: Some blank lines and comments might make these tests more legible. https://codereview.chromium.org/2538483002/diff/100001/chrome/common/safe_bro... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/common/safe_bro... chrome/common/safe_browsing/csd.proto:407: repeated string ip_address = 3; This should either be ip_address_list or ip_addresses, since it's plural. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: <script> nit: Wrong indent. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:3: <script> Same. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html:3: <script> Same. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html:4: // Click on a link by id to star a test case. start https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect_to_landing.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect_to_landing.html:6: ... Should there be text here instead? https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.SubFrameNewTabDownload I don't see other SafeBrowsing tests in here. Do you know why these need to be disabled? (Changing this filter file requires a PlzNavigate owner. I'm in that owners list, but I know they're looking to avoid adding new tests to the filter.) https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53360: + failure. This is incremented each time when a safe browsing ping or download nit: Drop "when" https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53378: + incremented each time when a safe browsing ping or download ping is nit: Drop "when"
https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53356: + units="SafeBrowsingAttributionResultTypes"> units -> enum https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53377: + The size of referrer URL chain we get from referrer attribution. This is Size -> length/ number of URLs? I assumed you meant in bytes until I looked at code.
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
The CQ bit was checked by jialiul@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...
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...)
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by jialiul@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...
Thank you, holte@ and creis@. I've addressed all your comments. creis@, yes, I'll do a Finch experiment before launching it to all safe browsing users. Also I emailed rschoen@ for suggestion on how often we should do cleanups. Let's see how he replies. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:140: // For browser initiated navigation (e.g. from address bar or bookmark), we On 2016/12/09 22:00:24, Charlie Reis wrote: > Sounds right. thanks for confirming. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:216: if (!details.url.host().empty()) On 2016/12/09 22:00:24, Charlie Reis wrote: > Style nit: Needs braces, since body is more than one line. Thanks for catching this. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:174: // This function needs javascript support from test pages. On 2016/12/09 22:00:25, Charlie Reis wrote: > Let's elaborate a bit on what it needs, so that others can add tests using it. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:256: observer_manager_->IdentifyReferrerChain( On 2016/12/09 22:00:25, Charlie Reis wrote: > Please include an EXPECT_EQ for the return value. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:257: download->GetURL(), download_tab_id, 2, &referrer_chain); On 2016/12/09 22:00:24, Charlie Reis wrote: > Let's document that 2 is the user gesture limit. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:266: ASSERT_EQ(1ULL, actual_host_ip_map->size()); On 2016/12/09 22:00:25, Charlie Reis wrote: > I'm curious, is there a reason to use ULL here? I've seen U in a bunch of > places, but ULL seems rarer to me. Maybe I just haven't seen the cases that > need it? You're right. U is more commonly used in size literals. Change "ULL" into "U" instead. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:319: auto referrer_chain = IdentifyReferrerChain(GetDownload()); On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: Blank line before (same in all tests). This is a separate block verifying > the referrer chain. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:1353: // Click three links before reach download. On 2016/12/09 22:00:24, Charlie Reis wrote: > nit: reaching Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:29: const char kDownloadAttributionResultUMA[] = On 2016/12/09 22:00:25, Charlie Reis wrote: > These shouldn't be constants. Just use the strings directly in the histogram > macro. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:54: // navigation related records that happened 2 minutes ago is considered as On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { On 2016/12/09 22:00:25, Charlie Reis wrote: > This will be running every 2 minutes, right? Please check in with rschoen@ or > someone from the performance team to see if there are best practices for this, > to avoid unnecessary idle wakeups, etc. I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:166: // Back trace to the next nav_event that initiated by user. On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: that was initiated by the user Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:178: user_gesture_count++; On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: Blank line before and after. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:180: // is empty, and is_user_initiated is true, this is a browser initiated On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: are empty Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:188: nav_event = On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: Blank line before. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:196: : ReferrerChainEntry::LANDING_REFERRER); On 2016/12/09 22:00:25, Charlie Reis wrote: > This is a bit unclear from reading the code. I'd suggest declaring a local > variable with this value and explaining in a comment what it means to be the > landing page or landing referrer. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:210: UMA_HISTOGRAM_BOOLEAN(kDownloadHasInvalidTabIDUMA, true); On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: Replace this whole block with: > UMA_HISTOGRAM_BOOLEAN("SafeBrowsing.ReferrerHasInvalidTabID.DownloadAttribution", > download_tab_id == -1); Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:339: DCHECK(interval >= base::TimeDelta()); On 2016/12/09 22:00:25, Charlie Reis wrote: > Would DCHECK_GE work? Also, presumably we don't want to allow an empty > interval? (Thus DCHECK_GT.) Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: // the vector in reverse order to get the latest match. On 2016/12/09 22:00:25, Charlie Reis wrote: > I think we discussed earlier that this is imperfect, right? A more recent but > unrelated navigation to the same URL can cause an incorrect attribution. I seem > to remember that we decided it wasn't easily exploitable since the attacker > would generally end up in the chain either way? Let's elaborate on that in the > FindNavigationEvent declaration comment. Added comment and example to safe_browsing_navigation_observer_manager.h file. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:364: !rit->is_user_initiated) On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: This if/else should probably have braces. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:39: // Always at the end On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: End with period. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:65: // Remove all the observed NavigationEvents, user gestures, and resolved IP On 2016/12/09 22:00:25, Charlie Reis wrote: > nit: These will be easier to read with blank lines between the related blocks > (e.g., before lines 63, 65, 68, 74). Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: // Identify referrer chain within user_gesture_count_limit user gestures. On 2016/12/09 22:00:25, Charlie Reis wrote: > Can you elaborate on this comment? This is kind of the heart of the analysis, > right? It would help to describe that this looks through connected navigation > events that have been recorded to find which page was responsible. Also worth > mentioning that it includes only user initiated events. > > Please also document that |referrer_chain| is an out parameter. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:74: // Identify and add referrer chain to ClientDownloadRequest proto. On 2016/12/09 22:00:25, Charlie Reis wrote: > Is |request| an output parameter? Where is the proto? Let's elaborate in the > comment. > > Also mention that this records UMA stats on the results. ClientDownloadRequest is located at chrome/common/safe_browsing/csd.proto, this is the proto going to be send to SB backend for download classification. Added more details to this comment. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:88: FRIEND_TEST_ALL_PREFIXES(SBNavigationObserverTest, On 2016/12/09 22:00:25, Charlie Reis wrote: > This is already listed as a friend class above. Is there a reason these tests > need to be listed here as well? Done. These tests called SafeBrowsingNavigationObserverManager's private functions, though it actually makes more sense to add wrappers in the SBNavigationObserverTest class. I'll remove these FRIEND_TEST_ALL_PREFIXES. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:144: navigation_map()->insert( On 2016/12/09 22:00:26, Charlie Reis wrote: > nit: Some blank lines and comments might make these tests more legible. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/common/safe_bro... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/common/safe_bro... chrome/common/safe_browsing/csd.proto:407: repeated string ip_address = 3; On 2016/12/09 22:00:26, Charlie Reis wrote: > This should either be ip_address_list or ip_addresses, since it's plural. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: <script> On 2016/12/09 22:00:26, Charlie Reis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html:3: <script> On 2016/12/09 22:00:26, Charlie Reis wrote: > Same. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html:3: <script> On 2016/12/09 22:00:26, Charlie Reis wrote: > Same. Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html:4: // Click on a link by id to star a test case. On 2016/12/09 22:00:26, Charlie Reis wrote: > start Done. https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect_to_landing.html (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect_to_landing.html:6: ... On 2016/12/09 22:00:26, Charlie Reis wrote: > Should there be text here instead? Added a line to describe what does this page do. https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.SubFrameNewTabDownload On 2016/12/09 22:00:26, Charlie Reis wrote: > I don't see other SafeBrowsing tests in here. Do you know why these need to be > disabled? (Changing this filter file requires a PlzNavigate owner. I'm in that > owners list, but I know they're looking to avoid adding new tests to the > filter.) This is because the browser_site_navigation_test seems to impact the result of NavigationHanle::IsRendererInitiated(). And my code depends on this function to determine if the download is triggered by users (browser_initiated || has_user_gesture). Do you know if this is a known bug or it is WAI in PlzNavigate world? https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53356: + units="SafeBrowsingAttributionResultTypes"> On 2016/12/10 00:33:30, Steven Holte wrote: > units -> enum Oops, thanks for catching this. https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53360: + failure. This is incremented each time when a safe browsing ping or download On 2016/12/09 22:00:26, Charlie Reis wrote: > nit: Drop "when" Done. https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53377: + The size of referrer URL chain we get from referrer attribution. This is On 2016/12/10 00:33:30, Steven Holte wrote: > Size -> length/ number of URLs? I assumed you meant in bytes until I looked at > code. Oh yes, it means length. Thanks for pointing out this ambiguity. https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53378: + incremented each time when a safe browsing ping or download ping is On 2016/12/09 22:00:26, Charlie Reis wrote: > nit: Drop "when" Done.
histograms lgtm
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_...)
creis@, any further comments?
creis@chromium.org changed reviewers: + clamy@chromium.org
[+clamy@] Thanks! Basically looks good now, but I'd like to check with clamy@ about whether we need to get the tests working in PlzNavigate before landing (and if she has any ideas about why IsRendererInitiated might behave differently there). Otherwise just a few nits below. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { On 2016/12/12 23:43:37, Jialiu Lin wrote: > On 2016/12/09 22:00:25, Charlie Reis wrote: > > This will be running every 2 minutes, right? Please check in with rschoen@ or > > someone from the performance team to see if there are best practices for this, > > to avoid unnecessary idle wakeups, etc. > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. Thanks! Sounds like a circular buffer may be a good suggestion, but that can be done in a separate CL if you'd prefer. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: // the vector in reverse order to get the latest match. On 2016/12/12 23:43:37, Jialiu Lin wrote: > On 2016/12/09 22:00:25, Charlie Reis wrote: > > I think we discussed earlier that this is imperfect, right? A more recent but > > unrelated navigation to the same URL can cause an incorrect attribution. I > seem > > to remember that we decided it wasn't easily exploitable since the attacker > > would generally end up in the chain either way? Let's elaborate on that in > the > > FindNavigationEvent declaration comment. > > Added comment and example to safe_browsing_navigation_observer_manager.h file. Acknowledged. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: // Identify referrer chain within user_gesture_count_limit user gestures. On 2016/12/12 23:43:38, Jialiu Lin wrote: > On 2016/12/09 22:00:25, Charlie Reis wrote: > > Can you elaborate on this comment? This is kind of the heart of the analysis, > > right? It would help to describe that this looks through connected navigation > > events that have been recorded to find which page was responsible. Also worth > > mentioning that it includes only user initiated events. > > > > Please also document that |referrer_chain| is an out parameter. > > Done. Thanks! That helps a lot. https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.SubFrameNewTabDownload On 2016/12/12 23:43:38, Jialiu Lin wrote: > On 2016/12/09 22:00:26, Charlie Reis wrote: > > I don't see other SafeBrowsing tests in here. Do you know why these need to > be > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm in > that > > owners list, but I know they're looking to avoid adding new tests to the > > filter.) > > This is because the browser_site_navigation_test seems to impact the result of > NavigationHanle::IsRendererInitiated(). And my code depends on this function to > determine if the download is triggered by users (browser_initiated || > has_user_gesture). Do you know if this is a known bug or it is WAI in > PlzNavigate world? Maybe it has to do with how these tests are setting up their navigation? clamy@, do you know? I know PlzNavigate folks are getting close to a Finch trial, so we should probably figure this out before landing so that we can omit these tests from this filter file. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:193: // gesture before this navigaiton event, this is a navigation lead to the nit: navigation nit: leading https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:71: // Based on the |target_url| and |target_tab_id|, backtrace observed nit: s/backtrace/trace back the/ https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:72: // NavigationEvents in navigation_map_ to identify the sequence of navigations user-initiated navigations? https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:73: // lead to the target with the coverage limited to |user_gesture_count_limit| nit: s/lead/leading/ nit: comma after target https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:145: // about::blank page window C and injects script code in it to trigger a nit: about:blank https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: <script> I meant that the <script> tag has the wrong indent, and doesn't match the </script> tag. Maybe there's a tab character here instead of spaces? Same in the other files.
On 2016/12/14 07:43:58, Charlie Reis wrote: > [+clamy@] > > Thanks! Basically looks good now, but I'd like to check with clamy@ about > whether we need to get the tests working in PlzNavigate before landing (and if > she has any ideas about why IsRendererInitiated might behave differently there). > > Otherwise just a few nits below. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > (right): > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: > void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > This will be running every 2 minutes, right? Please check in with rschoen@ > or > > > someone from the performance team to see if there are best practices for > this, > > > to avoid unnecessary idle wakeups, etc. > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. > > Thanks! Sounds like a circular buffer may be a good suggestion, but that can be > done in a separate CL if you'd prefer. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: > // the vector in reverse order to get the latest match. > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > I think we discussed earlier that this is imperfect, right? A more recent > but > > > unrelated navigation to the same URL can cause an incorrect attribution. I > > seem > > > to remember that we decided it wasn't easily exploitable since the attacker > > > would generally end up in the chain either way? Let's elaborate on that in > > the > > > FindNavigationEvent declaration comment. > > > > Added comment and example to safe_browsing_navigation_observer_manager.h file. > > Acknowledged. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > (right): > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: // > Identify referrer chain within user_gesture_count_limit user gestures. > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > Can you elaborate on this comment? This is kind of the heart of the > analysis, > > > right? It would help to describe that this looks through connected > navigation > > > events that have been recorded to find which page was responsible. Also > worth > > > mentioning that it includes only user initiated events. > > > > > > Please also document that |referrer_chain| is an out parameter. > > > > Done. > > Thanks! That helps a lot. > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter > (right): > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: > -SBNavigationObserverBrowserTest.SubFrameNewTabDownload > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > I don't see other SafeBrowsing tests in here. Do you know why these need to > > be > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm in > > that > > > owners list, but I know they're looking to avoid adding new tests to the > > > filter.) > > > > This is because the browser_site_navigation_test seems to impact the result of > > NavigationHanle::IsRendererInitiated(). And my code depends on this function > to > > determine if the download is triggered by users (browser_initiated || > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > PlzNavigate world? > > Maybe it has to do with how these tests are setting up their navigation? > clamy@, do you know? I wasn't aware of that fact, but looking at how NavigationHandle::IsRendererInitiated, I may have an idea why this is the case. Could you patch https://codereview.chromium.org/2567253005/ to see if that fixes the issue? (Note this is not a patch, I just want to confirm that the issue is coming from what I'm thinking). > > I know PlzNavigate folks are getting close to a Finch trial, so we should > probably figure this out before landing so that we can omit these tests from > this filter file. > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:193: > // gesture before this navigaiton event, this is a navigation lead to the > nit: navigation > nit: leading > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:71: // > Based on the |target_url| and |target_tab_id|, backtrace observed > nit: s/backtrace/trace back the/ > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:72: // > NavigationEvents in navigation_map_ to identify the sequence of navigations > user-initiated navigations? > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:73: // > lead to the target with the coverage limited to |user_gesture_count_limit| > nit: s/lead/leading/ > nit: comma after target > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:145: // > about::blank page window C and injects script code in it to trigger a > nit: about:blank > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > File > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: > <script> > I meant that the <script> tag has the wrong indent, and doesn't match the > </script> tag. Maybe there's a tab character here instead of spaces? > > Same in the other files.
On 2016/12/14 07:43:58, Charlie Reis wrote: > [+clamy@] > > Thanks! Basically looks good now, but I'd like to check with clamy@ about > whether we need to get the tests working in PlzNavigate before landing (and if > she has any ideas about why IsRendererInitiated might behave differently there). > > Otherwise just a few nits below. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > (right): > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: > void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > This will be running every 2 minutes, right? Please check in with rschoen@ > or > > > someone from the performance team to see if there are best practices for > this, > > > to avoid unnecessary idle wakeups, etc. > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. > > Thanks! Sounds like a circular buffer may be a good suggestion, but that can be > done in a separate CL if you'd prefer. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: > // the vector in reverse order to get the latest match. > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > I think we discussed earlier that this is imperfect, right? A more recent > but > > > unrelated navigation to the same URL can cause an incorrect attribution. I > > seem > > > to remember that we decided it wasn't easily exploitable since the attacker > > > would generally end up in the chain either way? Let's elaborate on that in > > the > > > FindNavigationEvent declaration comment. > > > > Added comment and example to safe_browsing_navigation_observer_manager.h file. > > Acknowledged. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > (right): > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: // > Identify referrer chain within user_gesture_count_limit user gestures. > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > Can you elaborate on this comment? This is kind of the heart of the > analysis, > > > right? It would help to describe that this looks through connected > navigation > > > events that have been recorded to find which page was responsible. Also > worth > > > mentioning that it includes only user initiated events. > > > > > > Please also document that |referrer_chain| is an out parameter. > > > > Done. > > Thanks! That helps a lot. > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter > (right): > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: > -SBNavigationObserverBrowserTest.SubFrameNewTabDownload > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > I don't see other SafeBrowsing tests in here. Do you know why these need to > > be > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm in > > that > > > owners list, but I know they're looking to avoid adding new tests to the > > > filter.) > > > > This is because the browser_site_navigation_test seems to impact the result of > > NavigationHanle::IsRendererInitiated(). And my code depends on this function > to > > determine if the download is triggered by users (browser_initiated || > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > PlzNavigate world? > > Maybe it has to do with how these tests are setting up their navigation? > clamy@, do you know? I wasn't aware of that fact, but looking at how NavigationHandle::IsRendererInitiated, I may have an idea why this is the case. Could you patch https://codereview.chromium.org/2567253005/ to see if that fixes the issue? (Note this is not a patch, I just want to confirm that the issue is coming from what I'm thinking). > > I know PlzNavigate folks are getting close to a Finch trial, so we should > probably figure this out before landing so that we can omit these tests from > this filter file. > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:193: > // gesture before this navigaiton event, this is a navigation lead to the > nit: navigation > nit: leading > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:71: // > Based on the |target_url| and |target_tab_id|, backtrace observed > nit: s/backtrace/trace back the/ > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:72: // > NavigationEvents in navigation_map_ to identify the sequence of navigations > user-initiated navigations? > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:73: // > lead to the target with the coverage limited to |user_gesture_count_limit| > nit: s/lead/leading/ > nit: comma after target > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:145: // > about::blank page window C and injects script code in it to trigger a > nit: about:blank > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > File > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html > (right): > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: > <script> > I meant that the <script> tag has the wrong indent, and doesn't match the > </script> tag. Maybe there's a tab character here instead of spaces? > > Same in the other files.
On 2016/12/14 15:53:32, clamy wrote: > On 2016/12/14 07:43:58, Charlie Reis wrote: > > [+clamy@] > > > > Thanks! Basically looks good now, but I'd like to check with clamy@ about > > whether we need to get the tests working in PlzNavigate before landing (and if > > she has any ideas about why IsRendererInitiated might behave differently > there). > > > > Otherwise just a few nits below. > > > > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: > > void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() > { > > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > > This will be running every 2 minutes, right? Please check in with > rschoen@ > > or > > > > someone from the performance team to see if there are best practices for > > this, > > > > to avoid unnecessary idle wakeups, etc. > > > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. > > > > Thanks! Sounds like a circular buffer may be a good suggestion, but that can > be > > done in a separate CL if you'd prefer. > > > > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:354: > > // the vector in reverse order to get the latest match. > > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > > I think we discussed earlier that this is imperfect, right? A more recent > > but > > > > unrelated navigation to the same URL can cause an incorrect attribution. > I > > > seem > > > > to remember that we decided it wasn't easily exploitable since the > attacker > > > > would generally end up in the chain either way? Let's elaborate on that > in > > > the > > > > FindNavigationEvent declaration comment. > > > > > > Added comment and example to safe_browsing_navigation_observer_manager.h > file. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:68: > // > > Identify referrer chain within user_gesture_count_limit user gestures. > > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > > Can you elaborate on this comment? This is kind of the heart of the > > analysis, > > > > right? It would help to describe that this looks through connected > > navigation > > > > events that have been recorded to find which page was responsible. Also > > worth > > > > mentioning that it includes only user initiated events. > > > > > > > > Please also document that |referrer_chain| is an out parameter. > > > > > > Done. > > > > Thanks! That helps a lot. > > > > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > > File > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > > > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: > > -SBNavigationObserverBrowserTest.SubFrameNewTabDownload > > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > > I don't see other SafeBrowsing tests in here. Do you know why these need > to > > > be > > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm > in > > > that > > > > owners list, but I know they're looking to avoid adding new tests to the > > > > filter.) > > > > > > This is because the browser_site_navigation_test seems to impact the result > of > > > NavigationHanle::IsRendererInitiated(). And my code depends on this function > > to > > > determine if the download is triggered by users (browser_initiated || > > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > > PlzNavigate world? > > > > Maybe it has to do with how these tests are setting up their navigation? > > clamy@, do you know? > > I wasn't aware of that fact, but looking at how > NavigationHandle::IsRendererInitiated, I may have an idea why this is the case. > Could you patch https://codereview.chromium.org/2567253005/ to see if that fixes > the issue? (Note this is not a patch, I just want to confirm that the issue is > coming from what I'm thinking). Thanks clamy@! Your patch works. :-) Will you land this patch soon? Or let me know when you land it, I'll remove these tests from this test filter. > > > > > I know PlzNavigate folks are getting close to a Finch trial, so we should > > probably figure this out before landing so that we can omit these tests from > > this filter file. > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:193: > > // gesture before this navigaiton event, this is a navigation lead to the > > nit: navigation > > nit: leading > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:71: > // > > Based on the |target_url| and |target_tab_id|, backtrace observed > > nit: s/backtrace/trace back the/ > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:72: > // > > NavigationEvents in navigation_map_ to identify the sequence of navigations > > user-initiated navigations? > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:73: > // > > lead to the target with the coverage limited to |user_gesture_count_limit| > > nit: s/lead/leading/ > > nit: comma after target > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:145: > // > > about::blank page window C and injects script code in it to trigger a > > nit: about:blank > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > > File > > > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html > > (right): > > > > > https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... > > > chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: > > <script> > > I meant that the <script> tag has the wrong indent, and doesn't match the > > </script> tag. Maybe there's a tab character here instead of spaces? > > > > Same in the other files.
Thanks creis@! https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { On 2016/12/14 07:43:57, Charlie Reis wrote: > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > This will be running every 2 minutes, right? Please check in with rschoen@ > or > > > someone from the performance team to see if there are best practices for > this, > > > to avoid unnecessary idle wakeups, etc. > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. > > Thanks! Sounds like a circular buffer may be a good suggestion, but that can be > done in a separate CL if you'd prefer. Let me think about this. I feel circular buffer is much easier to game than a 2 minutes clean up window. If an attacker want to hide its landing page, it can easily stuff this circular buffer with irrelevant navigation events than asking their victims to wait for 2 minutes before starting download, since the latter would introduce a significant friction. https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.SubFrameNewTabDownload On 2016/12/14 07:43:57, Charlie Reis wrote: > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > I don't see other SafeBrowsing tests in here. Do you know why these need to > > be > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm in > > that > > > owners list, but I know they're looking to avoid adding new tests to the > > > filter.) > > > > This is because the browser_site_navigation_test seems to impact the result of > > NavigationHanle::IsRendererInitiated(). And my code depends on this function > to > > determine if the download is triggered by users (browser_initiated || > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > PlzNavigate world? > > Maybe it has to do with how these tests are setting up their navigation? > clamy@, do you know? > > I know PlzNavigate folks are getting close to a Finch trial, so we should > probably figure this out before landing so that we can omit these tests from > this filter file. clamy@'s test patch works. If clamy@'s going to land this patch soon, I'll wait for her. Or I can land my CL first and remove these tests from this file later. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:193: // gesture before this navigaiton event, this is a navigation lead to the On 2016/12/14 at 07:43:58, Charlie Reis wrote: > nit: navigation > nit: leading done. Thanks. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:71: // Based on the |target_url| and |target_tab_id|, backtrace observed On 2016/12/14 at 07:43:58, Charlie Reis wrote: > nit: s/backtrace/trace back the/ Done. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:72: // NavigationEvents in navigation_map_ to identify the sequence of navigations On 2016/12/14 at 07:43:58, Charlie Reis wrote: > user-initiated navigations? Not only user-initiated navigations but also redirects in between. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:73: // lead to the target with the coverage limited to |user_gesture_count_limit| On 2016/12/14 at 07:43:58, Charlie Reis wrote: > nit: s/lead/leading/ > nit: comma after target Done. https://codereview.chromium.org/2538483002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h:145: // about::blank page window C and injects script code in it to trigger a On 2016/12/14 at 07:43:58, Charlie Reis wrote: > nit: about:blank Oops. Fixed. https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html (right): https://codereview.chromium.org/2538483002/diff/140001/chrome/test/data/safe_... chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html:3: <script> On 2016/12/14 at 07:43:58, Charlie Reis wrote: > I meant that the <script> tag has the wrong indent, and doesn't match the </script> tag. Maybe there's a tab character here instead of spaces? > > Same in the other files. Yes, you're right, there is a tab in front of <script>. I'm checking other files to see if there are similar problems. Thanks!
Thanks! LGTM with a few thoughts below. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { On 2016/12/14 19:06:29, Jialiu Lin wrote: > On 2016/12/14 07:43:57, Charlie Reis wrote: > > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > > This will be running every 2 minutes, right? Please check in with > rschoen@ > > or > > > > someone from the performance team to see if there are best practices for > > this, > > > > to avoid unnecessary idle wakeups, etc. > > > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for suggestion. > > > > Thanks! Sounds like a circular buffer may be a good suggestion, but that can > be > > done in a separate CL if you'd prefer. > Let me think about this. I feel circular buffer is much easier to game than a 2 > minutes clean up window. If an attacker want to hide its landing page, it can > easily > stuff this circular buffer with irrelevant navigation events than asking their > victims > to wait for 2 minutes before starting download, since the latter would introduce > a significant friction. True, though the flip side is that they can allocate almost arbitrary memory in the browser process with the 2 minute approach. In general, we don't consider DoS a security bug, but it is good to avoid. (For example, there's a limit of 50 navigation entries in session history for each tab.) Anyway, we can discuss how to deal with this separate from this CL. https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter (right): https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: -SBNavigationObserverBrowserTest.SubFrameNewTabDownload On 2016/12/14 19:06:29, Jialiu Lin wrote: > On 2016/12/14 07:43:57, Charlie Reis wrote: > > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > > I don't see other SafeBrowsing tests in here. Do you know why these need > to > > > be > > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm > in > > > that > > > > owners list, but I know they're looking to avoid adding new tests to the > > > > filter.) > > > > > > This is because the browser_site_navigation_test seems to impact the result > of > > > NavigationHanle::IsRendererInitiated(). And my code depends on this function > > to > > > determine if the download is triggered by users (browser_initiated || > > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > > PlzNavigate world? > > > > Maybe it has to do with how these tests are setting up their navigation? > > clamy@, do you know? > > > > I know PlzNavigate folks are getting close to a Finch trial, so we should > > probably figure this out before landing so that we can omit these tests from > > this filter file. > clamy@'s test patch works. If clamy@'s going to land this patch soon, I'll > wait for her. Or I can land my CL first and remove these tests from this file > later. Great! Sounds like it's a problem on the PlzNavigate side rather than your CL, so I think it'll be ok if this lands with these tests disabled for the moment, and clamy@ can fix and re-enable. (Sound ok, clamy@?)
On 2016/12/15 00:38:33, Charlie Reis (OOO soon) wrote: > Thanks! LGTM with a few thoughts below. > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc > (right): > > https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: > void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { > On 2016/12/14 19:06:29, Jialiu Lin wrote: > > On 2016/12/14 07:43:57, Charlie Reis wrote: > > > On 2016/12/12 23:43:37, Jialiu Lin wrote: > > > > On 2016/12/09 22:00:25, Charlie Reis wrote: > > > > > This will be running every 2 minutes, right? Please check in with > > rschoen@ > > > or > > > > > someone from the performance team to see if there are best practices for > > > this, > > > > > to avoid unnecessary idle wakeups, etc. > > > > > > > > I sent an email to rschoen@ (cc'ed you and nparker@) asking for > suggestion. > > > > > > Thanks! Sounds like a circular buffer may be a good suggestion, but that > can > > be > > > done in a separate CL if you'd prefer. > > Let me think about this. I feel circular buffer is much easier to game than a > 2 > > minutes clean up window. If an attacker want to hide its landing page, it can > > easily > > stuff this circular buffer with irrelevant navigation events than asking their > > victims > > to wait for 2 minutes before starting download, since the latter would > introduce > > a significant friction. > > True, though the flip side is that they can allocate almost arbitrary memory in > the browser process with the 2 minute approach. In general, we don't consider > DoS a security bug, but it is good to avoid. (For example, there's a limit of > 50 navigation entries in session history for each tab.) > > Anyway, we can discuss how to deal with this separate from this CL. SG, let's deal with this later. > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > File testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter > (right): > > https://codereview.chromium.org/2538483002/diff/100001/testing/buildbot/filte... > testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter:13: > -SBNavigationObserverBrowserTest.SubFrameNewTabDownload > On 2016/12/14 19:06:29, Jialiu Lin wrote: > > On 2016/12/14 07:43:57, Charlie Reis wrote: > > > On 2016/12/12 23:43:38, Jialiu Lin wrote: > > > > On 2016/12/09 22:00:26, Charlie Reis wrote: > > > > > I don't see other SafeBrowsing tests in here. Do you know why these > need > > to > > > > be > > > > > disabled? (Changing this filter file requires a PlzNavigate owner. I'm > > in > > > > that > > > > > owners list, but I know they're looking to avoid adding new tests to the > > > > > filter.) > > > > > > > > This is because the browser_site_navigation_test seems to impact the > result > > of > > > > NavigationHanle::IsRendererInitiated(). And my code depends on this > function > > > to > > > > determine if the download is triggered by users (browser_initiated || > > > > has_user_gesture). Do you know if this is a known bug or it is WAI in > > > > PlzNavigate world? > > > > > > Maybe it has to do with how these tests are setting up their navigation? > > > clamy@, do you know? > > > > > > I know PlzNavigate folks are getting close to a Finch trial, so we should > > > probably figure this out before landing so that we can omit these tests from > > > this filter file. > > clamy@'s test patch works. If clamy@'s going to land this patch soon, I'll > > wait for her. Or I can land my CL first and remove these tests from this file > > later. > > Great! Sounds like it's a problem on the PlzNavigate side rather than your CL, > so I think it'll be ok if this lands with these tests disabled for the moment, > and clamy@ can fix and re-enable. (Sound ok, clamy@?) SGTM. Thanks!
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2538483002/#ps160001 (title: "address nits")
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: 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/...)
The CQ bit was checked by jialiul@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": 160001, "attempt_start_ts": 1481773667518650, "parent_rev": "3354028521dec80c9e0a943fdb2d8f9458bf908a", "commit_rev": "5c049d9c0700e22a29a272109e36bc8a1a770530"}
Message was sent while issue was closed.
Description was changed from ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 ========== to ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 Review-Url: https://codereview.chromium.org/2538483002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 Review-Url: https://codereview.chromium.org/2538483002 ========== to ========== Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 Committed: https://crrev.com/4e2ebcc9087c02598a638aa86cba21c043069b76 Cr-Commit-Position: refs/heads/master@{#438745} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4e2ebcc9087c02598a638aa86cba21c043069b76 Cr-Commit-Position: refs/heads/master@{#438745} |