PlzNavigate: Make ServiceWorker work with PlzNavigate.

content/browser/service_worker/service_worker_request_handler.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_request_handler.h"
 
 #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"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_url_request_job.h"
 #include "content/common/resource_request_body.h"
 #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"
 #include "storage/browser/blob/blob_storage_context.h"
 
 namespace content {
 
 namespace {
 
 int kUserDataKey;  // Key value is not important.
@@ skipping 15 matching lines @@
         request, network_delegate, resource_context_);
   }
 
  private:
   ResourceContext* resource_context_;
   DISALLOW_COPY_AND_ASSIGN(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,
     storage::BlobStorageContext* blob_storage_context,
     int process_id,
     int provider_id,
     bool skip_service_worker,
     FetchRequestMode request_mode,
     FetchCredentialsMode credentials_mode,
     FetchRedirectMode redirect_mode,
@@ skipping 11 matching lines @@
   if (!context_wrapper || !context_wrapper->context() ||
       provider_id == kInvalidServiceWorkerProviderId) {
     return;
   }
 
   ServiceWorkerProviderHost* provider_host =
       context_wrapper->context()->GetProviderHost(process_id, provider_id);
   if (!provider_host || !provider_host->IsContextAlive())
     return;
 
   if (skip_service_worker) {

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

please refctor and share common code between these two init methods

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

This is kinda awkward due to the large number of arguments passed around but
done.

     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;
   }
 
@@ skipping 25 matching lines @@
     net::URLRequest* request) {
   ServiceWorkerRequestHandler* handler = GetHandler(request);
   if (!handler || !handler->provider_host_)
     return false;
   return handler->provider_host_->associated_registration() ||
          handler->provider_host_->running_hosted_version();
 }
 
 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;
   }
   CompleteCrossSiteTransfer(old_process_id_, old_provider_id_);
 }
 
 ServiceWorkerRequestHandler::~ServiceWorkerRequestHandler() {
 }
 
 ServiceWorkerRequestHandler::ServiceWorkerRequestHandler(
     base::WeakPtr<ServiceWorkerContextCore> context,
     base::WeakPtr<ServiceWorkerProviderHost> provider_host,
     base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
     ResourceType resource_type)
     : context_(context),
       provider_host_(provider_host),
       blob_storage_context_(blob_storage_context),
       resource_type_(resource_type),
       old_process_id_(0),
       old_provider_id_(kInvalidServiceWorkerProviderId) {
 }
 
 }  // namespace content