PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/browser/service_worker/service_worker_request_handler.cc

Index: content/browser/service_worker/service_worker_request_handler.cc
diff --git a/content/browser/service_worker/service_worker_request_handler.cc b/content/browser/service_worker/service_worker_request_handler.cc
index 6d7d04b2e0a1b99a9101916faee3f4dd1612a9fb..59c8fea67fc7db4a32b13e4bfb07010b4aeeeac7 100644
--- a/content/browser/service_worker/service_worker_request_handler.cc
+++ b/content/browser/service_worker/service_worker_request_handler.cc
@@ -6,6 +6,9 @@
 
 #include <string>
 
+#include "base/command_line.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/loader/navigation_url_loader_impl_core.h"

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

are the navstack includes still needed?

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

No, that's leftovers from earlier attempts, removed.

 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
@@ -15,7 +18,9 @@
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/resource_context.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/origin_util.h"
+#include "ipc/ipc_message.h"

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

is this needed? what brought this include in?

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

MSG_ROUTING_NONE

 #include "net/base/net_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_interceptor.h"
@@ -51,6 +56,63 @@ class ServiceWorkerRequestInterceptor
 
 }  // namespace
 
+// PlzNavigate
+void ServiceWorkerRequestHandler::InitializeForNavigation(
+    net::URLRequest* request,
+    ServiceWorkerContextWrapper* context_wrapper,
+    storage::BlobStorageContext* blob_storage_context,
+    int provider_id,
+    bool skip_service_worker,
+    ResourceType resource_type,
+    RequestContextType request_context_type,
+    RequestContextFrameType frame_type,
+    scoped_refptr<ResourceRequestBody> body) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(

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

CHECK vs DCHECK?

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

We use CHECK for PlzNavigate checks in the browser. If current navigation end up
calling this method, something went very very wrong down the road.

+      switches::kEnableBrowserSideNavigation));
+
+  // Create the handler even for insecure HTTP since it's used in the
+  // case of redirect to HTTPS.
+  if (!request->url().SchemeIsHTTPOrHTTPS() &&
+      !OriginCanAccessServiceWorkers(request->url())) {
+    return;
+  }
+
+  if (!context_wrapper || !context_wrapper->context()) {
+    return;
+  }
+
+  // Initialize the SWProviderHost.
+  ServiceWorkerProviderHost* provider_host = new ServiceWorkerProviderHost(
+      ServiceWorkerProviderHost::kVirtualProcessIDForBrowserRequest,
+      MSG_ROUTING_NONE, provider_id, SERVICE_WORKER_PROVIDER_FOR_WINDOW,
+      context_wrapper->context()->AsWeakPtr(), nullptr);
+
+  // Add to context map.
+  context_wrapper->context()->AddNavigationProviderHost(provider_host);

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

I really don't like the passing around of int types as a way of expressing
ownership. It's hard to follow. Somebody trying to read this code a few years
from now will appreciate clarifying how this works in code.

It'd be good to make the xfer of ownership more obvious. Upon return from this
function there is no scoped_ptr to the object anywhere so who exactly owns it is
not at all clear. The navstack is resposible for taking care to delete it
eventually by id but that is not at all obvious. InitializeForNavigation returns
nothing but as a hidden artifact it allocates an object that the caller is
reponsible for managing. The method the caller must use when its done is a
method on a completely different class. And the ids are coined by yet another
class.

I'd like to consolidate and clean that up.

It's not difficult to make it more obvious. One way clarify could be to have a
wrapper 'handle' class coordinate its construction and conditional destruction
so its all inside of one small .cc file inside instead of being scattered about.
That wrapper could also help to avoid things like generating an id without ever
generating a corresponding object. Or retaining the id value after the object it
refers to has been deleted.

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

I changed the API in ServiceWorkerContextCore to use scoped_ptr instead.

+
+  if (skip_service_worker) {
+    // TODO(horo): Does this work properly for PlzNavigate?
+    if (ServiceWorkerUtils::IsMainResourceType(resource_type)) {
+      provider_host->SetDocumentUrl(net::SimplifyUrlForRequest(request->url()));
+      provider_host->SetTopmostFrameUrl(request->first_party_for_cookies());
+      // A page load with skip_service_worker should be triggered by
+      // shift-reload, so retain all live matching registrations.
+      provider_host->AddAllMatchingRegistrations();
+    }
+    return;
+  }
+
+  scoped_ptr<ServiceWorkerRequestHandler> handler(
+      provider_host->CreateRequestHandler(
+          FETCH_REQUEST_MODE_SAME_ORIGIN, FETCH_CREDENTIALS_MODE_INCLUDE,
+          FetchRedirectMode::MANUAL_MODE, resource_type, request_context_type,
+          frame_type, blob_storage_context->AsWeakPtr(), body));
+  if (!handler)
+    return;
+
+  request->SetUserData(&kUserDataKey, handler.release());
+}
+
 void ServiceWorkerRequestHandler::InitializeHandler(
     net::URLRequest* request,
     ServiceWorkerContextWrapper* context_wrapper,
@@ -128,30 +190,33 @@ bool ServiceWorkerRequestHandler::IsControlledByServiceWorker(
 
 void ServiceWorkerRequestHandler::PrepareForCrossSiteTransfer(
     int old_process_id) {
+  CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(

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

ditto question about CHECK vs DCHECK

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

There is no cross-site transfer mechanics in PlzNavigate. Once again if this
ends up being called, we have a big problem on our hands and it is much safer to
crash.

+      switches::kEnableBrowserSideNavigation));
   if (!provider_host_ || !context_)
     return;
   old_process_id_ = old_process_id;
   old_provider_id_ = provider_host_->provider_id();
-  host_for_cross_site_transfer_ =
-      context_->TransferProviderHostOut(old_process_id,
-                                        provider_host_->provider_id());
+  host_for_cross_site_transfer_ = context_->TransferProviderHostOut(
+      old_process_id, provider_host_->provider_id());
   DCHECK_EQ(provider_host_.get(), host_for_cross_site_transfer_.get());
 }
 
 void ServiceWorkerRequestHandler::CompleteCrossSiteTransfer(
     int new_process_id, int new_provider_id) {
+  CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
   if (!host_for_cross_site_transfer_.get() || !context_)
     return;
   DCHECK_EQ(provider_host_.get(), host_for_cross_site_transfer_.get());
-  context_->TransferProviderHostIn(
-      new_process_id,
-      new_provider_id,
-      host_for_cross_site_transfer_.Pass());
+  context_->TransferProviderHostIn(new_process_id, new_provider_id,
+                                   host_for_cross_site_transfer_.Pass());
   DCHECK_EQ(provider_host_->provider_id(), new_provider_id);
 }
 
 void ServiceWorkerRequestHandler::MaybeCompleteCrossSiteTransferInOldProcess(
     int old_process_id) {
+  CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
   if (!host_for_cross_site_transfer_.get() || !context_ ||
       old_process_id_ != old_process_id) {
     return;