Set the redirected document URL of ServiceWorkerProviderHost even if skip_service_worker is set.

Currently the redirected document URL of ServiceWorkerProviderHost is not
correctly set if skip_service_worker is set. This inconsistency is causing the
renderer kill crashes in ServiceWorkerDispatcherHost::OnGetRegistration() and
OnRegisterServiceWorker().

BUG=630496,630495

Review-Url: https://codereview.chromium.org/2562523003
Cr-Commit-Position: refs/heads/master@{#437493}

content/browser/service_worker/service_worker_provider_host.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/guid.h"
+#include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "content/browser/message_port_message_filter.h"
@@ -27,6 +28,7 @@
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/origin_util.h"
+#include "net/base/url_util.h"
 
 namespace content {
 
@@ -37,6 +39,38 @@ namespace {
 // going down.
 int g_next_navigation_provider_id = -2;
 
+// A request handler derivative used to handle navigation requests when
+// skip_service_worker flag is set. It tracks the document URL and sets the url
+// to the provider host.
+class ServiceWorkerURLTrackingRequestHandler
+    : public ServiceWorkerRequestHandler {
+ public:
+  ServiceWorkerURLTrackingRequestHandler(
+      base::WeakPtr<ServiceWorkerContextCore> context,
+      base::WeakPtr<ServiceWorkerProviderHost> provider_host,
+      base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
+      ResourceType resource_type)
+      : ServiceWorkerRequestHandler(context,
+                                    provider_host,
+                                    blob_storage_context,
+                                    resource_type) {}
+  ~ServiceWorkerURLTrackingRequestHandler() override {}
+
+  // Called via custom URLRequestJobFactory.
+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate,
+      ResourceContext* resource_context) override {
+    const GURL stripped_url = net::SimplifyUrlForRequest(request->url());
+    provider_host_->SetDocumentUrl(stripped_url);
+    provider_host_->SetTopmostFrameUrl(request->first_party_for_cookies());
+    return nullptr;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ServiceWorkerURLTrackingRequestHandler);
+};
+
 }  // anonymous namespace
 
 ServiceWorkerProviderHost::OneShotGetReadyCallback::OneShotGetReadyCallback(
@@ -333,19 +367,24 @@ ServiceWorkerProviderHost::CreateRequestHandler(
     RequestContextType request_context_type,
     RequestContextFrameType frame_type,
     base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
-    scoped_refptr<ResourceRequestBodyImpl> body) {
+    scoped_refptr<ResourceRequestBodyImpl> body,
+    bool skip_service_worker) {
+  if (skip_service_worker) {
+    if (!ServiceWorkerUtils::IsMainResourceType(resource_type))
+      return std::unique_ptr<ServiceWorkerRequestHandler>();
+    return base::MakeUnique<ServiceWorkerURLTrackingRequestHandler>(
+        context_, AsWeakPtr(), blob_storage_context, resource_type);
+  }
   if (IsHostToRunningServiceWorker()) {
-    return std::unique_ptr<ServiceWorkerRequestHandler>(
-        new ServiceWorkerContextRequestHandler(
-            context_, AsWeakPtr(), blob_storage_context, resource_type));
+    return base::MakeUnique<ServiceWorkerContextRequestHandler>(
+        context_, AsWeakPtr(), blob_storage_context, resource_type);
   }
   if (ServiceWorkerUtils::IsMainResourceType(resource_type) ||
       controlling_version()) {
-    return std::unique_ptr<ServiceWorkerRequestHandler>(
-        new ServiceWorkerControlleeRequestHandler(
-            context_, AsWeakPtr(), blob_storage_context, request_mode,
-            credentials_mode, redirect_mode, resource_type,
-            request_context_type, frame_type, body));
+    return base::MakeUnique<ServiceWorkerControlleeRequestHandler>(
+        context_, AsWeakPtr(), blob_storage_context, request_mode,
+        credentials_mode, redirect_mode, resource_type, request_context_type,
+        frame_type, body);
   }
   return std::unique_ptr<ServiceWorkerRequestHandler>();
 }

content/browser/service_worker/service_worker_provider_host.h

@@ -174,7 +174,8 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
       RequestContextType request_context_type,
       RequestContextFrameType frame_type,
       base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
-      scoped_refptr<ResourceRequestBodyImpl> body);
+      scoped_refptr<ResourceRequestBodyImpl> body,
+      bool skip_service_worker);
 
   // Used to get a ServiceWorkerObjectInfo to send to the renderer. Finds an
   // existing ServiceWorkerHandle, and increments its reference count, or else

content/browser/service_worker/service_worker_request_handler.cc

@@ -67,20 +67,11 @@ void FinalizeHandlerInitialization(
     RequestContextType request_context_type,
     RequestContextFrameType frame_type,
     scoped_refptr<ResourceRequestBodyImpl> body) {
-  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());
-    }
-    return;
-  }
-
   std::unique_ptr<ServiceWorkerRequestHandler> handler(
       provider_host->CreateRequestHandler(
           request_mode, credentials_mode, redirect_mode, resource_type,
           request_context_type, frame_type, blob_storage_context->AsWeakPtr(),
-          body));
+          body, skip_service_worker));
   if (!handler)
     return;
 

third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/bypass-for-network-redirect.php

+<?php
+  $url = "http://127.0.0.1:8000/inspector/service-workers/resources/" .
+         "bypass-for-network-redirected.html";
+  header("Location: $url");
+?>

third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/bypass-for-network-redirected.html

+<script>
+navigator.serviceWorker.getRegistration().then(r =>
+  console.log("getRegistration finished"));
+</script>

third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-bypass-for-network-redirect-expected.txt

+CONSOLE MESSAGE: line 3: getRegistration finished
+Tests "Bypass for network" checkbox with redirection doesn't cause crash.
+
+
+Success
+

third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-bypass-for-network-redirect.html

+<html>
+<head>
+<script src="../inspector-test.js"></script>
+<script src="service-workers-test.js"></script>
+<script src="../resources-test.js"></script>
+<script>
+
+function loadIframe()
+{
+    var frame = document.createElement('iframe');
+    frame.src = "http://localhost:8000/inspector/service-workers/resources/" +
+                "bypass-for-network-redirect.php"
+    document.body.appendChild(frame);
+}
+
+function test()
+{
+    UI.inspectorView.showPanel("sources")
+        .then(function(){
+            Common.settings.settingForTest("bypassServiceWorker").set(true);
+            var callback;
+            var promise = new Promise((fulfill) => callback = fulfill);
+            InspectorTest.addConsoleSniffer(message => {
+                if (message.messageText == "getRegistration finished") {
+                  callback();
+                }
+              }, true);
+            InspectorTest.evaluateInPage("loadIframe()");
+            return promise;
+        })
+        .then(function() {
+            InspectorTest.addResult("Success");
+            InspectorTest.completeTest();
+        })
+        .catch(function(exception) {
+            InspectorTest.addResult("Error");
+            InspectorTest.addResult(exception);
+            InspectorTest.completeTest();
+        });
+}
+
+</script>
+</head>
+<body onload="runTest()">
+<p>Tests "Bypass for network" checkbox with redirection doesn't cause crash.<p>
+</body>
+</html>