Create ServiceWorkerProviderHost before starting worker

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 537f97cd56a533ca0eaece7cf094c457f2854e67..e987eb6c380a3ba860b20a261e2e4b41f23147d2 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -245,7 +245,6 @@ ServiceWorkerDispatcherHost::GetOrCreateRegistrationHandle(
     existing_handle->IncrementRefCount();
     return existing_handle;
   }
-
   std::unique_ptr<ServiceWorkerRegistrationHandle> new_handle(
       new ServiceWorkerRegistrationHandle(GetContext()->AsWeakPtr(),
                                           provider_host, registration));
@@ -970,6 +969,9 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
                                                  std::move(info), this);
     GetContext()->AddProviderHost(std::move(provider_host));
   } else {
+    // Provider host for controller should be pre-created on StartWorker in
+    // ServiceWorkerVersion.
+    DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);

[dcheng, 2017/06/20 21:09:47]

We shouldn't be DCHECKing things in IPC handlers--same comment for line 967,
especially on the browser side. It's hard for me to tell if this is OK, but from
what I can tell, it's possible for a bad renderer to violate this assumption,
right?

[shimazu, 2017/06/21 03:11:26]

Sorry, I probably don't understand what is the point of your concern. 
Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's the
"bad renderer"? Basically this DCHECK won't be failed unless we don't put any
bugs in, so failing DCHECK indicates something is broken.
Also, I don't understand the difference between DCHECKing in IPC handlers and in
methods used by IPC handlers. Should both be restricted? or Should DCHECKing in
IPC handlers directly be avoided?

[falken, 2017/06/21 03:26:20]

In Chromium the renderer process is considered untrusted. It's possible the
render process was hacked and sends bad IPC messages.

See
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

Actually a long time ago, this said "DCHECK for IPC messages is fine", which I
always found strange, but it looks updated since then.

[shimazu, 2017/06/21 05:20:13]

That makes sense!
So we should use IF statements and bad_message::ReceivedBadMessage for checking
inputs of IPCs to ensure the inputs are valid because DCKECK won't work in
release builds, right?

Updated, thanks!

[dcheng, 2017/06/21 05:45:05]

Basically, it's two things:
- We want to make sure we handle invalid inputs from IPC.
- We also don't want to crash on invalid input from an IPC (since this would
kill Chrome).

It is occasionally OK to DCHECK in IPC message handlers, if we're asserting
about state that's not related to the IPC at all--but in general, DCHECK/CHECK
in IPC message handlers will be viewed with some suspicion =)

     if (ServiceWorkerUtils::IsBrowserAssignedProviderId(info.provider_id)) {
       bad_message::ReceivedBadMessage(
           this, bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
@@ -980,88 +982,6 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
   }
 }
 
-void ServiceWorkerDispatcherHost::OnSetHostedVersionId(int provider_id,
-                                                       int64_t version_id,
-                                                       int embedded_worker_id) {
-  TRACE_EVENT0("ServiceWorker",
-               "ServiceWorkerDispatcherHost::OnSetHostedVersionId");
-  if (!GetContext())
-    return;
-  ServiceWorkerProviderHost* provider_host =
-      GetContext()->GetProviderHost(render_process_id_, provider_id);
-  if (!provider_host) {
-    bad_message::ReceivedBadMessage(
-        this, bad_message::SWDH_SET_HOSTED_VERSION_NO_HOST);
-    return;
-  }
-
-  // This provider host must be specialized for a controller.
-  if (provider_host->IsProviderForClient()) {
-    bad_message::ReceivedBadMessage(
-        this, bad_message::SWDH_SET_HOSTED_VERSION_INVALID_HOST);
-    return;
-  }
-
-  // A service worker context associated with this provider host was destroyed
-  // due to restarting the service worker system etc.
-  if (!provider_host->IsContextAlive())
-    return;
-
-  // We might not be STARTING if the stop sequence was entered (STOPPING) or
-  // ended up being detached (STOPPED).
-  ServiceWorkerVersion* version = GetContext()->GetLiveVersion(version_id);
-  if (!version || version->running_status() != EmbeddedWorkerStatus::STARTING)
-    return;
-
-  // If the version has a different embedded worker, assume the message is about
-  // a detached worker and ignore.
-  if (version->embedded_worker()->embedded_worker_id() != embedded_worker_id)
-    return;
-
-  // A process for the worker must be equal to a process for the provider host.
-  if (version->embedded_worker()->process_id() != provider_host->process_id()) {
-    // Temporary debugging for https://crbug.com/668633
-    base::debug::ScopedCrashKey scope_worker_pid(
-        "swdh_set_hosted_version_worker_pid",
-        base::IntToString(version->embedded_worker()->process_id()));
-    base::debug::ScopedCrashKey scope_provider_host_pid(
-        "swdh_set_hosted_version_host_pid",
-        base::IntToString(provider_host->process_id()));
-    if (version->embedded_worker()->process_id() !=
-        ChildProcessHost::kInvalidUniqueID) {
-      base::debug::ScopedCrashKey scope_is_new_process(
-          "swdh_set_hosted_version_is_new_process",
-          version->embedded_worker()->is_new_process() ? "true" : "false");
-    }
-    base::debug::ScopedCrashKey scope_worker_restart_count(
-        "swdh_set_hosted_version_restart_count",
-        base::IntToString(version->embedded_worker()->restart_count()));
-    bad_message::ReceivedBadMessage(
-        this, bad_message::SWDH_SET_HOSTED_VERSION_PROCESS_MISMATCH);
-    return;
-  }
-
-  provider_host->SetHostedVersion(version);
-
-  // Retrieve the registration associated with |version|. The registration
-  // must be alive because the version keeps it during starting worker.
-  ServiceWorkerRegistration* registration =
-      GetContext()->GetLiveRegistration(version->registration_id());
-  DCHECK(registration);
-
-  // Set the document URL to the script url in order to allow
-  // register/unregister/getRegistration on ServiceWorkerGlobalScope.
-  provider_host->SetDocumentUrl(version->script_url());
-
-  ServiceWorkerRegistrationObjectInfo info;
-  ServiceWorkerVersionAttributes attrs;
-  GetRegistrationObjectInfoAndVersionAttributes(
-      provider_host->AsWeakPtr(), registration, &info, &attrs);
-
-  Send(new ServiceWorkerMsg_AssociateRegistration(kDocumentMainThreadId,
-                                                  provider_id, info, attrs));
-}
-
 template <typename SourceInfo>
 void ServiceWorkerDispatcherHost::DispatchExtendableMessageEventInternal(
     scoped_refptr<ServiceWorkerVersion> worker,
@@ -1178,17 +1098,17 @@ ServiceWorkerDispatcherHost::FindRegistrationHandle(int provider_id,
 void ServiceWorkerDispatcherHost::GetRegistrationObjectInfoAndVersionAttributes(
     base::WeakPtr<ServiceWorkerProviderHost> provider_host,
     ServiceWorkerRegistration* registration,
-    ServiceWorkerRegistrationObjectInfo* info,
-    ServiceWorkerVersionAttributes* attrs) {
+    ServiceWorkerRegistrationObjectInfo* out_info,
+    ServiceWorkerVersionAttributes* out_attrs) {
   ServiceWorkerRegistrationHandle* handle =
       GetOrCreateRegistrationHandle(provider_host, registration);
-  *info = handle->GetObjectInfo();
+  *out_info = handle->GetObjectInfo();
 
-  attrs->installing = provider_host->GetOrCreateServiceWorkerHandle(
+  out_attrs->installing = provider_host->GetOrCreateServiceWorkerHandle(
       registration->installing_version());
-  attrs->waiting = provider_host->GetOrCreateServiceWorkerHandle(
+  out_attrs->waiting = provider_host->GetOrCreateServiceWorkerHandle(
       registration->waiting_version());
-  attrs->active = provider_host->GetOrCreateServiceWorkerHandle(
+  out_attrs->active = provider_host->GetOrCreateServiceWorkerHandle(
       registration->active_version());
 }