New issue
Advanced search Search tips

Issue 778878 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Proj-Servicification

Blocking:
issue 715640



Sign in to add a comment

S13nServiceWorker: Support request bodies in FetchEvent

Project Member Reported by falken@chromium.org, Oct 27 2017

Issue description

There's two issues now:
- Main resource requests can't handle the new data pipe DataElement (we build a Blob which would DCHECK if there's a data pipe element)
- Subresource requests can't read the body (currently DCHECK)

See some background at https://docs.google.com/document/d/1y3WCk1JNJXx4RHn0I7dV5Xsfg38-OiQc3D892naF588/edit#heading=h.z52v7ipy44fd
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2741b4b4bd974490e26c8a86865653eb337dc5bb

commit 2741b4b4bd974490e26c8a86865653eb337dc5bb
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Nov 02 10:07:25 2017

S13nServiceWorker: Use ResourceRequest instead of ServiceWorkerFetchRequest for FetchEvent#request.

For more context, when NetworkService is enabled, SW interception begins in either:
* for main resources (in browser process): content::ServiceWorkerURLLoaderJob::StartRequest(), or
* for subresources (in renderer process): content::ServiceWorkerSubresourceLoader::StartRequest()

At the time of interception, we have a ResourceRequest from Mojo Loading. The ResourceRequest
has a ResourceRequestBody, which is made of storage::DataElement. 

For FetchEvent#request, the browser process has to send this request and body back to Blink.
Before this patch, we converted the ResourceRequest to ServiceWorkerFetchRequest, which contained a
a Blob for the body. But there were two problems:
* The renderer cannot create a new Blob, in the subresource case. The browser process does
it using Blob functionality only available to the browser.
* Now ResourceRequestBody includes a DataPipe element. We can’t add the DataPipe element
to Blob currently (BlobDataBuilder doesn’t accept it).

The solution is to just pass the request as a ResourceRequest. This allows us to support
request bodies by just using ResourceRequestBody.

The request body flow is now:
(Page/Client) blink::EncodedFormData -> blink::WebHTTPBody -> content::ResourceRequestBody

Then ServiceWorker*Loader* gets the ResourceRequest and sends it back to the service worker:
content::ResourceRequestBody -> blink::WebHTTPBody -> blink::EncodedFormData -> FetchRequestData/FormDataBytesConsumer

The FetchEvent#request is backed by the FetchRequestData.

For now this only changes the S13nServiceWorker path, but it might make sense to later change
non-S13nSW so we don't have two different code paths.

We are also still missing Blob support in FetchEvent#request. ResourceRequestBody in the
NetworkService path uses data pipes instead of Blobs, but when we convert back to WebHTTPBody
the data pipes are ignored. This will be fixed in a future patch.

Bug:  778878 
Change-Id: Ib7192b24ef96990fc84259b30d7a0f0bb258f85d
Reviewed-on: https://chromium-review.googlesource.com/742866
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513442}
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_fetch_dispatcher.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_url_loader_job_unittest.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/common/service_worker/controller_service_worker.mojom
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/common/service_worker/service_worker_event_dispatcher.mojom
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/loader/web_url_request_util.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/controller_service_worker_impl.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/controller_service_worker_impl.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[add] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-request-body.html
[add] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-event-request-body-worker.js
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/Source/modules/fetch/FetchRequestData.cpp
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/Source/platform/exported/WebHTTPBody.cpp
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/public/platform/WebHTTPBody.h
[modify] https://crrev.com/2741b4b4bd974490e26c8a86865653eb337dc5bb/third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ca1c0f98960d5153b66e5546c943be07dd1c0cc

commit 3ca1c0f98960d5153b66e5546c943be07dd1c0cc
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Nov 02 18:41:34 2017

service worker: Cleanup around ServiceWorkerFetchDispatcher

Mostly a follow up to https://crrev.com/513442
* Update tests to use the appropriate SWFetchDispatcher ctor based on
S13nServiceWorker flag.
* Use OnceCallback.
* Avoid passing scoped_refptr by reference.
* Removed now unused CreateRequestBodyBlob() function.
* Other minor cleanups.

Bug:  778878 
Change-Id: I7dffc8dd912bfed7afea6f513449151eade6ec5d
Reviewed-on: https://chromium-review.googlesource.com/751342
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513562}
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_fetch_dispatcher.h
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_url_loader_job.h
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/3ca1c0f98960d5153b66e5546c943be07dd1c0cc/content/browser/service_worker/service_worker_url_request_job.h

Comment 3 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service

Comment 4 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34eb281538f9e6f5ddf635226d12e8a70b9d29e3

commit 34eb281538f9e6f5ddf635226d12e8a70b9d29e3
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Dec 15 03:36:39 2017

Network Service: Teach Blink to read request bodies made of data pipes.

This is needed for service worker since the flow is:
1) Blink sends //content the WebHTTPBody containing blobs.
2) //content makes a ResourceRequestBody where blobs are converted to
data pipes.
3) //content sends the ResourceRequestBody back to Blink for service
worker FetchEvent dispatch. It is converted to a WebHTTPBody with
data pipes.

Bug:  778878 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id260d34dcfd5457e0b4abf72d8e185728eed15a6
Reviewed-on: https://chromium-review.googlesource.com/771092
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524309}
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/common/service_worker/service_worker_fetch_response_callback.mojom
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/network/url_loader_unittest.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/history_serialization.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/loader/web_url_request_util.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/loader/web_url_request_util.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/test/test_blink_web_unit_test_support.cc
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/content/test/test_blink_web_unit_test_support.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/services/network/public/interfaces/data_pipe_getter.mojom
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-request-body.html
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-event-request-body-worker.js
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/BytesConsumer.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/DEPS
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/platform/DEPS
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/platform/exported/WebHTTPBody.cpp
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/platform/network/EncodedFormData.cpp
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/Source/platform/network/EncodedFormData.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/public/platform/Platform.h
[modify] https://crrev.com/34eb281538f9e6f5ddf635226d12e8a70b9d29e3/third_party/WebKit/public/platform/WebHTTPBody.h

Comment 6 by falken@chromium.org, Dec 18 2017

Cc: dmu...@chromium.org
This is mostly implemented, the last thing is to get request bodies to work for network fallback requests. Right now passing the content::ResourceRequestBody  over Mojo/IPC moves the data pipe getter elements out, so after the callsites to DispatchFetchEvent in ServiceWorkeFetchDispatcher and ServiceWorkerSubresourceLoader the body cannot be reused. This is bad because we need to use the body again if the service worker did not provide a response, and we fall back to network.

My idea is to make a copy of the ResourceRequestBody or at least just copy the DataPipeGetter elements inside of it in order to reconstruct the body. We already have a network::mojom::DataPipeGetter::Clone() method.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f93ff357d00911b941797baf0ed1c8be413e8c8f

commit f93ff357d00911b941797baf0ed1c8be413e8c8f
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jan 24 03:44:58 2018

S13nServiceWorker: Support network fallback with blob request body.

For subresource requests only. This makes a clone of the request body
before passing it to the service worker in ServiceWorkerSubresourceLoader.
This way if the service worker does network fallback, the request
can be passed to network with the request body intact.

Change-Id: I00c1861449e997a50c417336961a7c8c62bc710e
Bug:  778878 
Reviewed-on: https://chromium-review.googlesource.com/867817
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531414}
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/common/service_worker/service_worker_loader_helpers.h
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/renderer/DEPS
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https-expected.txt
[modify] https://crrev.com/f93ff357d00911b941797baf0ed1c8be413e8c8f/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/010e9daec33af68f8cae5e74a2c09397d747cd95

commit 010e9daec33af68f8cae5e74a2c09397d747cd95
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Jan 25 03:09:30 2018

S13nServiceWorker: Remove the chromium.fetch-event-request-body.html test

This test case is covered by WPT test fetch-event.https.html, which is now
passing in S13nServiceWorker.

R=kinuko, shimazu

Bug:  778878 
Change-Id: I2d79be1c20ebd363f4ff06cbe708ce5bee287a23
Reviewed-on: https://chromium-review.googlesource.com/882787
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531801}
[delete] https://crrev.com/5688e36bf038d9d42c01079eaa48c118c3b832fd/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-event-request-body.html
[delete] https://crrev.com/5688e36bf038d9d42c01079eaa48c118c3b832fd/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-event-request-body-worker.js

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f32e9c093b7c0d060cdafa72596b758cb94a285

commit 9f32e9c093b7c0d060cdafa72596b758cb94a285
Author: Matt Falkenhagen <falken@chromium.org>
Date: Sat Jan 27 01:45:44 2018

S13nServiceWorker: Add tests for network fallback for navigations with request bodies

Originally I planned to clone the request body for main resource requests,
similar to subresource requests, but it looks unnecessary for now because
main resource request bodies don't have data pipe getter elements. They
are only created by the renderer when converting from a Blob, which doesn't
happen for navigations.

So this patch:
- Adds a DCHECK to the main resource request handling code that there are
no data pipe elements.
- Adds a WPT test for network fallback with a request body (using just strings
since we can't test uploading a file with WPT as far as a I can tell, it needs
user interaction or a special test harness flag).
- Adds a http/tests/local/ test case for file upload with network fallback.
This test file was already passing, so the failing expectation is also removed.

R=kinuko, shimazu

Bug:  778878 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I7a5f7a72ef9ac2ca3ffbe54549739ca6bcc8d071
Reviewed-on: https://chromium-review.googlesource.com/885684
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532121}
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https-expected.txt
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-event.https.html
[add] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/echo-content.py
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html
[add] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-test.php
[modify] https://crrev.com/9f32e9c093b7c0d060cdafa72596b758cb94a285/third_party/WebKit/LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js

Status: Fixed (was: Started)
I think this is now done.

Sign in to add a comment