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 d27ceb40602b71fea60092d55c96cb3e9b71b7dc..dce2d0080f4f9cb6c5a4cfb88e8bfffeaec7052a 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 {
 
@@ -59,6 +61,21 @@ ServiceWorkerClientInfo FocusOnUIThread(int render_process_id,
 
 }  // anonymous namespace
 
+// PlzNavigate
+int ServiceWorkerProviderHost::kVirtualProcessIDForBrowserRequest = -2;
+
+// PlzNavigate
+int ServiceWorkerProviderHost::next_navigation_provider_id_ = -2;
+
+// PlzNavigate
+int ServiceWorkerProviderHost::GetNextNavigationProviderId() {

[michaeln, 2015/10/01 01:42:16]

please DCHECK_CURRENTLY_ON(BrowserThread::UI), since its unusual for sw code to
run on the UI thread i'd like to call attention to that

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

I moved it so there should no longer be any confusion here.

+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(

[michaeln, 2015/10/01 01:42:16]

Does this need to be a runtime check? I don't think this method should be
getting called unless this switch is set. If that's right, can you change this
to a DCHECK instead.

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

It's called in both cases actually, when initializing RequestNavigationParams.

+          switches::kEnableBrowserSideNavigation)) {
+    return next_navigation_provider_id_--;
+  }
+  return kInvalidServiceWorkerProviderId;
+}
+
 ServiceWorkerProviderHost::OneShotGetReadyCallback::OneShotGetReadyCallback(
     const GetRegistrationForReadyCallback& callback)
     : callback(callback),
@@ -87,6 +104,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;
@@ -512,6 +535,8 @@ void ServiceWorkerProviderHost::CompleteCrossSiteTransfer(
     int new_provider_id,
     ServiceWorkerProviderType new_provider_type,
     ServiceWorkerDispatcherHost* new_dispatcher_host) {
+  CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));

[michaeln, 2015/10/01 01:42:16]

ah, i didn't notice the ! char!

Ok, thanx for pointing out the CrossSiteXferring and PlzNavigating are mutually
exclusive. Is there reason to CHECK this or would a DCHECK to document it and
catch programmer errors be enuf?

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

I think we should leave a CHECK here as stated above.

[michaeln, 2015/10/01 23:11:12]

If this check is to defend against bad ipc inputs, please hoist the test higher
up in the stack as mentioned below.

[Fabrice (no longer in Chrome), 2015/10/02 16:37:34]

Looking at the calling stack, this is actually taken care of in
ResourceDispatcherHostImpl::BeginRequest. Removing it here.

   DCHECK_EQ(ChildProcessHost::kInvalidUniqueID, render_process_id_);
   DCHECK_NE(ChildProcessHost::kInvalidUniqueID, new_process_id);
   DCHECK_NE(MSG_ROUTING_NONE, new_frame_id);
@@ -541,6 +566,42 @@ void ServiceWorkerProviderHost::CompleteCrossSiteTransfer(
   }
 }
 
+// PlzNavigate
+void ServiceWorkerProviderHost::CompleteNavigationInitialized(
+    int process_id,
+    int frame_id,
+    ServiceWorkerDispatcherHost* dispatcher_host) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(

[michaeln, 2015/10/01 01:42:16]

same question about CHECK vs DCHECK?

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

If we get here, it means the browser received a negative provider ID from the
renderer (could have been compromised) and was able to retrieve a SWProviderHost
with that ID from the map that should only be used with PlzNavigate.

[michaeln, 2015/10/01 23:11:11]

Please use BadMessageRecieved for that and put the test closer to where messages
are first received.

It's good to put checks of that nature where the inputs from the renderer are
first received. Also we don't want to kill the browser process in cases like
that. We want to kill the borken renderer. See usages of BadMessageRecieved for
examples of that. Notice that all (modulo one special case) uses of that
function under service_worker/ are in the ServiceWorkerDispatcherHost class.
That is the ipc message dispatching class which first handles a message from
potentially busted renderer.

If your other usages of CHECK are to defend against bad ipc inputs, please
shuffle them around to where messages are initially received and convert them to
BadMessageReceived.

+      switches::kEnableBrowserSideNavigation));
+  DCHECK_EQ(kVirtualProcessIDForBrowserRequest, render_process_id_);
+  DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_WINDOW, provider_type_);
+  DCHECK_EQ(kDocumentMainThreadId, render_thread_id_);
+
+  DCHECK_NE(ChildProcessHost::kInvalidUniqueID, process_id);
+  DCHECK_NE(MSG_ROUTING_NONE, frame_id);
+
+  render_process_id_ = process_id;
+  route_id_ = frame_id;
+  dispatcher_host_ = dispatcher_host;
+
+  for (const GURL& pattern : associated_patterns_)

[michaeln, 2015/10/01 01:42:16]

CompleteCrossSiteTransfer and CompleteNavigationInitialized contain a lot of the
same code, can you factor out the common bits and reuse it

[Fabrice (no longer in Chrome), 2015/10/01 18:29:55]

Done.

+    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 */));
+    }
+  }
+}
+
 void ServiceWorkerProviderHost::SendUpdateFoundMessage(
     int registration_handle_id) {
   if (!dispatcher_host_)