PlzNavigate: detect when a ServiceWorker is present

PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7a7befa5297e4b50439e16a97dd541dfb438aaaf
Cr-Commit-Position: refs/heads/master@{#402791}

[clamy, 2016-06-08 16:59:06]

Description was changed from

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
==========

to

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-06-08 17:02:41]

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

[clamy, 2016-06-08 17:02:42]

@falken: PTAL at the changes in service_worker

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_impl_core.cc:57:
service_worker_context_wrapper->CheckHasServiceWorker(
@falken: Is it the right to call to check whether there's a ServiceWorker
registered for the url?

[falken, 2016-06-09 20:15:49]

I'm wondering about racy cases like SW is being installed or being unregistered
around the time this check occurs.

What would be the consequence of "sending to renderer to be handled by the SW",
but then by the time that happens, the SW is not yet activated or is
unregistered?

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_impl_core.cc:57:
service_worker_context_wrapper->CheckHasServiceWorker(
On 2016/06/08 17:02:42, clamy wrote:
> @falken: Is it the right to call to check whether there's a ServiceWorker
> registered for the url?

Hm, unfortunately CheckHasServiceWorker was designed for AppBanner's use case,
so has some behavior you might not want here:
- It waits for an installing worker to become installed.
- CheckHasServiceWorker calls back on the UI thread.
- |other_url| must be in-scope

[clamy, 2016-06-23 15:51:43]

Thanks! My idea is to do the following:
1) Check if there is a ServiceWorker with a fetch event for the url.
2) If yes, send the navigation to a renderer to create the ServiceWorker and
have it do the navigation.
3) If that fails, fall back to the browser and this time skip the ServiceWorker
check. The url is then loaded by the loader normally.
4) If redirects happen to a page with a ServiceWorker, do something similar.

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_impl_core.cc:57:
service_worker_context_wrapper->CheckHasServiceWorker(
On 2016/06/09 20:15:48, falken wrote:
> On 2016/06/08 17:02:42, clamy wrote:
> > @falken: Is it the right to call to check whether there's a ServiceWorker
> > registered for the url?
> 
> Hm, unfortunately CheckHasServiceWorker was designed for AppBanner's use case,
> so has some behavior you might not want here:
> - It waits for an installing worker to become installed.
> - CheckHasServiceWorker calls back on the UI thread.
> - |other_url| must be in-scope
> 
> 
> 

I've switched to FindReadyRegistartionForDocument. Is it better?

[falken, 2016-06-24 08:21:52]

service worker bits lgtm

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_impl_core.cc:57:
service_worker_context_wrapper->CheckHasServiceWorker(
On 2016/06/23 15:51:43, clamy wrote:
> On 2016/06/09 20:15:48, falken wrote:
> > On 2016/06/08 17:02:42, clamy wrote:
> > > @falken: Is it the right to call to check whether there's a ServiceWorker
> > > registered for the url?
> > 
> > Hm, unfortunately CheckHasServiceWorker was designed for AppBanner's use
case,
> > so has some behavior you might not want here:
> > - It waits for an installing worker to become installed.
> > - CheckHasServiceWorker calls back on the UI thread.
> > - |other_url| must be in-scope
> > 
> > 
> > 
> 
> I've switched to FindReadyRegistartionForDocument. Is it better?

Yes I think this is right. This will return a registration with an activated
worker.

https://codereview.chromium.org/2045383002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:159: loader_));
do you need to return; here?

https://codereview.chromium.org/2045383002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2045383002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:96: if
(provider_id_ == kInvalidServiceWorkerProviderId)
I think it should be rare/impossible for GetNextProviderId to return an invalid
provider id. I guess overflow is possible? Maybe DCHECK() or DVLOG(1) here?

[clamy, 2016-06-24 11:16:33]

Description was changed from

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-06-24 11:16:33]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2016-06-24 11:16:55]

Thanks!
@nasko: PTAL

https://codereview.chromium.org/2045383002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:159: loader_));
On 2016/06/24 08:21:51, falken wrote:
> do you need to return; here?

Yes, good catch.

https://codereview.chromium.org/2045383002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2045383002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:96: if
(provider_id_ == kInvalidServiceWorkerProviderId)
On 2016/06/24 08:21:51, falken wrote:
> I think it should be rare/impossible for GetNextProviderId to return an
invalid
> provider id. I guess overflow is possible? Maybe DCHECK() or DVLOG(1) here?

Done.

[nasko, 2016-06-27 16:54:26]

IPC LGTM

https://codereview.chromium.org/2045383002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:167: if
(ResourceDispatcherHostImpl::Get()) {
nit: if (!RDH::Get()) return;

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:788: this,
bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
Should this bad message reason be removed/nullified/overwritten?

[clamy, 2016-06-29 12:15:51]

Thanks!

https://codereview.chromium.org/2045383002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/2045383002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:167: if
(ResourceDispatcherHostImpl::Get()) {
On 2016/06/27 16:54:26, nasko wrote:
> nit: if (!RDH::Get()) return;

Done.

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:788: this,
bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
On 2016/06/27 16:54:26, nasko wrote:
> Should this bad message reason be removed/nullified/overwritten?

According to the bad_message file it can't be removed. I added a comment saying
it was obsolete.

https://codereview.chromium.org/2045383002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2045383002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1340:
contents()->GetMainFrame()->PrepareForCommit();
The new check this patch adds in NavigationRequest for the navigating RFH showed
that there was an issue with this test when PlzNavigate is enabled. Without
these calls to PrepareForCommit, the 2 RFH are not in a situation where they can
expect a DidCommitProvisionalLoad.

[clamy, 2016-06-29 12:16:02]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-06-29 12:16:02]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2045383002/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-06-29 12:16:17]

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, 2016-06-29 13:56:13]

Description was changed from

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-29 13:56:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 13:57:26]

Description was changed from

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: detect when a ServiceWorker is present

This CL is the first step in PlzNavigate support of ServiceWorker. Before
starting a navigation, it checks whether a ServiceWorker is registered for the
url. If yes, it bails out. Currently this cancel the navigation. The next step
is ot have the navigation sent to the renderer to be handled by the
ServiceWorker.

This CL also removes the code that was introduced as part as the previous
attempt to support ServiceWorker and is no longer needed with the current plan.
BUG=440463
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7a7befa5297e4b50439e16a97dd541dfb438aaaf
Cr-Commit-Position: refs/heads/master@{#402791}
==========

[commit-bot: I haz the power, 2016-06-29 13:57:27]

Patchset 4 (id:??) landed as
https://crrev.com/7a7befa5297e4b50439e16a97dd541dfb438aaaf
Cr-Commit-Position: refs/heads/master@{#402791}

[falken, 2016-06-30 00:50:11]

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:788: this,
bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
On 2016/06/29 12:15:51, clamy wrote:
> On 2016/06/27 16:54:26, nasko wrote:
> > Should this bad message reason be removed/nullified/overwritten?
> 
> According to the bad_message file it can't be removed. I added a comment
saying
> it was obsolete.

This is still used in line 761. I'll make a followup patch to remove the
comment.

[clamy, 2016-06-30 08:58:14]

On 2016/06/30 00:50:11, falken wrote:
>
https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
> File content/browser/service_worker/service_worker_dispatcher_host.cc (left):
> 
>
https://codereview.chromium.org/2045383002/diff/40001/content/browser/service...
> content/browser/service_worker/service_worker_dispatcher_host.cc:788: this,
> bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
> On 2016/06/29 12:15:51, clamy wrote:
> > On 2016/06/27 16:54:26, nasko wrote:
> > > Should this bad message reason be removed/nullified/overwritten?
> > 
> > According to the bad_message file it can't be removed. I added a comment
> saying
> > it was obsolete.
> 
> This is still used in line 761. I'll make a followup patch to remove the
> comment.

Sorry about that, thanks for fixing it!