Create ServiceWorkerProviderHost before starting worker

content/browser/service_worker/service_worker_provider_host.cc

Index: content/browser/service_worker/service_worker_provider_host.cc
diff --git a/content/browser/service_worker/service_worker_provider_host.cc b/content/browser/service_worker/service_worker_provider_host.cc
index 34be8dd0c8f7916e44abfbc074dc82f235056dd3..0d3ad09f1e0287f633d67779f6f4969fbc216752 100644
--- a/content/browser/service_worker/service_worker_provider_host.cc
+++ b/content/browser/service_worker/service_worker_provider_host.cc
@@ -36,10 +36,13 @@ namespace content {
 
 namespace {
 
-// PlzNavigate
-// Next ServiceWorkerProviderHost ID for navigations, starts at -2 and keeps
-// going down.
-int g_next_navigation_provider_id = -2;
+// Provider host for navigation with PlzNavigate or service worker's context is
+// created on the browser side. This function provides the next
+// ServiceWorkerProviderHost ID for them, starts at -2 and keeps going down.
+int NextBrowserProvidedProviderId() {
+  static int g_next_browser_provided_provider_id = -2;
+  return g_next_browser_provided_provider_id--;
+}
 
 // A request handler derivative used to handle navigation requests when
 // skip_service_worker flag is set. It tracks the document URL and sets the url
@@ -59,10 +62,9 @@ class ServiceWorkerURLTrackingRequestHandler
   ~ServiceWorkerURLTrackingRequestHandler() override {}
 
   // Called via custom URLRequestJobFactory.
-  net::URLRequestJob* MaybeCreateJob(
-      net::URLRequest* request,
-      net::NetworkDelegate* /* network_delegate */,
-      ResourceContext* /* resource_context */) override {
+  net::URLRequestJob* MaybeCreateJob(net::URLRequest* request,
+                                     net::NetworkDelegate*,
+                                     ResourceContext*) override {
     // |provider_host_| may have been deleted when the request is resumed.
     if (!provider_host_)
       return nullptr;
@@ -114,18 +116,32 @@ ServiceWorkerProviderHost::PreCreateNavigationHost(
     bool are_ancestors_secure,
     const WebContentsGetter& web_contents_getter) {
   CHECK(IsBrowserSideNavigationEnabled());
-  // Generate a new browser-assigned id for the host.
-  int provider_id = g_next_navigation_provider_id--;
   auto host = base::WrapUnique(new ServiceWorkerProviderHost(
       ChildProcessHost::kInvalidUniqueID,
-      ServiceWorkerProviderHostInfo(provider_id, MSG_ROUTING_NONE,
-                                    SERVICE_WORKER_PROVIDER_FOR_WINDOW,
-                                    are_ancestors_secure),
+      ServiceWorkerProviderHostInfo(
+          NextBrowserProvidedProviderId(), MSG_ROUTING_NONE,
+          SERVICE_WORKER_PROVIDER_FOR_WINDOW, are_ancestors_secure),
       context, nullptr));
   host->web_contents_getter_ = web_contents_getter;
   return host;
 }
 
+// static
+std::unique_ptr<ServiceWorkerProviderHost>
+ServiceWorkerProviderHost::PreCreateForWorkerContext(
+    ServiceWorkerVersion* version,
+    base::WeakPtr<ServiceWorkerContextCore> context) {
+  auto host = base::WrapUnique(new ServiceWorkerProviderHost(
+      ChildProcessHost::kInvalidUniqueID,
+      ServiceWorkerProviderHostInfo(NextBrowserProvidedProviderId(),
+                                    MSG_ROUTING_NONE,
+                                    SERVICE_WORKER_PROVIDER_FOR_CONTROLLER,
+                                    true /* is_parent_frame_secure */),
+      context, nullptr));
+  host->SetHostedVersion(version);
+  return host;
+}
+
 // static
 std::unique_ptr<ServiceWorkerProviderHost> ServiceWorkerProviderHost::Create(
     int process_id,
@@ -177,20 +193,27 @@ ServiceWorkerProviderHost::ServiceWorkerProviderHost(
       binding_(this) {
   DCHECK_NE(SERVICE_WORKER_PROVIDER_UNKNOWN, info_.type);
 
-  // PlzNavigate
-  CHECK(render_process_id != ChildProcessHost::kInvalidUniqueID ||
-        IsBrowserSideNavigationEnabled());
 
   if (info_.type == SERVICE_WORKER_PROVIDER_FOR_CONTROLLER) {
-    // Actual thread id is set when the service worker context gets started.
+    // Actual |render_process_id| will be set after choosing a process for the
+    // controller, and |render_thread id| will be set when the service worker
+    // context gets started.
+    CHECK_EQ(ChildProcessHost::kInvalidUniqueID, render_process_id);

[falken, 2017/06/14 08:10:34]

Is there a reason this is a CHECK isntead of a DCHECK? It doesn't look like a
security issue if this is violated.

[shimazu, 2017/06/16 04:18:43]

Done.

     render_thread_id_ = kInvalidEmbeddedWorkerThreadId;
+  } else {
+    // PlzNavigate
+    CHECK(render_process_id != ChildProcessHost::kInvalidUniqueID ||
+          IsBrowserSideNavigationEnabled());
   }
+
   context_->RegisterProviderHostByClientID(client_uuid_, this);
 
-  // PlzNavigate
-  // |provider_| and |binding_| will be bound on CompleteNavigationInitialized.
-  if (IsBrowserSideNavigationEnabled()) {
-    DCHECK(!info.client_ptr_info.is_valid() && !info.host_request.is_pending());
+  // |client_| and |binding_| will be bound on CompleteNavigationInitialized
+  // (PlzNavigate) or on CompleteStartWorkerPreparation (providers for
+  // controller).
+  if (!info_.client_ptr_info.is_valid() && !info_.host_request.is_pending()) {
+    DCHECK(IsBrowserSideNavigationEnabled() ||
+           info_.type == SERVICE_WORKER_PROVIDER_FOR_CONTROLLER);
     return;
   }
 
@@ -327,8 +350,7 @@ void ServiceWorkerProviderHost::SetControllerVersionAttribute(
 void ServiceWorkerProviderHost::SetHostedVersion(
     ServiceWorkerVersion* version) {
   DCHECK(!IsProviderForClient());
-  DCHECK_EQ(EmbeddedWorkerStatus::STARTING, version->running_status());
-  DCHECK_EQ(render_process_id_, version->embedded_worker()->process_id());
+  DCHECK_EQ(EmbeddedWorkerStatus::STOPPED, version->running_status());

[falken, 2017/06/14 08:10:35]

does DCHECK(!running_hosted_version_) also hold?

[shimazu, 2017/06/16 04:18:43]

Sounds good!

   running_hosted_version_ = version;
 }
 
@@ -659,6 +681,50 @@ void ServiceWorkerProviderHost::CompleteNavigationInitialized(
   NotifyControllerToAssociatedProvider();
 }
 
+void ServiceWorkerProviderHost::CompleteStartWorkerPreparation(
+    int process_id,
+    mojom::ServiceWorkerProviderClientInfoPtr* provider_client_info) {
+  DCHECK(context_);
+
+  DCHECK_EQ(kInvalidEmbeddedWorkerThreadId, render_thread_id_);
+  DCHECK_EQ(ChildProcessHost::kInvalidUniqueID, render_process_id_);
+  DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, provider_type());
+  DCHECK(running_hosted_version_);
+
+  DCHECK_NE(ChildProcessHost::kInvalidUniqueID, process_id);
+
+  ServiceWorkerDispatcherHost* dispatcher_host =
+      context_->GetDispatcherHost(process_id);
+  DCHECK(dispatcher_host);

[falken, 2017/06/14 08:10:35]

Is it possible the process crashed by the time we got here?

[shimazu, 2017/06/16 04:18:43]

Yes, I think so. 
Between EmbeddedWorkerInstance::StartTask::OnAllocatedProcess and here, there
are two (IO -> UI -> IO) thread hops. The renderer is possible to crash during
that.

[falken, 2017/06/16 15:44:46]

If the process crashed, I think dispatcher_host would be nullptr so this DCHECK
would fail.

[kinuko, 2017/06/19 03:20:07]

I have a question, if it's possible that process crash could be propagated to
the IO thread before here could it already have called CallDetach on client_
error and reset client_?  Is it still safe to call client_ method in
SendStartWorker?

[shimazu, 2017/06/19 03:53:30]

Ah, sorry, I didn't understand what falken pointed out.
If the process crashed before reaching here, CallDetach is called and StartTask
is stopped at the moment. That means SendStartWorker which invokes this method
isn't called. Otherwise, dispatcher_host should be found here.

+  render_process_id_ = process_id;
+  dispatcher_host_ = dispatcher_host;
+
+  // Retrieve the registration associated with |version|. The registration
+  // must be alive because the version keeps it during starting worker.
+  ServiceWorkerRegistration* registration = context_->GetLiveRegistration(
+      running_hosted_version()->registration_id());
+  DCHECK(registration);
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  dispatcher_host->GetRegistrationObjectInfoAndVersionAttributes(
+      AsWeakPtr(), registration, &info, &attrs);
+
+  // Initialize provider_client_info.
+  (*provider_client_info)->provider_id = provider_id();
+  (*provider_client_info)->type = provider_type();
+  (*provider_client_info)->attributes = std::move(attrs);
+  (*provider_client_info)->registration = std::move(info);
+  (*provider_client_info)->is_parent_frame_secure = is_parent_frame_secure();
+  (*provider_client_info)->client_request = mojo::MakeRequest(&provider_);
+  binding_.Bind(mojo::MakeRequest(&(*provider_client_info)->host_ptr_info));
+  binding_.set_connection_error_handler(
+      base::Bind(&RemoveProviderHost, context_, process_id, provider_id()));

[falken, 2017/06/14 08:10:34]

If the process already crashed, does the error handler get caleld?

[shimazu, 2017/06/16 04:18:43]

Yes, I think so. It'll be triggered when calling StartWorker() on the Mojo
interface. 

+
+  // Set the document URL to the script url in order to allow
+  // register/unregister/getRegistration on ServiceWorkerGlobalScope.
+  SetDocumentUrl(running_hosted_version()->script_url());
+}
+
 void ServiceWorkerProviderHost::SendUpdateFoundMessage(
     int registration_handle_id) {
   if (!dispatcher_host_)