Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615883003:
[NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING (Closed)
|
Created:
3 years, 11 months ago by droger Modified:
3 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, Randy Smith (Not in Mondays), gavinp+prer_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING
This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch.
The SafeBrowsing throttle attempts to destroy the prefetch if the
main resource is unsafe.
If a subresource is unsafe, it is cancelled, but the network requests
for other subresources on the same page will typically NOT be canceled.
The issue of subresources will be addressed separately in the future.
BUG=678941
Review-Url: https://codereview.chromium.org/2615883003
Cr-Commit-Position: refs/heads/master@{#442543}
Committed: https://chromium.googlesource.com/chromium/src/+/316b5bbb9d35045363bd3954f7c3a68951314cd8
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : plumb FINAL_STATUS_SAFEBROWSING #
Total comments: 3
Patch Set 4 : main resource only #
Messages
Total messages: 31 (22 generated)
The CQ bit was checked by droger@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...
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Re-enable NoStatePrefetch SafeBrowsing test BUG= ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresource will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. BUG=678941 ==========
Description was changed from ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresource will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. BUG=678941 ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresource will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources will typically NOT be canceled. BUG=678941 ==========
Description was changed from ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresource will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources will typically NOT be canceled. BUG=678941 ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresources will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources will typically NOT be canceled. BUG=678941 ==========
Description was changed from ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresources will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources will typically NOT be canceled. BUG=678941 ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresources will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources on the same page will typically NOT be canceled. BUG=678941 ==========
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
droger@chromium.org changed reviewers: + mattcary@chromium.org
lgtm https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:509: // already destroyed when the subresource is loaded. It seems a shame to not to have a reliable final status, especially since if we did we could use this as a reliable counter for how much we prefetch unsafe resources. Maybe we could add a histogram back in safe_browsing_resource_throttle::DestroyPrerenderContents, so we could at least count correctly... but then we have to do subtraction between histograms to get accurate counts, which is hard. Hmmm. I'm not sure. I guess this is fine for now until we think of something better.
https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:509: // already destroyed when the subresource is loaded. On 2017/01/06 16:37:20, mattcary wrote: > It seems a shame to not to have a reliable final status, especially since if we > did we could use this as a reliable counter for how much we prefetch unsafe > resources. > > Maybe we could add a histogram back in > safe_browsing_resource_throttle::DestroyPrerenderContents, so we could at least > count correctly... but then we have to do subtraction between histograms to get > accurate counts, which is hard. > > Hmmm. I'm not sure. I guess this is fine for now until we think of something > better. The easy way to make it reliable would be to call Destroy(FINAL_STATUS_SAFE_BROWSING) only for the main resource. In that case, subresource would still be cancelled, but the status would be FINAL_STATUS_NOSTATE_PREFETCH_FINISHED. And then we can revisit once we know what we want to do for subresources (which is not very clear yet).
On 2017/01/06 16:43:42, droger wrote: > https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): > > https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:509: // > already destroyed when the subresource is loaded. > On 2017/01/06 16:37:20, mattcary wrote: > > It seems a shame to not to have a reliable final status, especially since if > we > > did we could use this as a reliable counter for how much we prefetch unsafe > > resources. > > > > Maybe we could add a histogram back in > > safe_browsing_resource_throttle::DestroyPrerenderContents, so we could at > least > > count correctly... but then we have to do subtraction between histograms to > get > > accurate counts, which is hard. > > > > Hmmm. I'm not sure. I guess this is fine for now until we think of something > > better. > > The easy way to make it reliable would be to call > Destroy(FINAL_STATUS_SAFE_BROWSING) only for the main resource. > > In that case, subresource would still be cancelled, but the status would be > FINAL_STATUS_NOSTATE_PREFETCH_FINISHED. > > And then we can revisit once we know what we want to do for subresources (which > is not very clear yet). I like that. At least the counts will be specific.
Description was changed from ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if it detects something unsafe, both main resource and sub resource. In practice though, FINAL_STATUS_SAFEBROWSING is guaranteed to be set only for main resources. Subresources will only trigger FINAL_STATUS_SAFEBROWSING if they are caught before the PrerenderContents is finished. If a subresource is unsafe, the network requests for other subresources on the same page will typically NOT be canceled. BUG=678941 ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if the main resource is unsafe. If a subresource is unsafe, it is cancelled, but the network requests for other subresources on the same page will typically NOT be canceled. The issue of subresources will be addressed separately in the future. BUG=678941 ==========
The CQ bit was checked by droger@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...
droger@chromium.org changed reviewers: + vakh@chromium.org
+vakh for safe_browsing_resource_throttle.cc https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2615883003/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:509: // already destroyed when the subresource is loaded. On 2017/01/06 16:43:42, droger wrote: > On 2017/01/06 16:37:20, mattcary wrote: > > It seems a shame to not to have a reliable final status, especially since if > we > > did we could use this as a reliable counter for how much we prefetch unsafe > > resources. > > > > Maybe we could add a histogram back in > > safe_browsing_resource_throttle::DestroyPrerenderContents, so we could at > least > > count correctly... but then we have to do subtraction between histograms to > get > > accurate counts, which is hard. > > > > Hmmm. I'm not sure. I guess this is fine for now until we think of something > > better. > > The easy way to make it reliable would be to call > Destroy(FINAL_STATUS_SAFE_BROWSING) only for the main resource. > > In that case, subresource would still be cancelled, but the status would be > FINAL_STATUS_NOSTATE_PREFETCH_FINISHED. > > And then we can revisit once we know what we want to do for subresources (which > is not very clear yet). Done.
vakh@chromium.org changed reviewers: + csharrison@chromium.org
lgtm +csharrison for safe_browsing_resource_throttle.* since I am not an OWNER of those files but csharrison is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/l lgtm
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org Link to the patchset: https://codereview.chromium.org/2615883003/#ps100001 (title: "main resource only")
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": 100001, "attempt_start_ts": 1484039521273910,
"parent_rev": "383b174a3c321fc6963c50515745d559222ba709", "commit_rev":
"316b5bbb9d35045363bd3954f7c3a68951314cd8"}
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if the main resource is unsafe. If a subresource is unsafe, it is cancelled, but the network requests for other subresources on the same page will typically NOT be canceled. The issue of subresources will be addressed separately in the future. BUG=678941 ========== to ========== [NoStatePrefetch] Implement FINAL_STATUS_SAFEBROWSING This CL plumbs through FINAL_STATUS_SAFEBROWSING for NoStatePrefetch. The SafeBrowsing throttle attempts to destroy the prefetch if the main resource is unsafe. If a subresource is unsafe, it is cancelled, but the network requests for other subresources on the same page will typically NOT be canceled. The issue of subresources will be addressed separately in the future. BUG=678941 Review-Url: https://codereview.chromium.org/2615883003 Cr-Commit-Position: refs/heads/master@{#442543} Committed: https://chromium.googlesource.com/chromium/src/+/316b5bbb9d35045363bd3954f7c3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/316b5bbb9d35045363bd3954f7c3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
