Figure out what to do about network fallback and CORS preflight for foreign fetch |
|||
Issue descriptionCurrently the way CORS requests are implemented in the context of service workers involves the renderer synchronously deciding that a request is going to be intercepted by a service worker (because the page is controlled), and then skipping any kind of CORS preflight check. The browser process then does its service worker thing, and if in the end the conclusion is that the request should be handled by the network after all it gets bounced back to the renderer which will restart the request. In the context of foreign fetch this somewhat breaks. With foreign fetch the renderer can't synchronously decide if a service worker should get to have a go at the request before possible dealing with CORS preflights etc. I'm not sure what the answer is here. It's probably fine for now to limit what foreign fetch is allowed to do (to limit the need for fallback-to-network from foreign fetch workers to work) and not officially support requests that would have needed CORS preflights (as in, these preflights might still get send out even if the request would have been ending up handled by a foreign fetch service worker). Maybe solutions could involve moving all the CORS preflight logic to the browser process, but that's probably far from trivial. And introducing an extra round-trip through the browser process for any cross origin requests that could possibly be intercepted by foreign fetch is likely not a good solution either.
,
Apr 18 2016
For what it's worth, the way network fallback and CORS preflight works for regular service workers is already broken/can crash the renderer because of race conditions: if an uncontrolled client sends out a request that involves a CORS preflight, but by the time the preflight receives a response the client is controlled, the renderer will crash (I have a test case that trivially reproduces it, using a custom server which delays responding to OPTIONS requests).
,
Apr 19 2016
horo@ is most familiar with CORS and SW. Can you give more details on comment #2? I think the renderer has a basic assumption that the document lives and dies with the same SW or no SW. Do you use clients.claim() to go from uncontrolled to controlled? (I thought we handled that case somehow.)
,
Apr 19 2016
https://codereview.chromium.org/1894183003 is the test case that I came up with to cause this code to crash with regular fetch: - start preflight request from uncontrolled client - install worker - clients.claim - (tell server to) respond to preflight - cors request is send to service worker - service worker doesn't handle request - fallback to network code crashes This case is probably easier to fix than figuring out what to do about foreign fetch though (to be spec compliant the actual request after a preflight should never be intercepted by a serviceworker anyway. Once the preflight is send it should already have been decided that the request should not be intercepted).
,
Apr 19 2016
Thanks, I opened issue 604583 for that crash.
,
Apr 19 2016
The current CORS preflights and fallback mechanism is relying on the fact that once the page is controlled by a SW, the page will not be freed from SW forever. So once the renderer knows that the page is controlled by a SW, it can skip the preflight check when sending CORS request. But in the foreign fetch case, the renderer doesn't know whether the fetch requests will go to SW or not. So this mechanism doesn't work. Yes, moving the CORS preflight to the browser process is one solution. I wrote a design doc about it last year. https://docs.google.com/a/chromium.org/document/d/1LWyPoIptnBdGxG0dW3yCl4abIcdfAOx0yobaw_2wDkw/edit?usp=sharing But we don't have any progress in implementation...
,
Jun 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0 commit 2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0 Author: mek <mek@chromium.org> Date: Thu Jun 30 22:17:25 2016 Skip foreign fetch when the skipServiceWorker flag is set on a request. Foreign fetch used to completely ignore the skipServiceWorker flag. Just skipping foreign fetch whenever skipServiceWorker is true would not be correct as the flag is both set in situations where a request needs to bypass service workers, but also when the renderer doesn't believe the client making the request is controlled by a service worker. In that second case foreign fetch would still need to process the request. To fix this, this changes skipServiceWorker to an enum with three states, in order to distinguish between skipping all service workers (for CORS preflight, dev-tools skip service worker, force-refresh, etc) and skipping just the controlling service worker. This also makes sure that (for now) any request that requires a CORS preflight will not get intercepted by foreign fetch. BUG= 540509 , 604084 Review-Url: https://codereview.chromium.org/2105503002 Cr-Commit-Position: refs/heads/master@{#403312} [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/browser/loader/resource_dispatcher_host_impl.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/browser/service_worker/foreign_fetch_request_handler.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/browser/service_worker/foreign_fetch_request_handler.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/request_info.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/request_info.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/web_url_loader_impl.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/web_url_loader_impl.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/web_url_request_util.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/child/web_url_request_util.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/common/resource_messages.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/common/resource_request.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/common/service_worker/service_worker_types.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/public/renderer/resource_fetcher.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/fetchers/resource_fetcher_impl.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/fetchers/resource_fetcher_impl.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/pepper/pepper_url_loader_host.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/render_frame_impl.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/content/renderer/shared_worker/embedded_shared_worker_stub.cc [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/loader/FrameLoader.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/modules/fetch/FetchManager.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/platform/network/ResourceRequest.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/platform/network/ResourceRequest.h [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/Source/platform/network/ResourceRequestTest.cpp [modify] https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0/third_party/WebKit/public/platform/WebURLRequest.h
,
Aug 18 2016
Hmm, thinking about this a bit more, I don't see any reason why when a foreign fetch service worker decides not to handle a request we can't just always directly fall back to the network, rather than falling back to the renderer. After all from the point of view of the renderer these requests that foreign ended up intercepting are already going straight to the network anyway, so all the correct CORS checks are in place. And specifically in the case of CORS preflight requests, the requests bypass the foreign fetch service worker anyway. Disallowing falling back to the network from a foreign fetch handler was only necessary before I completely disabled intercepting requests involving CORS preflights.
,
Aug 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/976f647c1325d73b120e234289ee9d7ef0973767 commit 976f647c1325d73b120e234289ee9d7ef0973767 Author: mek <mek@chromium.org> Date: Mon Aug 22 20:35:44 2016 Allow a foreign fetch event handler to not handle an event. Previously when a foreign fetch event handler decided not to handle an event that would be considered a failure, as bouncing the request back through the renderer (as would be necesary in the case of a cross origin request to a regular service worker's fetch event) doesn't work for foreign fetch. But since all the CORS logic is still in place for requests that are intercepted by foreign fetch (and in particular requests with preflights can't be intercepted by foreign fetch) it is always safe to fall back to the network directly rather than falling back to the renderer for foreign fetch requests. BUG= 604084 Review-Url: https://codereview.chromium.org/2255383002 Cr-Commit-Position: refs/heads/master@{#413525} [modify] https://crrev.com/976f647c1325d73b120e234289ee9d7ef0973767/content/browser/service_worker/service_worker_url_request_job.cc [modify] https://crrev.com/976f647c1325d73b120e234289ee9d7ef0973767/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e1b65b8cd5a5f4ddb4e37f68a61432306741107 commit 4e1b65b8cd5a5f4ddb4e37f68a61432306741107 Author: Takeshi Yoshino <tyoshino@chromium.org> Date: Fri Jun 23 11:10:52 2017 Clean up the CORS preflight part of DocumentThreadableLoader::MakeCrossOriginAccessRequest() The bunch of if-clauses are hard to read. Reorganize them for better readability. Unify the multiple checks on IsExternalRequest. Early return for IsExternalRequest(). Don't check CORS-safelisted-ness when either of the external request or forced preflight condition is already met. Clarify where the ServiceWorkerMode configuration is required and its reason for each place. Bug: 695808 , 604084 Change-Id: I2a056afeebea4d6472e60c52cc72fd58c6bef703 Reviewed-on: https://chromium-review.googlesource.com/542676 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Commit-Queue: Takeshi Yoshino <tyoshino@chromium.org> Cr-Commit-Position: refs/heads/master@{#481846} [modify] https://crrev.com/4e1b65b8cd5a5f4ddb4e37f68a61432306741107/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp [modify] https://crrev.com/4e1b65b8cd5a5f4ddb4e37f68a61432306741107/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
,
Nov 27 2017
Foreign fetch will be removed. |
|||
►
Sign in to add a comment |
|||
Comment 1 by mek@chromium.org
, Apr 16 2016