devtools 'disable cache' breaks doc.write script blocking intervention |
||||||||||||||||
Issue descriptionThe doc.write script blocking intervention allows doc.written scripts to load only if they are available from cache. This is implemented by returning WebCachePolicy::ReturnCacheDataDontLoad from FrameFetchContext::resourceRequestCachePolicy for these scripts. When devtools is open and 'disable cache' is selected in the network panel, these scripts are no longer blocked. This is a blocker to ship this feature in M54, since developers need to be able to reproduce the blocking behavior when using devtools. To repro: Start the blink layout tests: ninja -C out/Release -j1000 blink_tests cd third_party/WebKit/Tools/Scripts ./run-blink-httpd Open Chrome like so: google-chrome-unstable --blink-settings=disallowFetchForDocWrittenScriptsInMainFrame=true --user-data-dir=$(mktemp -d) (or substitute another chrome binary, with the same flags) navigate to: http://127.0.0.1:8000/loading/doc-write-sync-third-party-script-block-all-conn-types.html The screen should show: This is a testharness.js-based test. FAIL Document.written scripts are not blocked by default. assert_true: expected true got false PASS Document.written scripts are blocked on all connections when disallowFetchForDocWrittenScriptsInMainFrame is specified. Harness: the test ran to completion. The initial 'Document.written scripts are not blocked by default.' test will fail because we've turned on script blocking for all scripts via the blink-settings flag. The second 'Document.written scripts are blocked on all connections when disallowFetchForDocWrittenScriptsInMainFrame is specified.' should pass, again because we've turned on script blocking via the blink-settings flag. Close chrome, and re-open it: google-chrome-unstable --blink-settings=disallowFetchForDocWrittenScriptsInMainFrame=true --user-data-dir=$(mktemp -d) open devtools, select the network panel, and check the 'Disable cache' option. navigate to http://127.0.0.1:8000/loading/doc-write-sync-third-party-script-block-all-conn-types.html Observe that the expected script blocking behavior no longer happens. Instead the UI shows: This is a testharness.js-based test. PASS Document.written scripts are not blocked by default. FAIL Document.written scripts are blocked on all connections when disallowFetchForDocWrittenScriptsInMainFrame is specified. assert_false: expected false got true Harness: the test ran to completion. This indicates that neither doc.written script was blocked. It seems that devtools is breaking the intervention when 'disable cache' is selected. Perhaps it disables our 'load only from cache' load flag. We need it to not do that. Assigning to Josh to start. Josh, please take this or reassign.
,
Aug 18 2016
When "disable cache" is checked the NetworkAgentState::cacheDisabled is true, and the effect is that all requests have their cache policy changed to WebCachePolicy::BypassingCache. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp?q=cacheDisabled&sq=package:chromium&dr=C&l=553 Mapping ReturnCacheDataDontLoad to WebCachePolicy::BypassingCache probably isn't right as the network should never be invoked on such a request. Instead, it seems we should just fail the request.
,
Aug 18 2016
There is a handy InspectorNetworkAgent::shouldBlockRequest method that we could block if the cache policy matches, but unfortunately the cache policy hasn't been changed for doc written resources yet at that point.
,
Aug 19 2016
Okay, so one solution I've demonstrated is to call initializeResourceRequest() before context().canRequest() in ResourceFetcher::requestResource(), this way the caching policy is set before devtools determines if it needs to block it. Then devtools will block cache-only requests if "disable cache" is checked. This is a bit weird however since the inspector shows that dev-tools blocked the request.
,
Aug 19 2016
The biggest problem with blocking the request at requestResource() is that onerror isn't thrown. I don't see a way to get to onerror without actually sending the request to the browser.
,
Aug 23 2016
As luck would have it, https://codereview.chromium.org/2231523002 will change ResourceFetcher to call onerror if shouldBlockRequest blocks. CL https://codereview.chromium.org/2266913002/ is ready to land once the dependency does.
,
Sep 6 2016
Thanks Josh for moving this forward. It looks like the dependent patch https://codereview.chromium.org/2231523002 has been idle for a few weeks. Anything we can do to make sure it moves forward and lands?
,
Sep 6 2016
I spoke to engedy@ a couple of weeks ago and he said he'd get to it soon. I emailed again just now :)
,
Sep 6 2016
Looks like it'll be another week or two. Might as well wait since we have some time for the M55 branch.
,
Sep 6 2016
sounds good, thanks!
,
Oct 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/938a1f2e20387f1008f4d34536b705da109792ac commit 938a1f2e20387f1008f4d34536b705da109792ac Author: jkarlin <jkarlin@chromium.org> Date: Mon Oct 17 17:10:05 2016 DevTools Disable cache should properly handle LoadOnlyFromCache resources Currently, checking the "disable cache" button on the networking page in dev tools causes all requests to run with a bypass cache directive. Even those resources meant to be fetched exclusively from the cache. This means resources meant to be served only from the cache wind up being served from the network. This CL causes such requests to fail early, as would happen if the object were not in cache. BUG= 634189 Review-Url: https://codereview.chromium.org/2266913002 Cr-Commit-Position: refs/heads/master@{#425706} [modify] https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp [modify] https://crrev.com/938a1f2e20387f1008f4d34536b705da109792ac/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
,
Oct 19 2016
No issues in canary so far. Requesting merge to beta.
,
Oct 19 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87b20b798d7d9c054bdea42b20b7ad9c772077c7 commit 87b20b798d7d9c054bdea42b20b7ad9c772077c7 Author: Josh Karlin <jkarlin@chromium.org> Date: Wed Oct 19 18:06:07 2016 DevTools Disable cache should properly handle LoadOnlyFromCache resources Currently, checking the "disable cache" button on the networking page in dev tools causes all requests to run with a bypass cache directive. Even those resources meant to be fetched exclusively from the cache. This means resources meant to be served only from the cache wind up being served from the network. This CL causes such requests to fail early, as would happen if the object were not in cache. BUG= 634189 Review-Url: https://codereview.chromium.org/2266913002 Cr-Commit-Position: refs/heads/master@{#425706} (cherry picked from commit 938a1f2e20387f1008f4d34536b705da109792ac) Review URL: https://codereview.chromium.org/2437783002 . Cr-Commit-Position: refs/branch-heads/2883@{#189} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/87b20b798d7d9c054bdea42b20b7ad9c772077c7/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp [modify] https://crrev.com/87b20b798d7d9c054bdea42b20b7ad9c772077c7/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87b20b798d7d9c054bdea42b20b7ad9c772077c7 commit 87b20b798d7d9c054bdea42b20b7ad9c772077c7 Author: Josh Karlin <jkarlin@chromium.org> Date: Wed Oct 19 18:06:07 2016 DevTools Disable cache should properly handle LoadOnlyFromCache resources Currently, checking the "disable cache" button on the networking page in dev tools causes all requests to run with a bypass cache directive. Even those resources meant to be fetched exclusively from the cache. This means resources meant to be served only from the cache wind up being served from the network. This CL causes such requests to fail early, as would happen if the object were not in cache. BUG= 634189 Review-Url: https://codereview.chromium.org/2266913002 Cr-Commit-Position: refs/heads/master@{#425706} (cherry picked from commit 938a1f2e20387f1008f4d34536b705da109792ac) Review URL: https://codereview.chromium.org/2437783002 . Cr-Commit-Position: refs/branch-heads/2883@{#189} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/87b20b798d7d9c054bdea42b20b7ad9c772077c7/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp [modify] https://crrev.com/87b20b798d7d9c054bdea42b20b7ad9c772077c7/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Nov 7 2016
,
Nov 7 2016
I didn't realize till now that this was merged to 55. We need to address the regression this introduced ASAP (issue 660438), as this CL is spamming all devtools users an incorrect message about blocked requests. I've added a extremely simple repro at https://bugs.chromium.org/p/chromium/issues/detail?id=660438#c5. Looks like this issue was pointed out in code review but I don't believe it was addressed? https://codereview.chromium.org/2266913002#msg41 jkarlin, how do you want to handle this? If a variation on https://codereview.chromium.org/2479713003 could be merged to 55, can you help expedite the fix? Chrome DevSummit is this week and a number of speakers have asked me if they should edit their screenshots and demos in order to avoid this bug.
,
Nov 7 2016
,
Nov 7 2016
Happy to help, responding in issue 660438.
,
Nov 7 2016
We are planning to cut the Stable RC at 4.00 pm . Is this a blocker for M54- Stable release?
,
Nov 7 2016
No this is an issue for M55. Removing RBS label.
,
Nov 7 2016
Is this issue specific to any OS or all OSs? Also this bug has been marked as M55 Beta blocker for this week release, please try to have fix landed/merged to M55 before 5:00 PM PT tomorrow, Tuesday (11/08/16).
,
Nov 8 2016
,
Nov 8 2016
The fix for this is being tracked in bug 660438, so I'm moving the RBB and other labels to that bug.
,
Dec 2 2016
|
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by jkarlin@chromium.org
, Aug 11 2016