Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582833002:
Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. (Closed)
|
Created:
4 years ago by Marijn Kruisselbrink Modified:
4 years ago CC:
blink-reviews, chromium-reviews, falken+watch_chromium.org, gavinp+loader_chromium.org, horo+watch_chromium.org, Nate Chapin, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, kinuko+serviceworker, loading-reviews_chromium.org, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tyoshino+watch_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix foreign fetch intercepting CORS preflighted requests if preflight was in cache.
This moves setting of the skip service worker flag on CORS preflighted requests
to happen earlier so that even non-simple requests for which we don't end up
sending a preflight request still skip any service workers.
BUG=674370
Committed: https://crrev.com/6c2c67d53758fdae01937ae43b8e5a30ff9854ea
Cr-Commit-Position: refs/heads/master@{#439863}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Messages
Total messages: 22 (15 generated)
The CQ bit was checked by mek@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...
Description was changed from ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. BUG=674370 ========== to ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. This moves setting of the skip service worker flag on CORS preflighted requests to happen earlier so that even non-simple requests for which we don't end up sending a preflight request still skip any service workers. BUG=674370 ==========
mek@chromium.org changed reviewers: + falken@chromium.org, yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Sorry I somehow missed this yesterday. sw lgtm (but actually non-owner for these files) https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:373: // controlled by a SW at this point, a new SW may be controlling the page so we only get to this point when the page is not controlled by a SW? Can the comment be revised to say "Although the page is not controlled by a SW at this point"? https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:374: // when this request gets send later. We should not send the actual request sent later https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:378: // result of the preflight cached already. https://crbug.com/674370 Not sure I understand the bug.. the cache was surviving between layout test iterations? Could you add a comment to the bug for future reference about the cause?
lgtm https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html (right): https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:241: .then(() => fetch(remote_url, {method: 'SPECIAL'})) Isn't it better to have two separate tests?
The CQ bit was checked by mek@chromium.org to run a CQ dry run
https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html (right): https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:241: .then(() => fetch(remote_url, {method: 'SPECIAL'})) On 2016/12/20 at 07:42:26, yhirano wrote: > Isn't it better to have two separate tests? I don't think so. I don't like having dependencies between tests (even if the tests are in the same file/are guaranteed by promise_test to be run in order). I think it's much clearer to just have all this in the same test. (unless you meant have two tests, one that just does a cors-preflighted fetch once as before, and one that does the exact same thing twice as this test does now (but both with different SW scopes to avoid any chance of dependency between the two). I support I could do that, but the benefit of that seems rather small). https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:373: // controlled by a SW at this point, a new SW may be controlling the page On 2016/12/20 at 05:27:42, falken wrote: > so we only get to this point when the page is not controlled by a SW? Yes, ::start() only ends up calling dispatchInitialRequest (which then calls makeCrossOriginAccessRequest) when it decides that the page is not controlled by a SW (otherwise all the CORS logic is bypassed). > Can the comment be revised to say "Although the page is not controlled by a SW at this point"? Done https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:374: // when this request gets send later. We should not send the actual request On 2016/12/20 at 05:27:42, falken wrote: > sent later Done https://codereview.chromium.org/2582833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:378: // result of the preflight cached already. https://crbug.com/674370 On 2016/12/20 at 05:27:42, falken wrote: > Not sure I understand the bug.. the cache was surviving between layout test iterations? Could you add a comment to the bug for future reference about the cause? Yes, that seems to be what was happening. Not sure if it is expected or not that the preflight cache sometimes survives. Added a comment to the bug.
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.
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2582833002/#ps20001 (title: "address comments")
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": 20001, "attempt_start_ts": 1482260490217430,
"parent_rev": "8f9faa85ed92a0931555e353a391fa567eb2b260", "commit_rev":
"81198278e25a679fcb76812ef90fc1db573b7314"}
Message was sent while issue was closed.
Description was changed from ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. This moves setting of the skip service worker flag on CORS preflighted requests to happen earlier so that even non-simple requests for which we don't end up sending a preflight request still skip any service workers. BUG=674370 ========== to ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. This moves setting of the skip service worker flag on CORS preflighted requests to happen earlier so that even non-simple requests for which we don't end up sending a preflight request still skip any service workers. BUG=674370 Review-Url: https://codereview.chromium.org/2582833002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. This moves setting of the skip service worker flag on CORS preflighted requests to happen earlier so that even non-simple requests for which we don't end up sending a preflight request still skip any service workers. BUG=674370 Review-Url: https://codereview.chromium.org/2582833002 ========== to ========== Fix foreign fetch intercepting CORS preflighted requests if preflight was in cache. This moves setting of the skip service worker flag on CORS preflighted requests to happen earlier so that even non-simple requests for which we don't end up sending a preflight request still skip any service workers. BUG=674370 Committed: https://crrev.com/6c2c67d53758fdae01937ae43b8e5a30ff9854ea Cr-Commit-Position: refs/heads/master@{#439863} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6c2c67d53758fdae01937ae43b8e5a30ff9854ea Cr-Commit-Position: refs/heads/master@{#439863} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
