Reland "Make custom protocol handlers work with service workers' fetch event"

This relands [1] but removing the test
RegisterProtocolHandlerAndServiceWorkerInterceptor.DoNotRegisterFetchListener
which is flaky on mac and not essential for testing the bug fix.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2379672

Original change's description:
> [Sheriff] Revert "Make custom protocol handlers work with service workers' fetch event"
>
> This reverts commit c007dfe1.
>
> Reason for revert: RegisterProtocolHandlerAndServiceWorkerInterceptor.DoNotRegisterFetchListener is very flaky on mac (and possibly elsewhere).
>
> Sample failure:
> [ RUN      ] RegisterProtocolHandlerAndServiceWorkerInterceptor.DoNotRegisterFetchListener
> [23479:30231:1019/055809.421386:WARNING:notification_platform_bridge_mac.mm(374)] AlertNotificationService: XPC connection invalidated.
> [23479:775:1019/055809.917320:ERROR:device_event_log_impl.cc(211)] [05:58:09.906] FIDO: touch_id_context.mm:127 Touch ID authenticator unavailable because keychain-access-group entitlement is missing or incorrect
> [23479:96771:1019/055810.044487:WARNING:embedded_test_server.cc(667)] Request not handled. Returning 404: /favicon.ico
> [23479:775:1019/055810.218450:INFO:CONSOLE(0)] "Failed to launch 'web+html:path' because the scheme does not have a registered handler.", source:  (0)
> ../../content/public/test/browser_test_base.cc:702: Failure
> Failed
> RunLoop::Run() timed out.
> Stack trace:
> 0   browser_tests                       0x0000000110d3e9c2 _ZN4base8internal7InvokerINS0_9BindStateIZNS_4test20ScopedRunLoopTimeoutC1ERKNS_8LocationENS_9TimeDeltaENS_17RepeatingCallbackIFNSt3__112basic_stringIcNSA_11char_traitsIcEENSA_9allocatorIcEEEEvEEEE3$_0JS5_SI_EEEFvvEE3RunEPNS0_13BindStateBaseE + 290
> 1   browser_tests                       0x0000000110c5916f base::(anonymous namespace)::OnRunLoopTimeout(base::RunLoop*, base::OnceCallback<void ()>) + 31
> 2   browser_tests                       0x0000000110c5a1a3 base::internal::Invoker<base::internal::BindState<void (*)(base::RunLoop*, base::OnceCallback<void ()>), base::internal::UnretainedWrapper<base::RunLoop>, base::RepeatingCallback<void ()> >, void ()>::RunOnce(base::internal::BindStateBase*) + 67
> 3   browser_tests                       0x000000010cf23a83 void base::internal::CancelableCallbackImpl<base::OnceCallback<void ()> >::ForwardOnce<>() + 35
> 4   browser_tests                       0x000000010cf23b34 base::internal::Invoker<base::internal::BindState<void (base::internal::CancelableCallbackImpl<base::OnceCallback<void ()> >::*)(), base::WeakPtr<base::internal::CancelableCallbackImpl<base::OnceCallback<void ()> > > >, void ()>::RunOnce(base::internal::BindStateBase*) + 148
> 5   browser_tests                       0x0000000110c7a946 base::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 310
> 6   browser_tests                       0x0000000110c8fb55 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::sequence_manager::LazyNow*) + 405
> 7   browser_tests                       0x0000000110c8f858 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() + 168
> 8   browser_tests                       0x0000000110ce29a1 ___ZN4base24MessagePumpCFRunLoopBase13RunWorkSourceEPv_block_invoke + 49
> 9   browser_tests                       0x0000000110cdb662 base::mac::CallWithEHFrame(void () block_pointer) + 10
> 10  browser_tests                       0x0000000110ce223f base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 63
> 11  CoreFoundation                      0x00007fff38600884 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
>
> Original change's description:
> > Make custom protocol handlers work with service workers' fetch event
> >
> > When a service worker intercepts a page that is later registered as a
> > handler for a custom scheme via navigator.registerProtocolHandler, and
> > if one clicks a link with this custom scheme, then the service worker
> > is only able to intercept the page after reload. This CL fixes that
> > bug by allowing the service worker to intercept the page after
> > registering the custom scheme.
> >
> > A new browser unit test is added to verify this fix. This also makes
> > the following manual WPT tests pass (*):
> >   protocol-handler-fragment-manual.https.html
> >   protocol-handler-path-manual.https.html
> >   protocol-handler-query-manual.https.html
> >
> > (*) https://w3c-test.org/html/webappapis/system-state-and-capabilities/the-navigator-object/
> >
> > Bug: 522370
> > Change-Id: I95eda9025885838665e9735ac9f47a3d65d89aa8
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379672
> > Commit-Queue: Frédéric Wang <fwang@igalia.com>
> > Reviewed-by: Dominick Ng <dominickn@chromium.org>
> > Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#818416}
>
> TBR=falken@chromium.org,fwang@igalia.com,dominickn@chromium.org
>
> Change-Id: I026705f0116f3e0d684c8b1d80847aec85ec36db
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 522370
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485837
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#818511}


Bug: 522370
Change-Id: Id30235a64a5a6a5c5d783ee686df321c7c853b16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2487107Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Frédéric Wang <fwang@igalia.com>
Cr-Commit-Position: refs/heads/master@{#820255}

chrome/browser/chrome_content_browser_client.cc

@@ -1811,6 +1811,18 @@ bool ChromeContentBrowserClient::IsHandledURL(const GURL& url) {
   return ProfileIOData::IsHandledURL(url);
 }
 
+bool ChromeContentBrowserClient::HasCustomSchemeHandler(
+    content::BrowserContext* browser_context,
+    const std::string& scheme) {
+  if (ProtocolHandlerRegistry* protocol_handler_registry =
+          ProtocolHandlerRegistryFactory::GetForBrowserContext(
+              browser_context)) {
+    return protocol_handler_registry->IsHandledProtocol(scheme);
+  }
+
+  return false;
+}
+
 bool ChromeContentBrowserClient::CanCommitURL(
     content::RenderProcessHost* process_host,
     const GURL& url) {

chrome/browser/chrome_content_browser_client.h

@@ -185,6 +185,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   bool LogWebUIUrl(const GURL& web_ui_url) override;
   bool IsWebUIAllowedToMakeNetworkRequests(const url::Origin& origin) override;
   bool IsHandledURL(const GURL& url) override;
+  bool HasCustomSchemeHandler(content::BrowserContext* browser_context,
+                              const std::string& scheme) override;
   bool CanCommitURL(content::RenderProcessHost* process_host,
                     const GURL& url) override;
   void OverrideNavigationParams(

chrome/browser/custom_handlers/protocol_handler_registry_browsertest.cc

@@ -17,7 +17,6 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/permissions/permission_request_manager.h"
-#include "components/permissions/test/mock_permission_prompt_factory.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
@@ -32,6 +31,30 @@
 
 using content::WebContents;
 
+namespace {
+
+class ProtocolHandlerChangeWaiter : public ProtocolHandlerRegistry::Observer {
+ public:
+  explicit ProtocolHandlerChangeWaiter(ProtocolHandlerRegistry* registry) {
+    registry_observer_.Add(registry);
+  }
+  ProtocolHandlerChangeWaiter(const ProtocolHandlerChangeWaiter&) = delete;
+  ProtocolHandlerChangeWaiter& operator=(const ProtocolHandlerChangeWaiter&) =
+      delete;
+  ~ProtocolHandlerChangeWaiter() override = default;
+
+  void Wait() { run_loop_.Run(); }
+  // ProtocolHandlerRegistry::Observer:
+  void OnProtocolHandlerRegistryChanged() override { run_loop_.Quit(); }
+
+ private:
+  ScopedObserver<ProtocolHandlerRegistry, ProtocolHandlerRegistry::Observer>
+      registry_observer_{this};
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
 class RegisterProtocolHandlerBrowserTest : public InProcessBrowserTest {
  public:
   RegisterProtocolHandlerBrowserTest() { }
@@ -154,13 +177,10 @@ IN_PROC_BROWSER_TEST_F(RegisterProtocolHandlerExtensionBrowserTest, Basic) {
 #if defined(OS_MAC)
   ASSERT_TRUE(test::RegisterAppWithLaunchServices());
 #endif
-  permissions::PermissionRequestManager* manager =
-      permissions::PermissionRequestManager::FromWebContents(
-          browser()->tab_strip_model()->GetActiveWebContents());
-  auto prompt_factory =
-      std::make_unique<permissions::MockPermissionPromptFactory>(manager);
-  prompt_factory->set_response_type(
-      permissions::PermissionRequestManager::ACCEPT_ALL);
+  permissions::PermissionRequestManager::FromWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents())
+      ->set_auto_response_for_test(
+          permissions::PermissionRequestManager::ACCEPT_ALL);
 
   const extensions::Extension* extension =
       LoadExtension(test_data_dir_.AppendASCII("protocol_handler"));
@@ -170,16 +190,66 @@ IN_PROC_BROWSER_TEST_F(RegisterProtocolHandlerExtensionBrowserTest, Basic) {
       "chrome-extension://" + extension->id() + "/test.html";
 
   // Register the handler.
-  ui_test_utils::NavigateToURL(browser(), GURL(handler_url));
-  ASSERT_TRUE(content::ExecuteScript(
-      browser()->tab_strip_model()->GetActiveWebContents(),
-      "navigator.registerProtocolHandler('geo', 'test.html?%s', 'test');"));
-
-  // Wait until the prompt is "displayed" and "accepted".
-  base::RunLoop().RunUntilIdle();
+  {
+    ProtocolHandlerRegistry* registry =
+        ProtocolHandlerRegistryFactory::GetForBrowserContext(
+            browser()->profile());
+    ProtocolHandlerChangeWaiter waiter(registry);
+    ui_test_utils::NavigateToURL(browser(), GURL(handler_url));
+    ASSERT_TRUE(content::ExecuteScript(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        "navigator.registerProtocolHandler('geo', 'test.html?%s', 'test');"));
+    waiter.Wait();
+  }
 
   // Test the handler.
   ui_test_utils::NavigateToURL(browser(), GURL("geo:test"));
   ASSERT_EQ(GURL(handler_url + "?geo%3Atest"),
             browser()->tab_strip_model()->GetActiveWebContents()->GetURL());
 }
+
+class RegisterProtocolHandlerAndServiceWorkerInterceptor
+    : public InProcessBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    // Navigate to the test page.
+    ui_test_utils::NavigateToURL(
+        browser(), embedded_test_server()->GetURL(
+                       "/protocol_handler/service_workers/"
+                       "test_protocol_handler_and_service_workers.html"));
+
+    // Bypass permission dialogs for registering new protocol handlers.
+    permissions::PermissionRequestManager::FromWebContents(
+        browser()->tab_strip_model()->GetActiveWebContents())
+        ->set_auto_response_for_test(
+            permissions::PermissionRequestManager::ACCEPT_ALL);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(RegisterProtocolHandlerAndServiceWorkerInterceptor,
+                       RegisterFetchListenerForHTMLHandler) {
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Register a service worker intercepting requests to the HTML handler.
+  EXPECT_EQ(true, content::EvalJs(web_contents,
+                                  "registerFetchListenerForHTMLHandler();"));
+
+  {
+    // Register a HTML handler with a user gesture.
+    ProtocolHandlerRegistry* registry =
+        ProtocolHandlerRegistryFactory::GetForBrowserContext(
+            browser()->profile());
+    ProtocolHandlerChangeWaiter waiter(registry);
+    ASSERT_TRUE(content::ExecJs(web_contents, "registerHTMLHandler();"));
+    waiter.Wait();
+  }
+
+  // Verify that a page with the registered scheme is managed by the service
+  // worker, not the HTML handler.
+  EXPECT_EQ(true,
+            content::EvalJs(web_contents,
+                            "pageWithCustomSchemeHandledByServiceWorker();"));
+}

chrome/test/data/protocol_handler/service_workers/fetch_listener_for_html_handler.js

+// Copyright 2020 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.
+
+self.addEventListener('fetch', function(event) {
+  if (!event.request.url.includes('handler.html'))
+    return;
+
+  event.respondWith(new Response(
+      `<script>
+window.opener.postMessage({handled_by_service_worker: true}, '*');</script>`,
+      {headers: {'Content-Type': 'text/html'}}));
+});

chrome/test/data/protocol_handler/service_workers/handler.html

+<script>
+  window.opener.postMessage({handled_by_service_worker: false}, '*');
+</script>

chrome/test/data/protocol_handler/service_workers/test_protocol_handler_and_service_workers.html

+<script src="test_protocol_handler_and_service_workers.js"></script>

chrome/test/data/protocol_handler/service_workers/test_protocol_handler_and_service_workers.js

+// Copyright 2020 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.
+
+// browser_tests
+// --gtest_filter=ProtocolHandlerBrowserTest.*
+
+async function registerFetchListenerForHTMLHandler() {
+  await navigator.serviceWorker.register(
+      '/protocol_handler/service_workers/fetch_listener_for_html_handler.js');
+  await navigator.serviceWorker.ready;
+  return true;
+}
+
+function absoluteURL(path) {
+  return `${location.origin}/protocol_handler/service_workers/${path}`;
+}
+
+function registerHTMLHandler() {
+  navigator.registerProtocolHandler(
+      'web+html', absoluteURL('handler.html?url=%s'), 'title');
+}
+
+async function handledByServiceWorker(url) {
+  const a = document.body.appendChild(document.createElement('a'));
+  a.href = url;
+  a.target = '_blank';
+  let handled_by_service_worker;
+  await new Promise(resolve => {
+    window.addEventListener('message', function(event) {
+      handled_by_service_worker = event.data.handled_by_service_worker;
+      event.source.close();
+      resolve();
+    }, {once: true})
+    a.click();
+  });
+  return handled_by_service_worker;
+}
+
+function pageWithCustomSchemeHandledByServiceWorker() {
+  return handledByServiceWorker('web+html:path');
+}

content/browser/service_worker/service_worker_main_resource_loader_interceptor.cc

@@ -19,6 +19,7 @@
 #include "content/browser/service_worker/service_worker_main_resource_handle.h"
 #include "content/browser/service_worker/service_worker_main_resource_handle_core.h"
 #include "content/public/browser/browser_task_traits.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/url_constants.h"
 #include "mojo/public/cpp/bindings/pending_associated_receiver.h"
@@ -174,7 +175,17 @@ void MaybeCreateLoaderOnCoreThread(
                      std::move(fallback_callback)));
 }
 
-bool SchemeMaySupportRedirectingToHTTPS(const GURL& url) {
+bool SchemeMaySupportRedirectingToHTTPS(BrowserContext* browser_context,
+                                        const GURL& url) {
+  // If there is a registered protocol handler for this scheme, the embedder is
+  // expected to redirect `url` to a registered URL in a URLLoaderThrottle, and
+  // the interceptor will operate on the registered URL. Note that the HTML
+  // specification requires that the registered URL is HTTPS.
+  // https://html.spec.whatwg.org/multipage/system-state.html#normalize-protocol-handler-parameters
+  if (GetContentClient()->browser()->HasCustomSchemeHandler(browser_context,
+                                                            url.scheme()))
+    return true;
+
 #if defined(OS_CHROMEOS)
   return url.SchemeIs(kExternalFileScheme);
 #else   // OS_CHROMEOS
@@ -202,7 +213,8 @@ ServiceWorkerMainResourceLoaderInterceptor::CreateForNavigation(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (!ShouldCreateForNavigation(
-          url, request_info.begin_params->request_destination)) {
+          url, request_info.begin_params->request_destination,
+          navigation_handle->context_wrapper()->browser_context())) {
     return nullptr;
   }
 
@@ -377,7 +389,8 @@ ServiceWorkerMainResourceLoaderInterceptor::
 // static
 bool ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation(
     const GURL& url,
-    network::mojom::RequestDestination request_destination) {
+    network::mojom::RequestDestination request_destination,
+    BrowserContext* browser_context) {
   // <embed> and <object> navigations must bypass the service worker, per the
   // discussion in https://w3c.github.io/ServiceWorker/#implementer-concerns.
   if (request_destination == network::mojom::RequestDestination::kEmbed ||
@@ -385,10 +398,10 @@ bool ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation(
     return false;
   }
 
-  // Create the handler even for insecure HTTP since it's used in the
+  // Create the interceptor even for insecure HTTP since it's used in the
   // case of redirect to HTTPS.
   return url.SchemeIsHTTPOrHTTPS() || OriginCanAccessServiceWorkers(url) ||
-         SchemeMaySupportRedirectingToHTTPS(url);
+         SchemeMaySupportRedirectingToHTTPS(browser_context, url);
 }
 
 void ServiceWorkerMainResourceLoaderInterceptor::RequestHandlerWrapper(

content/browser/service_worker/service_worker_main_resource_loader_interceptor.h

@@ -96,7 +96,8 @@ class CONTENT_EXPORT ServiceWorkerMainResourceLoaderInterceptor final
   // created for a navigation to |url|.
   static bool ShouldCreateForNavigation(
       const GURL& url,
-      network::mojom::RequestDestination request_destination);
+      network::mojom::RequestDestination request_destination,
+      BrowserContext* browser_context);
 
   // Given as a callback to NavigationURLLoaderImpl.
   void RequestHandlerWrapper(

content/browser/service_worker/service_worker_main_resource_loader_interceptor_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "content/browser/service_worker/service_worker_main_resource_loader_interceptor.h"
 
+#include "content/public/test/browser_task_environment.h"
+#include "content/public/test/test_browser_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -11,12 +13,28 @@ namespace content {
 
 class ServiceWorkerMainResourceLoaderInterceptorTest : public testing::Test {
  public:
+  ServiceWorkerMainResourceLoaderInterceptorTest() = default;
+  ServiceWorkerMainResourceLoaderInterceptorTest(
+      const ServiceWorkerMainResourceLoaderInterceptorTest&) = delete;
+  ServiceWorkerMainResourceLoaderInterceptorTest& operator=(
+      const ServiceWorkerMainResourceLoaderInterceptorTest&) = delete;
+  ~ServiceWorkerMainResourceLoaderInterceptorTest() override = default;
+
+  void SetUp() override {
+    browser_context_ = std::make_unique<TestBrowserContext>();
+  }
+
   bool ShouldCreateForNavigation(
       const GURL& url,
       network::mojom::RequestDestination request_destination) {
     return ServiceWorkerMainResourceLoaderInterceptor::
-        ShouldCreateForNavigation(url, request_destination);
+        ShouldCreateForNavigation(url, request_destination,
+                                  browser_context_.get());
   }
+
+ private:
+  BrowserTaskEnvironment task_environment_{BrowserTaskEnvironment::IO_MAINLOOP};
+  std::unique_ptr<TestBrowserContext> browser_context_;
 };
 
 TEST_F(ServiceWorkerMainResourceLoaderInterceptorTest,

content/public/browser/content_browser_client.cc

@@ -200,6 +200,12 @@ bool ContentBrowserClient::IsHandledURL(const GURL& url) {
   return false;
 }
 
+bool ContentBrowserClient::HasCustomSchemeHandler(
+    content::BrowserContext* browser_context,
+    const std::string& scheme) {
+  return false;
+}
+
 bool ContentBrowserClient::CanCommitURL(RenderProcessHost* process_host,
                                         const GURL& site_url) {
   return true;

content/public/browser/content_browser_client.h

@@ -463,6 +463,13 @@ class CONTENT_EXPORT ContentBrowserClient {
   // protocol handlers.
   virtual bool IsHandledURL(const GURL& url);
 
+  // Returns whether a custom handler is registered for the scheme of the
+  // specified URL scheme.
+  // https://html.spec.whatwg.org/multipage/system-state.html#custom-handlers
+  // TODO(crbug.com/1139176) Move custom protocol handler code to content.
+  virtual bool HasCustomSchemeHandler(content::BrowserContext* browser_context,
+                                      const std::string& scheme);
+
   // Returns whether the given process is allowed to commit |url|.  This is a
   // more conservative check than IsSuitableHost, since it is used after a
   // navigation has committed to ensure that the process did not exceed its