New issue
Advanced search Search tips

Issue 911930 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task

Blocking:
issue 789854


Participants' hotlists:
ServiceWorkerOnionSoup


Sign in to add a comment

Use blink::mojom::FetchAPIRequest for service workers

Project Member Reported by shimazu@chromium.org, Dec 5

Issue description

We have two types of "request" objects in mojom API surface: network.mojom.URLRequest and blink.mojom.FetchAPIRequest.
Currently, DispatchFetchEventParams uses network.mojom.URLRequest and other APIs using request objects like BackgroundFetch and CacheStorages are using blink.mojom.FetchAPIRequest.
We've had discussion on when to use network.mojom.URLRequest and blink.mojom.FetchAPIRequest at a doc [1], and now we concluded that blink.mojom.FetchAPIRequest would stand for blink::Request (which is web-exposed struct) and network.mojom.URLRequest would stand for blink::ResourceRequest (which is a internal representation of request used in fetch() implementation).

This is a prerequisite step to move content.mojom.ServiceWorker interface to blink, since we need to remove a dependency on network.mojom.URLRequest, which is not having a blink-side representation.

[1] https://docs.google.com/document/d/1b9KS0DXFnc6jifpHcUhX04XZcJEAkCC4HCbDwDgeM50/edit#
 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 7

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

commit 9a115b526e20b66a78426ab092bc43600f9a27a4
Author: Leon Han <leon.han@intel.com>
Date: Fri Dec 07 04:38:51 2018

[ServiceWorker] A cleanup in service_worker_context_client.cc.

The ToWebServiceWorkerRequest() is used only for dispatching fetch event
to service workers, and is not supposed to be used for other purposes
even in the future.

BUG=911930

Change-Id: Ie57f1b65c927ad3aed675b744135ed48f37df9b8
Reviewed-on: https://chromium-review.googlesource.com/c/1365074
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Leon Han <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#614595}
[modify] https://crrev.com/9a115b526e20b66a78426ab092bc43600f9a27a4/content/renderer/service_worker/service_worker_context_client.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 17

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

commit 55825ccfad75a22821091ee15977e69d3288f1cf
Author: Leon Han <leon.han@intel.com>
Date: Mon Dec 17 12:16:20 2018

[OnionSoup] Use FetchAPIRequest for FetchEvent#request dispatched to SW

Previously we were using network.mojom.URLRequest (a [Native] mojom
struct typemapping with network::ResourceRequest) for FetchEvent#request
dispatched to SW,
The reason not to use FetchAPIRequest: (see http://crrev.com/c/742866)
  - SW interception code (for loading both main resources and
    subresources) gets a network::ResourceRequest from Mojo Loading.
  - the ResourceRequest has a ResourceRequestBody.
  - in the subresource case (in the renderer process) we can not turn a
    ResourceRequestBody into a Blob to be carried as
    FetchAPIRequest.blob.
This CL adds a new member FetchAPIRequest.body to solve the above
problem.
Now SW interception code can convert a network::ResourceRequest (with
body) to a FetchAPIRequest without data loss, then dispatch the
FetchAPIRequest as FetchEvent#request to SW.

Background:
We had a discussion about network.mojom.URLRequest and FetchAPIRequest at
https://docs.google.com/document/d/1b9KS0DXFnc6jifpHcUhX04XZcJEAkCC4HCbDwDgeM50/edit#
with the concensus:
  - network.mojom.URLRequest is kind of internal data in the loading
    stack, while FetchAPIRequest should be a direct representation of a
    JS Request object for all API implementations like Background Fetch,
    Cache Storage and Service Worker, with no need to care how the
    loading logic happens.
  - Ideally, URLRequest will be typemapping with
    network::ResourceRequest for Chromium variant and
    blink::ResourceRequest for Blink variant in the future.
  - FetchAPIRequest aims to fully support blink::Request.

The request body flow is unchanged:
(Page/Client) blink::EncodedFormData -> blink::WebHTTPBody -> network::ResourceRequestBody
ResourceRequestBody -> blink::WebHTTPBody -> blink::EncodedFormData -> FetchRequestData/FormDataBytesConsumer
But in the future with more Onion Soup effort it would be like:
(Page/Client) blink::EncodedFormData --typemapping-> network.mojom.ResourceRequestBody --typemapping-> network::ResourceRequestBody
network::ResourceRequestBody --typemapping-> network.mojom.ResourceRequestBody --typemapping-> blink::EncodedFormData -> FetchRequestData/FormDataBytesConsumer

BUG=911930

Change-Id: Ia96cace7ff3bf7a3d2e66056c606cfb4a02fbb0b
Reviewed-on: https://chromium-review.googlesource.com/c/1367365
Commit-Queue: Leon Han <leon.han@intel.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617092}
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_fetch_dispatcher.h
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_navigation_loader_unittest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/common/BUILD.gn
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/common/background_fetch/background_fetch_types.cc
[add] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/common/fetch/OWNERS
[add] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/common/fetch/fetch_request_type_converters.cc
[add] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/common/fetch/fetch_request_type_converters.h
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/renderer/service_worker/service_worker_context_client_unittest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/renderer/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/services/network/public/cpp/url_request.typemap
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/services/network/public/mojom/url_loader.mojom
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/third_party/blink/public/mojom/fetch/fetch_api_request.mojom
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/third_party/blink/public/mojom/service_worker/dispatch_fetch_event_params.mojom
[modify] https://crrev.com/55825ccfad75a22821091ee15977e69d3288f1cf/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 20

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

commit 793c3c5282125961db5401a2b9485a66798adcd8
Author: Leon Han <leon.han@intel.com>
Date: Thu Dec 20 07:49:00 2018

[ServiceWorker] Remove blink.mojom.FetchAPIRequest.client_id

blink.mojom.FetchAPIRequest.client_id is not used anywhere so we'd
just remove it now.

BUG=911930

Change-Id: I0f64df6f91daa7d6bd52f2a3815b2f070e0a5291
Reviewed-on: https://chromium-review.googlesource.com/c/1377603
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Leon Han <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#618134}
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/content/common/background_fetch/background_fetch_types.cc
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/content/common/service_worker/service_worker_types.proto
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/content/common/service_worker/service_worker_types_unittest.cc
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/content/common/service_worker/service_worker_utils.cc
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/third_party/blink/public/mojom/fetch/fetch_api_request.mojom
[modify] https://crrev.com/793c3c5282125961db5401a2b9485a66798adcd8/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 26

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

commit 1ab13f85efc9219f9b32d73f3c57f17af426c747
Author: Leon Han <leon.han@intel.com>
Date: Wed Dec 26 12:48:37 2018

[OnionSoup] Remove DispatchFetchEventParams.request_body_blob_ptrs

Previously, in case of S13nServiceWorker without NetworkService,
to transfer those Blob elements of network::ResourceRequestBody when
calling blink.mojom.ServiceWorker.DispatchFetchEvent(), additionally
ServiceWorkerSubresourceLoader prepares an array of BlobPtrInfo for
those Blob data and transfers it as
DispatchFetchEventParams.request_body_blob_ptrs.
Then ServiceWorkerContextClient gets it and puts it into a
blink::WebHTTPBody as part of WebServiceWorkerRequest to enter Blink.

However, we can see that DispatchFetchEventParams.request_body_blob_ptrs
is unnecessary to be passed through the Mojo call,
ServiceWorkerContextClient itself should be able to get these
information from the received network::ResourceRequestBody.
This CL lets ServiceWorkerContextClient do that and then removes
DispatchFetchEventParams.request_body_blob_ptrs.

crrev.com/c/1390323 relies on this CL to have ServiceWorkerContextClient
get those BlobPtrInfos from a network::ResourceRequestBody sent by
another fetch event dispatcher ServiceWorkerURLRequestJob.

BUG=911930
TBR=thestig@chromium.org
for trivial changes in base/threading/thread_restrictions.h

Change-Id: I383732b60329a9cd7c4204c7fbeedff10eee1990
Reviewed-on: https://chromium-review.googlesource.com/c/1390263
Reviewed-by: Leon Han <leon.han@intel.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Leon Han <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#618924}
[modify] https://crrev.com/1ab13f85efc9219f9b32d73f3c57f17af426c747/base/threading/thread_restrictions.h
[modify] https://crrev.com/1ab13f85efc9219f9b32d73f3c57f17af426c747/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/1ab13f85efc9219f9b32d73f3c57f17af426c747/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/1ab13f85efc9219f9b32d73f3c57f17af426c747/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/1ab13f85efc9219f9b32d73f3c57f17af426c747/third_party/blink/public/mojom/service_worker/dispatch_fetch_event_params.mojom

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 27

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

commit 28e779303ba5c598a75dc00352b26d1b82056d14
Author: Leon Han <leon.han@intel.com>
Date: Thu Dec 27 06:57:24 2018

[OnionSoup] Use FetchAPIRequest.body instead of FetchAPIRequest.blob in SW code

Previously, in non-S13nServiceWorker case, a
network::ResourceRequestBody was being converted to a blob as
FetchAPIRequest.blob to transfer data for FetchEvent#request#body
dispatched to service workers.

With crrev.com/c/1367365 introducing a new field FetchAPIRequest.body of
type network::mojom::URLRequestBody which has a typemap to
network::ResourceRequestBody, now we can use it instead to avoid the
data conversion above.

After this CL, FetchAPIRequest.blob has only one user left: Background
Fetch API implementation.

BUG=911930

Change-Id: I034e3cc539e19f5cf7b9d35809f12a54a110ac8f
Reviewed-on: https://chromium-review.googlesource.com/c/1390323
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Leon Han <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#619010}
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/content/browser/service_worker/service_worker_blob_reader.h
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/content/browser/service_worker/service_worker_url_request_job.h
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/third_party/blink/public/mojom/fetch/fetch_api_request.mojom
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/third_party/blink/public/platform/modules/service_worker/web_service_worker_request.h
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/third_party/blink/renderer/core/fetch/fetch_request_data.cc
[modify] https://crrev.com/28e779303ba5c598a75dc00352b26d1b82056d14/third_party/blink/renderer/platform/exported/web_service_worker_request.cc

Sign in to add a comment