SafeBrowsingParallelResourceThrottle: should avoid out-of-band cancellation. |
||||||||||
Issue descriptionIt turned out that ResourceLoader doesn't handle out-of-band cancellation correctly in some cases. One example: - throttle A defers following a redirect; - throttle B does an out-of-band cancellation; - throttle A resumes; - in this case, ResourceLoader tries to resume a cancelled URLRequest and causes DCHECKs. Since we expect ResourceLoader to go away when Network service is ready, it may not be worthwhile to fix ResourceLoader (according to mmenke it would require quite some effort to fix the state machine properly). We could change SafeBrowsingParallelResourceThrottle to avoid out-of-band cancellation.
,
Oct 19 2017
,
Oct 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9a77f239dd32950fd1b02eba12cc61cc590fab4 commit a9a77f239dd32950fd1b02eba12cc61cc590fab4 Author: Yuzhu Shen <yzshen@chromium.org> Date: Fri Oct 20 08:01:35 2017 safe_browsing::BaseParallelResourceThrottle: avoid out-of-band cancellation. The state machine of ResourceLoader has issues with out-of-band cancellation. For example, an out-of-band cancellation of one throttle followed by resuming redirect of another throttle will confuse the state machine. Without this CL, the following test is flaky when S13nSafeBrowsingParallelUrlCheck is enabled: PrerenderBrowserTest.PrerenderSafeBrowsingServerRedirect BUG= 776509 Change-Id: I2ad94913ebb20cd7da4e8500f10d9d3e45c47adf Reviewed-on: https://chromium-review.googlesource.com/716921 Commit-Queue: Yuzhu Shen <yzshen@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Nate Fischer <ntfschr@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#510374} [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/android_webview/browser/aw_safe_browsing_resource_throttle.cc [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/chrome/test/BUILD.gn [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/base_resource_throttle.h [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/browser/BUILD.gn [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/browser/DEPS [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/browser/base_parallel_resource_throttle.cc [modify] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/browser/base_parallel_resource_throttle.h [add] https://crrev.com/a9a77f239dd32950fd1b02eba12cc61cc590fab4/components/safe_browsing/browser/base_parallel_resource_throttle_unittest.cc
,
Oct 20 2017
,
Oct 20 2017
,
Oct 21 2017
Please add appropriate OSs.
,
Oct 21 2017
,
Oct 21 2017
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 21 2017
The DEPS change is to allow the newly-added test file to include a test utility.
,
Oct 21 2017
Before we approve merge for CL listed #3, could you pls confirm change is well baked/verified in Canary and safe to merge to M63? Also this bug is currently marked as M63, can this wait until M64 or please provide justification for M63 merge. Thank you.
,
Oct 23 2017
Thanks, Krishna! I have confirmed that this CL doesn't result in any crash in canary. It is safe to merge. This CL fixes an issue for a feature that we would like to roll out using finch experiment[1] in M63. Therefore I hope to merge it to M63. Please feel free to ping me if you need further details. I am more than happy to chat about it. ============================================= [1] Please see https://docs.google.com/a/chromium.org/document/d/1p0yYIxUtgLOWwYe51pyWhORB5_BdR_xVpaJs-9Qs7D8/edit?usp=sharing
,
Oct 23 2017
Approving merge to M63 branch 3239 based on comment #11. Thank you.
,
Oct 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32a6a1ccb91986bd815849eb87486e8b14a7e22c commit 32a6a1ccb91986bd815849eb87486e8b14a7e22c Author: Yuzhu Shen <yzshen@chromium.org> Date: Mon Oct 23 17:46:39 2017 safe_browsing::BaseParallelResourceThrottle: avoid out-of-band cancellation. The state machine of ResourceLoader has issues with out-of-band cancellation. For example, an out-of-band cancellation of one throttle followed by resuming redirect of another throttle will confuse the state machine. Without this CL, the following test is flaky when S13nSafeBrowsingParallelUrlCheck is enabled: PrerenderBrowserTest.PrerenderSafeBrowsingServerRedirect BUG= 776509 TBR=yzshen@chromium.org (cherry picked from commit a9a77f239dd32950fd1b02eba12cc61cc590fab4) Change-Id: I2ad94913ebb20cd7da4e8500f10d9d3e45c47adf Reviewed-on: https://chromium-review.googlesource.com/716921 Commit-Queue: Yuzhu Shen <yzshen@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Nate Fischer <ntfschr@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#510374} Reviewed-on: https://chromium-review.googlesource.com/733805 Reviewed-by: Yuzhu Shen <yzshen@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#158} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/android_webview/browser/aw_safe_browsing_resource_throttle.cc [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/chrome/test/BUILD.gn [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/base_resource_throttle.h [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/browser/BUILD.gn [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/browser/DEPS [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/browser/base_parallel_resource_throttle.cc [modify] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/browser/base_parallel_resource_throttle.h [add] https://crrev.com/32a6a1ccb91986bd815849eb87486e8b14a7e22c/components/safe_browsing/browser/base_parallel_resource_throttle_unittest.cc
,
Oct 23 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by yzshen@chromium.org
, Oct 19 2017