PlzNavigate: Make ServiceWorker work with PlzNavigate.

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 a27a2c1bbd5b06aba31b540bdfe14306174fcc3f..28e6a0dd6eb3d8240f6b6aa99b8e67c6fc099c44 100644
--- a/content/browser/service_worker/service_worker_provider_host.cc
+++ b/content/browser/service_worker/service_worker_provider_host.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_provider_host.h"
 
+#include "base/command_line.h"
 #include "base/guid.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
@@ -27,6 +28,7 @@
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
+#include "content/public/common/content_switches.h"
 
 namespace content {
 
@@ -87,6 +89,12 @@ ServiceWorkerProviderHost::ServiceWorkerProviderHost(
   DCHECK_NE(ChildProcessHost::kInvalidUniqueID, render_process_id_);
   DCHECK_NE(SERVICE_WORKER_PROVIDER_UNKNOWN, provider_type_);
   DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME, provider_type_);
+
+  // PlzNavigate
+  CHECK_IMPLIES(render_process_id == kVirtualProcessIDForBrowserRequest,
+                base::CommandLine::ForCurrentProcess()->HasSwitch(
+                    switches::kEnableBrowserSideNavigation));
+
   if (provider_type_ == SERVICE_WORKER_PROVIDER_FOR_CONTROLLER) {
     // Actual thread id is set when the service worker context gets started.
     render_thread_id_ = kInvalidEmbeddedWorkerThreadId;
@@ -516,29 +524,28 @@ void ServiceWorkerProviderHost::CompleteCrossSiteTransfer(
   DCHECK_NE(ChildProcessHost::kInvalidUniqueID, new_process_id);
   DCHECK_NE(MSG_ROUTING_NONE, new_frame_id);
 
-  render_process_id_ = new_process_id;
-  route_id_ = new_frame_id;
   render_thread_id_ = kDocumentMainThreadId;
   provider_id_ = new_provider_id;
   provider_type_ = new_provider_type;
-  dispatcher_host_ = new_dispatcher_host;
 
-  for (const GURL& pattern : associated_patterns_)
-    IncreaseProcessReference(pattern);
+  FinalizeInitialization(new_process_id, new_frame_id, new_dispatcher_host);

[michaeln, 2015/10/07 01:16:44]

thnx for factoring out the common code

[Fabrice (no longer in Chrome), 2015/10/07 12:59:43]

Welcome!

+}
 
-  for (auto& key_registration : matching_registrations_)
-    IncreaseProcessReference(key_registration.second->pattern());
+// PlzNavigate
+void ServiceWorkerProviderHost::CompleteNavigationInitialized(
+    int process_id,
+    int frame_routing_id,
+    ServiceWorkerDispatcherHost* dispatcher_host) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));

[michaeln, 2015/10/07 01:16:44]

I think this idea with the CHECK is to defend against bad ipcs, if that's right,
please move to the OnProviderCreated method and convert it to a
BadMessageReceived() call. Otherwise having a CHECK here provides a vector for a
compromised renderer to crash the entire browser. I don't think having vectors
like that is a desireable so I don't understand the motivation for using CHECKs
in this fashion?

[Fabrice (no longer in Chrome), 2015/10/07 12:59:43]

BadMessageReceived is already in the caller function. If we manage to still get
in this method, this means that somehow, the browser created a corresponding
SWPH and has been compromised. Or worse, there is a stack corruption.
This is really more of a sanity check. But it also ensures that no one will
attempt to reuse that method for current navigation architecture in the future.
Or would at least think twice about calling it.

[michaeln, 2015/10/08 02:18:51]

1. To defend against memory corruption or a compromised browser
2. For documentation purposes.

Thnx for the explanation. In that case, I think you should switch these to
DCHECKs, at least for those in the /service_worker directory. The DCHECK covers
the latter case. We don't use CHECKs as sanity checks because doing so
pervasively would add code bloat without adding real value. Search the
content/browser codebase and you'll see there aren't very many of them,
espcecially if you look only in the service_worker subdirectory.

+  DCHECK_EQ(kVirtualProcessIDForBrowserRequest, render_process_id_);
+  DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_WINDOW, provider_type_);
+  DCHECK_EQ(kDocumentMainThreadId, render_thread_id_);
 
-  if (associated_registration_.get()) {
-    SendAssociateRegistrationMessage();
-    if (dispatcher_host_ && associated_registration_->active_version()) {
-      Send(new ServiceWorkerMsg_SetControllerServiceWorker(
-          render_thread_id_, provider_id(),
-          GetOrCreateServiceWorkerHandle(
-              associated_registration_->active_version()),
-          false /* shouldNotifyControllerChange */));
-    }
-  }
+  DCHECK_NE(ChildProcessHost::kInvalidUniqueID, process_id);
+  DCHECK_NE(MSG_ROUTING_NONE, frame_routing_id);
+
+  FinalizeInitialization(process_id, frame_routing_id, dispatcher_host);
 }
 
 void ServiceWorkerProviderHost::SendUpdateFoundMessage(
@@ -684,4 +691,30 @@ void ServiceWorkerProviderHost::Send(IPC::Message* message) const {
   dispatcher_host_->Send(message);
 }
 
+void ServiceWorkerProviderHost::FinalizeInitialization(
+    int process_id,
+    int frame_routing_id,
+    ServiceWorkerDispatcherHost* dispatcher_host) {
+  render_process_id_ = process_id;
+  route_id_ = frame_routing_id;
+  dispatcher_host_ = dispatcher_host;
+
+  for (const GURL& pattern : associated_patterns_)
+    IncreaseProcessReference(pattern);
+
+  for (auto& key_registration : matching_registrations_)
+    IncreaseProcessReference(key_registration.second->pattern());
+
+  if (associated_registration_.get()) {
+    SendAssociateRegistrationMessage();
+    if (dispatcher_host_ && associated_registration_->active_version()) {
+      Send(new ServiceWorkerMsg_SetControllerServiceWorker(
+          render_thread_id_, provider_id(),
+          GetOrCreateServiceWorkerHandle(
+              associated_registration_->active_version()),
+          false /* shouldNotifyControllerChange */));
+    }
+  }
+}
+
 }  // namespace content