PlzNavigate: ensure we create a ServiceWorkerProviderHost

content/browser/service_worker/service_worker_dispatcher_host.cc

Index: content/browser/service_worker/service_worker_dispatcher_host.cc
diff --git a/content/browser/service_worker/service_worker_dispatcher_host.cc b/content/browser/service_worker/service_worker_dispatcher_host.cc
index 1fb1130b493ef61f803b9a9205f9924391c1a082..d8a667c555454f71233e43c56697639dbaf8ceb1 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -1016,10 +1016,15 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
     if (navigation_handle_core != nullptr)
       provider_host = navigation_handle_core->RetrievePreCreatedHost();
 
-    // If no host is found, the navigation has been cancelled in the meantime.
-    // Just return as the navigation will be stopped in the renderer as well.
-    if (provider_host == nullptr)
+    // If no host is found, create one.
+    if (provider_host == nullptr) {

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

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]

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.

+      GetContext()->AddProviderHost(
+          ServiceWorkerProviderHost::Create(render_process_id_, std::move(info),
+                                            GetContext()->AsWeakPtr(), this));
       return;
+    }
+
+    // Otherwise, completed the initialization of the pre-created host.
     DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_WINDOW, info.type);
     provider_host->CompleteNavigationInitialized(render_process_id_,
                                                  info.route_id, this);