Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779643002:
Use new SafeBrowsing redirect tracking code in CWS pings. (Closed)
|
Created:
3 years, 9 months ago by robertshield Modified:
3 years, 7 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse new SafeBrowsing redirect tracking code in CWS pings.
The SafeBrowsing code tracks more types of redirects than the /net code which strictly adheres to spec and so can drop various types of redirects which can obscure the referrer chain.
BUG=685905
Review-Url: https://codereview.chromium.org/2779643002
Cr-Commit-Position: refs/heads/master@{#468389}
Committed: https://chromium.googlesource.com/chromium/src/+/39d1ffe7ba2c6fc8ebe6245472e0191b6f29aa9c
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add browser test to also test SafeBrowsing redirector implementation. #Patch Set 3 : Be more permissive about tracked redirects in browser test. #Patch Set 4 : cleanup #
Total comments: 10
Patch Set 5 : Devlin feedback. #
Total comments: 6
Patch Set 6 : Devlin feedback + rebase. #
Messages
Total messages: 47 (35 generated)
Description was changed from ========== Use new referrer code in CWS pings. BUG=685905 ========== to ========== Use new referrer code in CWS pings. BUG=685905 ==========
jialiul@chromium.org changed reviewers: + jialiul@chromium.org
lgtm https://codereview.chromium.org/2779643002/diff/1/chrome/browser/extensions/w... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/1/chrome/browser/extensions/w... chrome/browser/extensions/webstore_inline_installer.cc:143: navigation_observer_manager->IdentifyReferrerChainByDownloadWebContent( FYI, I updated the function name to IdentifyReferrerChainByWebContents(..) to make it sound more general. Logic remains the same.
The CQ bit was checked by robertshield@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robertshield@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robertshield@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.
Patchset #5 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Use new referrer code in CWS pings. BUG=685905 ========== to ========== Use new SafeBrowsing redirect tracking code in CWS pings. The new referrer code tracks more types of redirects though it may over-track in some cases that show up in browser tests. BUG=685905 ==========
The CQ bit was checked by robertshield@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.
robertshield@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Thanks Jialiu, updated to match your latest changes. I also added a browser test to test both flavours of redirect tracking. +Devlin to review extension-related changes.
Looks pretty good overall, but can you add a bit more detail either here in the CL or in the bug around what this is and why we're doing it? I don't have much context around what the "new SafeBrowsing redirect tracking code" is, or why it's preferable. (Doesn't need to be incredibly detailed - just another sentence or two.) https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:30: const int kExtensionReferrerUserGestureLimit = 2; \n https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:133: if (!safe_browsing_service) Document when this and navigation_observer_manager below can be null https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:426: // GURL pre_navigate_url = GenerateTestServerUrl(kAppDomain, "install.html"); dead code? https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:452: RunTestAsync("runTest"); Do we need to make this an async run? https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:485: testing::Values(false)); Optional: If you don't care much about the naming, I think you can use a single call to instantiate the tests with testing::Bool() instead of testing::Values(true) and testing::Values(false). Up to you.
LGTM :-)
The CQ bit was checked by robertshield@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.
Description was changed from ========== Use new SafeBrowsing redirect tracking code in CWS pings. The new referrer code tracks more types of redirects though it may over-track in some cases that show up in browser tests. BUG=685905 ========== to ========== Use new SafeBrowsing redirect tracking code in CWS pings. The SafeBrowsing code tracks more types of redirects than the /net code which strictly adheres to spec and so can drop various types of redirects which can obscure the referrer chain. BUG=685905 ==========
Thanks Devlin, comments addressed. Apologies, I rebased and forgot to upload a separate patch version, so there are a couple of superfluous diffs from #4 to #5. https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:30: const int kExtensionReferrerUserGestureLimit = 2; On 2017/04/13 15:03:07, Devlin wrote: > \n Done. https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:133: if (!safe_browsing_service) On 2017/04/13 15:03:07, Devlin wrote: > Document when this and navigation_observer_manager below can be null Done. https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:426: // GURL pre_navigate_url = GenerateTestServerUrl(kAppDomain, "install.html"); On 2017/04/13 15:03:07, Devlin wrote: > dead code? Oops, yes, thanks. https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:452: RunTestAsync("runTest"); On 2017/04/13 15:03:07, Devlin wrote: > Do we need to make this an async run? It seems to hang if we don't. This became true when I added the WebstoreInlineInstallerForTestFactory above. https://codereview.chromium.org/2779643002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:485: testing::Values(false)); On 2017/04/13 15:03:07, Devlin wrote: > Optional: If you don't care much about the naming, I think you can use a single > call to instantiate the tests with testing::Bool() instead of > testing::Values(true) and testing::Values(false). Up to you. Ah, true.. I think on the balance having the two more explicit names here makes it slightly easier to parse test log output.
lgtm https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:126: std::unique_ptr<base::ListValue> redirect_chain = optional: auto redirect_chain = base::MakeUnique<base::ListValue>(); MakeUnique<T> is clear that it will return a std::unique_ptr<T>, so auto is acceptable, if you wanted to make it a bit shorter. https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:173: navigation_entry->GetRedirectChain(); We used to have a check for if (navigation_entry), since GetLastCommittedEntry() can sometimes return null. Was the removal of the if deliberate? If so, why do we know this is non-null?
one more :) (still lgtm) https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:117: Profile::FromBrowserContext(web_contents()->GetBrowserContext())); nit: profile()
The CQ bit was checked by robertshield@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.
Thanks! https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:117: Profile::FromBrowserContext(web_contents()->GetBrowserContext())); On 2017/04/21 17:10:33, Devlin wrote: > nit: profile() Done. https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:126: std::unique_ptr<base::ListValue> redirect_chain = On 2017/04/21 17:06:11, Devlin wrote: > optional: auto redirect_chain = base::MakeUnique<base::ListValue>(); > MakeUnique<T> is clear that it will return a std::unique_ptr<T>, so auto is > acceptable, if you wanted to make it a bit shorter. Done. https://codereview.chromium.org/2779643002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:173: navigation_entry->GetRedirectChain(); On 2017/04/21 17:06:11, Devlin wrote: > We used to have a check for if (navigation_entry), since GetLastCommittedEntry() > can sometimes return null. Was the removal of the if deliberate? If so, why do > we know this is non-null? It was, but on further inspection, it was wrong. Added the check back, thanks for noticing.
The CQ bit was checked by robertshield@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2779643002/#ps140001 (title: "Devlin feedback + rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493664993437330,
"parent_rev": "6e2f717c183f4f56c8d962d7d4c1fb15da6625ff", "commit_rev":
"39d1ffe7ba2c6fc8ebe6245472e0191b6f29aa9c"}
Message was sent while issue was closed.
Description was changed from ========== Use new SafeBrowsing redirect tracking code in CWS pings. The SafeBrowsing code tracks more types of redirects than the /net code which strictly adheres to spec and so can drop various types of redirects which can obscure the referrer chain. BUG=685905 ========== to ========== Use new SafeBrowsing redirect tracking code in CWS pings. The SafeBrowsing code tracks more types of redirects than the /net code which strictly adheres to spec and so can drop various types of redirects which can obscure the referrer chain. BUG=685905 Review-Url: https://codereview.chromium.org/2779643002 Cr-Commit-Position: refs/heads/master@{#468389} Committed: https://chromium.googlesource.com/chromium/src/+/39d1ffe7ba2c6fc8ebe6245472e0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/39d1ffe7ba2c6fc8ebe6245472e0...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 468389 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2854763002/ by sky@chromium.org. The reason for reverting is: Reverting in hopes of fixing 717315. See it for details.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
