PlzNavigate: Make ServiceWorker work with PlzNavigate.

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 2de8b88307279714cb2493a75286313f6df5f8d4..5065999b96bce0c4b730b63d779c9c3f03209788 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 
+#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/utf_string_conversions.h"
@@ -23,6 +24,7 @@
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/origin_util.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/net_util.h"
@@ -725,10 +727,26 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
                                     bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
     return;
   }
-  scoped_ptr<ServiceWorkerProviderHost> provider_host(
-      new ServiceWorkerProviderHost(render_process_id_, route_id, provider_id,
-                                    provider_type, GetContext()->AsWeakPtr(),
-                                    this));
+
+  scoped_ptr<ServiceWorkerProviderHost> provider_host;
+  if (ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id)) {
+    // PlzNavigate
+    CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(

[michaeln, 2015/09/01 01:15:23]

Should this be treated as a bad_message instead of a CHECK? A buggy or
compromised renderer could provide garbage for this id and I don't think the
browser should crash as a result.

[Fabrice (no longer in Chrome), 2015/09/07 12:09:48]

That's a good point. How about changing it to a DCHECK here instead? We'd still
have a way to catch it on tests.

[nasko, 2015/09/14 21:28:32]

If we know that this can be hit from compromised renderer, then using CHECK is
the wrong thing to do. It is indeed a great fit for bad_message.

[clamy, 2015/09/14 21:34:59]

Note that historically, we have put CHECKs in PlzNavigate specific IPC handlers
in RFH. I think those can be triggered by a compromised renderer as well.

[Fabrice (no longer in Chrome), 2015/09/15 15:38:54]

I do not know if having a test at all is relevant then. Essentially, we would be
testing that if the provider_id received is negative, then PlzNavigate must be
enabled. This would only work during the transition period. Either way, the
second test on line 737 already takes care of that, indirectly.
I will remove this CHECK in a future patch.

+        switches::kEnableBrowserSideNavigation));
+    provider_host = GetContext()->TakeBrowserProviderHost(provider_id);
+    if (provider_host == nullptr) {
+      bad_message::ReceivedBadMessage(
+          this, bad_message::SWDH_PROVIDER_CREATED_NO_HOST);
+      return;
+    }

[michaeln, 2015/09/01 01:15:23]

That CHECK(CommandLine) might make more sense on this line, but it's at the top
of CompleteBrowserIntialized() so maybe its not needed in here at all?

[Fabrice (no longer in Chrome), 2015/09/07 12:09:48]

Acknowledged.

+    DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_WINDOW, provider_type);
+    provider_host->CompleteBrowserInitialized(render_process_id_, route_id);
+  } else {
+    provider_host =
+        scoped_ptr<ServiceWorkerProviderHost>(new ServiceWorkerProviderHost(
+            render_process_id_, route_id, provider_id, provider_type,
+            GetContext()->AsWeakPtr(), this));
+  }
   GetContext()->AddProviderHost(provider_host.Pass());
 }