[network service][SW] Rework browser-side SW on top of URL loader chaining

[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service https://googlechrome.github.io/samples/service-worker/basic/index.html and confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640, 724322
Review-Url: https://codereview.chromium.org/2906163002
Cr-Commit-Position: refs/heads/master@{#477355}
Committed: https://chromium.googlesource.com/chromium/src/+/1afcf3a6e926552729262f213769a6cbde2a0839

[kinuko, 2017-05-29 14:30:48]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-05-29 14:30:48]

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:319: factory_ =
base::MakeUnique<Factory>(delegate_, blob_storage_context_);
I think we can delay creating the factory and URLLoader even later until we got
the response from fetch event handler.

[shimazu, 2017-05-30 08:31:59]

shimazu@chromium.org changed reviewers:
+ shimazu@chromium.org

[shimazu, 2017-05-30 08:32:00]

drive-by comments:

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:132:
std::move(body_as_stream->stream));
If loading the body gets aborted, how the error will be caught?
It'll be notified via body_as_stream->callback_request.

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:319: factory_ =
base::MakeUnique<Factory>(delegate_, blob_storage_context_);
On 2017/05/29 14:30:48, kinuko wrote:
> I think we can delay creating the factory and URLLoader even later until we
got
> the response from fetch event handler.

+1 for deferring because currently the network request will be back to the
network when no response is provided via respondWith. It could be implemented to
defer creating the factory.

[scottmg, 2017-05-30 18:07:48]

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:319: factory_ =
base::MakeUnique<Factory>(delegate_, blob_storage_context_);
On 2017/05/29 14:30:48, kinuko wrote:
> I think we can delay creating the factory and URLLoader even later until we
got
> the response from fetch event handler.

Sorry, maybe I'm misunderstanding, but I think we need the ResourceRequest
object that's provided during CreateLoaderAndStart() in order to be able to
create the ServiceWorkerFetchDispatcher?

[shimazu, 2017-05-31 01:48:00]

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:110:
ServiceWorkerFetchEventResult fetch_result,
If the |fetch_result| is SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK, I think the
request should fall down to the network service because that means no response
was provided from the fetch event handler somehow. 
I guessed we should know whether the fallback happens or not before returning
URLLoaderFactory to URLLoaderRequestController, but it doesn't seems possible
because we need the ResourceRequest object for dispatching the fetch event.
Instead, do we need other way to fall back to the default behavior or restart
the selection process at URLLoaderRequestController again?

// Sorry if I'm missing some context or it's already been discussed.

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:319: factory_ =
base::MakeUnique<Factory>(delegate_, blob_storage_context_);
On 2017/05/30 18:07:48, scottmg wrote:
> On 2017/05/29 14:30:48, kinuko wrote:
> > I think we can delay creating the factory and URLLoader even later until we
> got
> > the response from fetch event handler.
> 
> Sorry, maybe I'm misunderstanding, but I think we need the ResourceRequest
> object that's provided during CreateLoaderAndStart() in order to be able to
> create the ServiceWorkerFetchDispatcher?

Ah, I missed that.. sorry!
We need the ResourceRequest object to dispatch the fetch event as you said.
Let me add another question at ::URLLoaderImpl::DidDispatchFetchEvent instead.

[kinuko, 2017-05-31 02:03:43]

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_job_wrapper.cc:319: factory_ =
base::MakeUnique<Factory>(delegate_, blob_storage_context_);
On 2017/05/31 01:48:00, shimazu wrote:
> On 2017/05/30 18:07:48, scottmg wrote:
> > On 2017/05/29 14:30:48, kinuko wrote:
> > > I think we can delay creating the factory and URLLoader even later until
we
> > got
> > > the response from fetch event handler.
> > 
> > Sorry, maybe I'm misunderstanding, but I think we need the ResourceRequest
> > object that's provided during CreateLoaderAndStart() in order to be able to
> > create the ServiceWorkerFetchDispatcher?
> 
> Ah, I missed that.. sorry!
> We need the ResourceRequest object to dispatch the fetch event as you said.
> Let me add another question at ::URLLoaderImpl::DidDispatchFetchEvent instead.

We can pass the info when we create a job.  Either works, but it's not really
necessary to make SW fetching procedure match with the lifetime of one
URLLoader.  I feel having smaller URLLoader implementations for individual cases
would be probably easier.

[scottmg, 2017-06-02 21:13:04]

Description was changed from

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

BUG=724322
==========

to

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html
BUG=724322
==========

[scottmg, 2017-06-02 21:13:59]

Description was changed from

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html
BUG=724322
==========

to

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=724322
==========

[scottmg, 2017-06-02 21:14:12]

Patchset #2 (id:20001) has been deleted

[scottmg, 2017-06-02 21:15:33]

Patchset #2 (id:40001) has been deleted

[scottmg, 2017-06-02 21:16:18]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 21:16:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-06-02 21:18:44]

Thank you both for reviewing, sorry for the slow iteration time.

I've deferred the factory and loader until later so that the
SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK case can be handled properly by
calling back to NavigationURLLoaderNetworkService to allow it to choose another
handler.

It's a bit rough still as I'm not sure where to get data for some pieces, but
it's enough to handle simple cases, so maybe we can start here.

[commit-bot: I haz the power, 2017-06-02 21:20:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 21:20:13]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scottmg, 2017-06-02 22:14:28]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 22:14:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 23:36:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 23:36:09]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[scottmg, 2017-06-05 22:44:38]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 22:45:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-06-05 22:45:53]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 22:46:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-06-05 23:36:44]

Description was changed from

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=724322
==========

to

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640
==========

[scottmg, 2017-06-05 23:36:57]

Description was changed from

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640
==========

to

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640,724322
==========

[commit-bot: I haz the power, 2017-06-06 00:48:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 00:48:34]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-06-06 01:33:30]

I think it's lgtm for starting from here with a comment.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:125: new
URLLoaderImpl(response_, std::move(body_as_stream_),
If the lifecycle of URLLoaderImpl is the same with the Mojo's pipe for
URLLoader, URLLoaderImpl should use StrongAssociatedBinding here instead of
having the mojo::AssociatedBinding in URLLoaderImpl.

==
mojo::MakeStrongAssociatedBinding(
    base::MakeUnique<URLLoaderImpl>(
        response_, std::move(body_as_stream_), blob_storage_context_,
        std::move(client)),
    std::move(request));

[kinuko, 2017-06-06 12:52:38]

Yeah thanks for working on this, I think this is looking great to hack further. 
lgtm

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:261:
active_worker->SetMainScriptHttpResponseInfo(http_info);
Do we need this?  (Esp. with the change in
ServiceWorkerVersion::OnScriptLoaded?)

[falken, 2017-06-06 13:20:22]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2017-06-06 13:20:23]

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:261:
active_worker->SetMainScriptHttpResponseInfo(http_info);
On 2017/06/06 12:52:37, kinuko wrote:
> Do we need this?  (Esp. with the change in
> ServiceWorkerVersion::OnScriptLoaded?)

Good point. I think quite a few things might break if the main script http
response info is not set: see crbug.com/485900. To be "safe" we should set the
info where we'd expect it to be set, which is whenever the service worker starts
up, instead of just setting it here before starting the SW for fetch
interception.

So I think instead we should move this to where the DCHECK is in OnScriptLoaded.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:296:
Referrer(GURL(request.referrer), blink::kWebReferrerPolicyDefault);
Why is line 293 a CHECK instead of DCHECK and line 296 uses a different referrer
policy? Could we add comments to clarify?

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:909: //
--network-service because subresources haven't been implemented yet.
In the non-servicifiied path,  ServiceWorkerReadFromCacheJob would set the http
response info. So, if this DCHECK is now failing, it's not really that
subresources are not implemented, it's that the service worker script reading is
not implemented.

In the servicified path, I'm a bit surprised that the service worker is able to
start at all if ServiceWorkerReadFromCacheJob is not being used. I guess it's
because we are fetching the service worker from network each time.

[scottmg, 2017-06-06 16:50:09]

Thanks for your patience.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_url_job_wrapper.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:125: new
URLLoaderImpl(response_, std::move(body_as_stream_),
On 2017/06/06 01:33:30, shimazu wrote:
> If the lifecycle of URLLoaderImpl is the same with the Mojo's pipe for
> URLLoader, URLLoaderImpl should use StrongAssociatedBinding here instead of
> having the mojo::AssociatedBinding in URLLoaderImpl.
> 
> ==
> mojo::MakeStrongAssociatedBinding(
>     base::MakeUnique<URLLoaderImpl>(
>         response_, std::move(body_as_stream_), blob_storage_context_,
>         std::move(client)),
>     std::move(request));

Done.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:261:
active_worker->SetMainScriptHttpResponseInfo(http_info);
On 2017/06/06 13:20:22, falken wrote:
> On 2017/06/06 12:52:37, kinuko wrote:
> > Do we need this?  (Esp. with the change in
> > ServiceWorkerVersion::OnScriptLoaded?)
> 
> Good point. I think quite a few things might break if the main script http
> response info is not set: see crbug.com/485900. To be "safe" we should set the
> info where we'd expect it to be set, which is whenever the service worker
starts
> up, instead of just setting it here before starting the SW for fetch
> interception.
> 
> So I think instead we should move this to where the DCHECK is in
OnScriptLoaded.
> 
> 

OK, removed for now.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_url_job_wrapper.cc:296:
Referrer(GURL(request.referrer), blink::kWebReferrerPolicyDefault);
On 2017/06/06 13:20:22, falken wrote:
> Why is line 293 a CHECK instead of DCHECK and line 296 uses a different
referrer
> policy? Could we add comments to clarify?

Oops, yeah. I copied the fallback case from
ServiceWorkerURLRequestJob::CreateFetchRequest() but that's the wrong path.
Updated to use the data from |request|.

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2906163002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:909: //
--network-service because subresources haven't been implemented yet.
On 2017/06/06 13:20:23, falken wrote:
> In the non-servicifiied path,  ServiceWorkerReadFromCacheJob would set the
http
> response info. So, if this DCHECK is now failing, it's not really that
> subresources are not implemented, it's that the service worker script reading
is
> not implemented.
> 
> In the servicified path, I'm a bit surprised that the service worker is able
to
> start at all if ServiceWorkerReadFromCacheJob is not being used. I guess it's
> because we are fetching the service worker from network each time.

I see. OK, I updated the comment. I think I got confused between the fetch of
the contents of index.html and the retrieval of the service worker js itself.

I'll continue with this as-is to avoid making this CL even messier, and then we
can look at whether to reuse ServiceWorkerReadFromCacheJob or implement
something similar.

[scottmg, 2017-06-06 16:50:18]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 16:50:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-06-06 16:53:16]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2017-06-06 18:20:48]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2017-06-06 18:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
shimazu@chromium.org
Link to the patchset: https://codereview.chromium.org/2906163002/#ps120001
(title: "fixes")

[commit-bot: I haz the power, 2017-06-06 18:21:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 18:53:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496773248738430,
"parent_rev": "b1a54322808398b47ccc09a288c963fce4d421b5", "commit_rev":
"1afcf3a6e926552729262f213769a6cbde2a0839"}

[commit-bot: I haz the power, 2017-06-06 18:53:19]

Description was changed from

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640,724322
==========

to

==========
[network service][SW] Rework browser-side SW on top of URL loader chaining

TEST=out\debug\content_shell --enable-network-service
https://googlechrome.github.io/samples/service-worker/basic/index.html and
confirm in DevTools that index.html is loaded via Service Worker.
BUG=715640,724322

Review-Url: https://codereview.chromium.org/2906163002
Cr-Commit-Position: refs/heads/master@{#477355}
Committed:
https://chromium.googlesource.com/chromium/src/+/1afcf3a6e926552729262f213769...
==========

[commit-bot: I haz the power, 2017-06-06 18:53:23]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1afcf3a6e926552729262f213769...