PlzNavigate: ensure we create a ServiceWorkerProviderHost

PlzNavigate: ensure we create a ServiceWorkerProviderHost

Prior to this CL, it was possible for
ServiceWorkerDispatcherHost::OnProviderCreated to return without
ensuring that a ServiceWorkerProviderHost had been added to the
ServiceWorkerContextCore when Plznavigate was enabled. This CL ensures
that we always add a ServiceWorkerProviderHost to the context. Not doing
so could result in a renderer kill later if the page tries to register a
ServiceWorker.

BUG=703972
Review-Url: https://codereview.chromium.org/2774513003
Cr-Commit-Position: refs/heads/master@{#459239}
Committed: https://chromium.googlesource.com/chromium/src/+/991a617bca1c8353bafa7ae0296a9c64c09b023f

[clamy, 2017-03-23 16:40:44]

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

[clamy, 2017-03-23 16:41:15]

Description was changed from

==========
PlzNavigate: ensure we create a ServiceWorkerProviderHost

Prior to this CL, it was possible for
ServiceWorkerDispatcherHost::OnProviderCreated to return without
ensuring that a ServiceWorkerProviderHost had been added to the
ServiceWorkerContextCore when Plznavigate was enabled. This CL ensures
that we always add a ServiceWorkerProviderHost to the context. Not doing
so could result in a renderer kill later if the page tries to register a
ServiceWorker.

BUG=703972
==========

to

==========
PlzNavigate: ensure we create a ServiceWorkerProviderHost

Prior to this CL, it was possible for
ServiceWorkerDispatcherHost::OnProviderCreated to return without
ensuring that a ServiceWorkerProviderHost had been added to the
ServiceWorkerContextCore when Plznavigate was enabled. This CL ensures
that we always add a ServiceWorkerProviderHost to the context. Not doing
so could result in a renderer kill later if the page tries to register a
ServiceWorker.

BUG=703972
==========

[clamy, 2017-03-23 16:41:15]

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

[commit-bot: I haz the power, 2017-03-23 16:41:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-03-23 16:41:26]

@falken: PTAL

[clamy, 2017-03-23 16:51:12]

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

[clamy, 2017-03-23 16:51:26]

+nasko

[commit-bot: I haz the power, 2017-03-23 17:16:05]

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

[commit-bot: I haz the power, 2017-03-23 17:16:06]

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

[jam, 2017-03-23 17:37:08]

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

[jam, 2017-03-23 17:37:09]

Even though I'm not an expert in this code, this seems like the right fix since
the object isn't created any other way for the plznavigate path. So I'll lgtm
and cq this, and if falken has any comments we can do them in a followup in the
interest of getting this in today's canary.

[jam, 2017-03-23 17:37:15]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-03-23 17:37:50]

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-03-23 19:04:37]

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

[commit-bot: I haz the power, 2017-03-23 19:04:38]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[nasko, 2017-03-23 20:54:08]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:55:03]

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-03-23 21:53:37]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490302448276280, "parent_rev":
"45a027c824ca3ddb07203bda371e735db1e1ba67", "commit_rev":
"991a617bca1c8353bafa7ae0296a9c64c09b023f"}

[commit-bot: I haz the power, 2017-03-23 21:54:20]

Description was changed from

==========
PlzNavigate: ensure we create a ServiceWorkerProviderHost

Prior to this CL, it was possible for
ServiceWorkerDispatcherHost::OnProviderCreated to return without
ensuring that a ServiceWorkerProviderHost had been added to the
ServiceWorkerContextCore when Plznavigate was enabled. This CL ensures
that we always add a ServiceWorkerProviderHost to the context. Not doing
so could result in a renderer kill later if the page tries to register a
ServiceWorker.

BUG=703972
==========

to

==========
PlzNavigate: ensure we create a ServiceWorkerProviderHost

Prior to this CL, it was possible for
ServiceWorkerDispatcherHost::OnProviderCreated to return without
ensuring that a ServiceWorkerProviderHost had been added to the
ServiceWorkerContextCore when Plznavigate was enabled. This CL ensures
that we always add a ServiceWorkerProviderHost to the context. Not doing
so could result in a renderer kill later if the page tries to register a
ServiceWorker.

BUG=703972

Review-Url: https://codereview.chromium.org/2774513003
Cr-Commit-Position: refs/heads/master@{#459239}
Committed:
https://chromium.googlesource.com/chromium/src/+/991a617bca1c8353bafa7ae0296a...
==========

[commit-bot: I haz the power, 2017-03-23 21:54:21]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/991a617bca1c8353bafa7ae0296a...

[michaeln, 2017-03-23 22:25:07]

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

[michaeln, 2017-03-23 22:25:08]

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

https://codereview.chromium.org/2774513003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_dispatcher_host.cc:1020: if
(provider_host == nullptr) {
Do we know how this happens?

The only way to get an IsBrowserAssignedProviderId is thru 
ServiceWorkerRequestHandler::InitializeForNavigation() which calls thru to
PreCreateNavigationHost().

There's a great comment in service_worker_navigation_handle.h, do we know where
that control flows off course in this bug's case?

// This class is used to manage the lifetime of ServiceWorkerProviderHosts
// created during navigation. This is a UI thread class, with a pendant class
// on the IO thread, the ServiceWorkerNavigationHandleCore.
//
// The lifetime of the ServiceWorkerNavigationHandle, the
// ServiceWorkerNavigationHandleCore and the ServiceWorkerProviderHost are the
// following :
//   1) We create a ServiceWorkerNavigationHandle on the UI thread with a
//   service worker provider id of -1. This also leads to the creation of a
//   ServiceWorkerNavigationHandleCore with an id of -1.
//
//   2) When the navigation request is sent to the IO thread, we include a
//   pointer to the ServiceWorkerNavigationHandleCore.
//
//   3) If we pre-create a ServiceWorkerProviderHost for this navigation, its
//   ownershipped is passed to the ServiceWorkerNavigationHandleCore. The
//   ServiceWorkerNavigationHandleCore id is updated.
//
//   4) The ServiceWorkerNavigationHandleCore informs the
//   ServiceWorkerNavigationHandle on the UI that the service worker provider
//   id was updated.
//
//   5) When the navigation is ready to commit, the NavigationRequest will
//   update the RequestNavigationParams based on the id from the
//   ServiceWorkerNavigationHandle.
//
//   6) If the commit leads to the creation of a ServiceWorkerNetworkProvider
//   in the renderer, a ServiceWorkerHostMsg_ProviderCreated will be received
//   in the browser. The ServiceWorkerDispatcherHost will retrieve the
//   ServiceWorkerProviderHost from the ServiceWorkerNavigationHandleCore and
//   put it in the ServiceWorkerContextCore map of ServiceWorkerProviderHosts.
//
//   7) When the navigation finishes, the ServiceWorkerNavigationHandle is
//   destroyed. The destructor of the ServiceWorkerNavigationHandle posts a
//   task to destroy the ServiceWorkerNavigationHandleCore on the IO thread.
//   This in turn leads to the destruction of an unclaimed
//   ServiceWorkerProviderHost.

[clamy, 2017-04-05 12:23:48]

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

https://codereview.chromium.org/2774513003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_dispatcher_host.cc:1020: if
(provider_host == nullptr) {
On 2017/03/23 22:25:07, michaeln wrote:
> Do we know how this happens?
> 
> The only way to get an IsBrowserAssignedProviderId is thru 
> ServiceWorkerRequestHandler::InitializeForNavigation() which calls thru to
> PreCreateNavigationHost().
> 
> There's a great comment in service_worker_navigation_handle.h, do we know
where
> that control flows off course in this bug's case?
> 
> // This class is used to manage the lifetime of ServiceWorkerProviderHosts
> // created during navigation. This is a UI thread class, with a pendant class
> // on the IO thread, the ServiceWorkerNavigationHandleCore.
> //
> // The lifetime of the ServiceWorkerNavigationHandle, the
> // ServiceWorkerNavigationHandleCore and the ServiceWorkerProviderHost are the
> // following :
> //   1) We create a ServiceWorkerNavigationHandle on the UI thread with a
> //   service worker provider id of -1. This also leads to the creation of a
> //   ServiceWorkerNavigationHandleCore with an id of -1.
> //
> //   2) When the navigation request is sent to the IO thread, we include a
> //   pointer to the ServiceWorkerNavigationHandleCore.
> //
> //   3) If we pre-create a ServiceWorkerProviderHost for this navigation, its
> //   ownershipped is passed to the ServiceWorkerNavigationHandleCore. The
> //   ServiceWorkerNavigationHandleCore id is updated.
> //
> //   4) The ServiceWorkerNavigationHandleCore informs the
> //   ServiceWorkerNavigationHandle on the UI that the service worker provider
> //   id was updated.
> //
> //   5) When the navigation is ready to commit, the NavigationRequest will
> //   update the RequestNavigationParams based on the id from the
> //   ServiceWorkerNavigationHandle.
> //
> //   6) If the commit leads to the creation of a ServiceWorkerNetworkProvider
> //   in the renderer, a ServiceWorkerHostMsg_ProviderCreated will be received
> //   in the browser. The ServiceWorkerDispatcherHost will retrieve the
> //   ServiceWorkerProviderHost from the ServiceWorkerNavigationHandleCore and
> //   put it in the ServiceWorkerContextCore map of ServiceWorkerProviderHosts.
> //
> //   7) When the navigation finishes, the ServiceWorkerNavigationHandle is
> //   destroyed. The destructor of the ServiceWorkerNavigationHandle posts a
> //   task to destroy the ServiceWorkerNavigationHandleCore on the IO thread.
> //   This in turn leads to the destruction of an unclaimed
> //   ServiceWorkerProviderHost.

My personal hunch (I haven't been able to reproduce that in practice), is that
this is due to some issues we have where the NavigationHandle gets deleted when
it shouldn't. Until the renderer claims the ServiceWorkerProviderHost, it is
actually owned by the NavigationHandle through NavigationHandle's ownership of
of the ServiceWorkerNavigationHandle. So if the NavigationHandle goes away
before the pre-created SWPH is claimed, the SWPH will be deleted. We have a few
race conditions where the NavigationHandle is wrongly deleted before commit.
They could result in the SWPH being deleted before we get OnproviderCreated IPC
from the renderer.