Use install_url instead of start_url for web app service worker registration install pass

This CL updates the background web app install process to use the
install_url instead of the start_url for registering the site's service
worker.

This is to avoid loading the start_url during start up (potentially
very resource intensive).

Note that this may cause policy installed web apps to fail to preinstall
the service worker if they happen to be using an install URL that
doesn't register a service worker (unlikely). This regression is very
mild though as the SW will just register when the user visits the site.

Default installed web apps (Canvas) already serve the service worker
from their install URL.

Bug: 1123435
Change-Id: I96349dddb43001ff54082b3f830d11b066378c76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389483
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803798}

chrome/browser/web_applications/components/pending_app_manager.cc

@@ -113,10 +113,10 @@ void PendingAppManager::ClearRegistrationCallbackForTesting() {
   registration_callback_ = RegistrationCallback();
 }
 
-void PendingAppManager::OnRegistrationFinished(const GURL& launch_url,
+void PendingAppManager::OnRegistrationFinished(const GURL& install_url,
                                                RegistrationResultCode result) {
   if (registration_callback_)
-    registration_callback_.Run(launch_url, result);
+    registration_callback_.Run(install_url, result);
 }
 
 void PendingAppManager::InstallForSynchronizeCallback(

chrome/browser/web_applications/pending_app_manager_impl.cc

@@ -105,18 +105,18 @@ PendingAppManagerImpl::CreateInstallationTask(
 }
 
 std::unique_ptr<PendingAppRegistrationTaskBase>
-PendingAppManagerImpl::StartRegistration(GURL launch_url) {
+PendingAppManagerImpl::StartRegistration(GURL install_url) {
   return std::make_unique<PendingAppRegistrationTask>(
-      launch_url, url_loader_.get(), web_contents_.get(),
+      install_url, url_loader_.get(), web_contents_.get(),
       base::BindOnce(&PendingAppManagerImpl::OnRegistrationFinished,
-                     weak_ptr_factory_.GetWeakPtr(), launch_url));
+                     weak_ptr_factory_.GetWeakPtr(), install_url));
 }
 
 void PendingAppManagerImpl::OnRegistrationFinished(
-    const GURL& launch_url,
+    const GURL& install_url,
     RegistrationResultCode result) {
-  DCHECK_EQ(current_registration_->launch_url(), launch_url);
-  PendingAppManager::OnRegistrationFinished(launch_url, result);
+  DCHECK_EQ(current_registration_->install_url(), install_url);
+  PendingAppManager::OnRegistrationFinished(install_url, result);
 
   current_registration_.reset();
   PostMaybeStartNext();
@@ -213,7 +213,7 @@ void PendingAppManagerImpl::StartInstallationTask(
   DCHECK(!current_install_);
   if (current_registration_) {
     // Preempt current registration.
-    pending_registrations_.push_front(current_registration_->launch_url());
+    pending_registrations_.push_front(current_registration_->install_url());
     current_registration_.reset();
   }
   current_install_ = std::move(task);
@@ -278,18 +278,19 @@ void PendingAppManagerImpl::CurrentInstallationFinished(
   if (app_id && code == InstallResultCode::kSuccessNewInstall &&
       base::FeatureList::IsEnabled(
           features::kDesktopPWAsCacheDuringDefaultInstall)) {
-    const GURL& launch_url = registrar()->GetAppLaunchURL(*app_id);
+    const GURL& install_url =
+        current_install_->task->install_options().install_url;
     bool is_local_resource =
-        launch_url.scheme() == content::kChromeUIScheme ||
-        launch_url.scheme() == content::kChromeUIUntrustedScheme;
+        install_url.scheme() == content::kChromeUIScheme ||
+        install_url.scheme() == content::kChromeUIUntrustedScheme;
     // TODO(crbug.com/809304): Call CreateWebContentsIfNecessary() instead of
     // checking web_contents_ once major migration of default hosted apps to web
     // apps has completed.
     // Temporarily using offline manifest migrations (in which |web_contents_|
     // is nullptr) in order to avoid overwhelming migrated-to web apps with hits
     // for service worker registrations.
-    if (!launch_url.is_empty() && !is_local_resource && web_contents_)
-      pending_registrations_.push_back(launch_url);
+    if (!install_url.is_empty() && !is_local_resource && web_contents_)
+      pending_registrations_.push_back(install_url);
   }
 
   // Post a task to avoid InstallableManager crashing and do so before

chrome/browser/web_applications/pending_app_manager_impl_browsertest.cc

@@ -300,51 +300,53 @@ IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest,
 
 IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, RegistrationSucceeds) {
   ASSERT_TRUE(embedded_test_server()->Start());
-  GURL launch_url(
-      embedded_test_server()->GetURL("/banners/manifest_test_page.html"));
-  GURL url(embedded_test_server()->GetURL(
-      "/banners/manifest_no_service_worker.html"));
 
-  ExternalInstallOptions install_options = CreateInstallOptions(url);
+  // Delay service worker registration to second load to simulate it not loading
+  // during the initial install pass.
+  GURL install_url(embedded_test_server()->GetURL(
+      "/web_apps/service_worker_on_second_load.html"));
+
+  ExternalInstallOptions install_options = CreateInstallOptions(install_url);
   install_options.bypass_service_worker_check = true;
   InstallApp(std::move(install_options));
   EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result_code_.value());
   WebAppRegistrationWaiter(&pending_app_manager())
-      .AwaitNextRegistration(launch_url, RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(install_url, RegistrationResultCode::kSuccess);
   CheckServiceWorkerStatus(
-      url, content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER);
+      install_url,
+      content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER);
 }
 
 IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, AlreadyRegistered) {
   ASSERT_TRUE(embedded_test_server()->Start());
-  GURL launch_url(
-      embedded_test_server()->GetURL("/banners/manifest_test_page.html"));
   {
-    GURL url(embedded_test_server()->GetURL(
-        "/banners/"
-        "manifest_no_service_worker.html?manifest=manifest_short_name_only."
-        "json"));
-    ExternalInstallOptions install_options = CreateInstallOptions(url);
+    // Delay service worker registration to second load to simulate it not
+    // loading during the initial install pass.
+    GURL install_url(embedded_test_server()->GetURL(
+        "/web_apps/service_worker_on_second_load.html"));
+    ExternalInstallOptions install_options = CreateInstallOptions(install_url);
     install_options.force_reinstall = true;
     install_options.bypass_service_worker_check = true;
     InstallApp(std::move(install_options));
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result_code_.value());
     WebAppRegistrationWaiter(&pending_app_manager())
-        .AwaitNextRegistration(launch_url, RegistrationResultCode::kSuccess);
+        .AwaitNextRegistration(install_url, RegistrationResultCode::kSuccess);
   }
+
   CheckServiceWorkerStatus(
-      launch_url,
+      embedded_test_server()->GetURL("/web_apps/basic.html"),
       content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER);
+
   {
-    GURL url(embedded_test_server()->GetURL(
-        "/banners/manifest_no_service_worker.html"));
-    ExternalInstallOptions install_options = CreateInstallOptions(url);
+    GURL install_url(
+        embedded_test_server()->GetURL("/web_apps/no_service_worker.html"));
+    ExternalInstallOptions install_options = CreateInstallOptions(install_url);
     install_options.force_reinstall = true;
     install_options.bypass_service_worker_check = true;
     InstallApp(std::move(install_options));
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result_code_.value());
     WebAppRegistrationWaiter(&pending_app_manager())
-        .AwaitNextRegistration(launch_url,
+        .AwaitNextRegistration(install_url,
                                RegistrationResultCode::kAlreadyRegistered);
   }
 }
@@ -398,7 +400,8 @@ IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, CannotFetchManifest) {
 IN_PROC_BROWSER_TEST_P(PendingAppManagerImplBrowserTest, RegistrationTimeout) {
   ASSERT_TRUE(embedded_test_server()->Start());
   PendingAppRegistrationTask::SetTimeoutForTesting(0);
-  GURL url(embedded_test_server()->GetURL("/web_apps/no_service_worker.html"));
+  GURL url(embedded_test_server()->GetURL(
+      "/banners/manifest_no_service_worker.html"));
   CheckServiceWorkerStatus(url,
                            content::ServiceWorkerCapability::NO_SERVICE_WORKER);
 

chrome/browser/web_applications/pending_app_manager_impl_unittest.cc

@@ -142,8 +142,8 @@ class TestPendingAppManagerImpl : public PendingAppManagerImpl {
 
   size_t registration_run_count() { return registration_run_count_; }
 
-  const GURL& last_registered_launch_url() {
-    return last_registered_launch_url_;
+  const GURL& last_registered_install_url() {
+    return last_registered_install_url_;
   }
 
   void SetNextInstallationTaskResult(const GURL& app_url,
@@ -175,10 +175,10 @@ class TestPendingAppManagerImpl : public PendingAppManagerImpl {
   }
 
   std::unique_ptr<PendingAppRegistrationTaskBase> StartRegistration(
-      GURL launch_url) override {
+      GURL install_url) override {
     ++registration_run_count_;
-    last_registered_launch_url_ = launch_url;
-    return std::make_unique<TestPendingAppRegistrationTask>(launch_url, this);
+    last_registered_install_url_ = install_url;
+    return std::make_unique<TestPendingAppRegistrationTask>(install_url, this);
   }
 
   void OnInstallCalled(const ExternalInstallOptions& install_options) {
@@ -299,23 +299,23 @@ class TestPendingAppManagerImpl : public PendingAppManagerImpl {
   class TestPendingAppRegistrationTask : public PendingAppRegistrationTaskBase {
    public:
     TestPendingAppRegistrationTask(
-        const GURL& launch_url,
+        const GURL& install_url,
         TestPendingAppManagerImpl* pending_app_manager_impl)
-        : PendingAppRegistrationTaskBase(launch_url),
+        : PendingAppRegistrationTaskBase(install_url),
           pending_app_manager_impl_(pending_app_manager_impl) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
           base::BindOnce(&TestPendingAppRegistrationTask::OnProgress,
-                         weak_ptr_factory_.GetWeakPtr(), launch_url));
+                         weak_ptr_factory_.GetWeakPtr(), install_url));
     }
     ~TestPendingAppRegistrationTask() override = default;
 
    private:
-    void OnProgress(GURL launch_url) {
+    void OnProgress(GURL install_url) {
       if (pending_app_manager_impl_->MaybePreemptRegistration())
         return;
       pending_app_manager_impl_->OnRegistrationFinished(
-          launch_url, RegistrationResultCode::kSuccess);
+          install_url, RegistrationResultCode::kSuccess);
     }
 
     TestPendingAppManagerImpl* const pending_app_manager_impl_;
@@ -329,7 +329,7 @@ class TestPendingAppManagerImpl : public PendingAppManagerImpl {
   TestAppRegistrar* test_app_registrar_;
 
   std::vector<ExternalInstallOptions> install_options_list_;
-  GURL last_registered_launch_url_;
+  GURL last_registered_install_url_;
   size_t install_run_count_ = 0;
   size_t registration_run_count_ = 0;
 
@@ -475,8 +475,8 @@ class PendingAppManagerImplTest
     return install_finalizer_->uninstall_external_web_app_urls();
   }
 
-  const GURL& last_registered_launch_url() {
-    return pending_app_manager_impl_->last_registered_launch_url();
+  const GURL& last_registered_install_url() {
+    return pending_app_manager_impl_->last_registered_install_url();
   }
 
   const GURL& last_uninstalled_app_url() {
@@ -526,9 +526,9 @@ TEST_P(PendingAppManagerImplTest, Install_Succeeds) {
   EXPECT_EQ(GetFooInstallOptions(), last_install_options());
 
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(FooLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(FooWebAppUrl(), RegistrationResultCode::kSuccess);
   EXPECT_EQ(1U, registration_run_count());
-  EXPECT_EQ(FooLaunchUrl(), last_registered_launch_url());
+  EXPECT_EQ(FooWebAppUrl(), last_registered_install_url());
 }
 
 TEST_P(PendingAppManagerImplTest, Install_SerialCallsDifferentApps) {
@@ -580,11 +580,11 @@ TEST_P(PendingAppManagerImplTest, Install_SerialCallsDifferentApps) {
   }
 
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(FooLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(FooWebAppUrl(), RegistrationResultCode::kSuccess);
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(BarLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(BarWebAppUrl(), RegistrationResultCode::kSuccess);
   EXPECT_EQ(3U, registration_run_count());
-  EXPECT_EQ(BarLaunchUrl(), last_registered_launch_url());
+  EXPECT_EQ(BarWebAppUrl(), last_registered_install_url());
 }
 
 TEST_P(PendingAppManagerImplTest, Install_ConcurrentCallsDifferentApps) {
@@ -1233,13 +1233,13 @@ TEST_P(PendingAppManagerImplTest, Install_PendingMultipleInstallApps) {
       }));
 
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(QuxLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(QuxWebAppUrl(), RegistrationResultCode::kSuccess);
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(FooLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(FooWebAppUrl(), RegistrationResultCode::kSuccess);
   WebAppRegistrationWaiter(pending_app_manager_impl())
-      .AwaitNextRegistration(BarLaunchUrl(), RegistrationResultCode::kSuccess);
+      .AwaitNextRegistration(BarWebAppUrl(), RegistrationResultCode::kSuccess);
   EXPECT_EQ(3U, registration_run_count());
-  EXPECT_EQ(BarLaunchUrl(), last_registered_launch_url());
+  EXPECT_EQ(BarWebAppUrl(), last_registered_install_url());
 }
 
 TEST_P(PendingAppManagerImplTest, InstallApps_PendingInstall) {

chrome/browser/web_applications/pending_app_registration_task.cc

@@ -17,19 +17,19 @@
 namespace web_app {
 
 PendingAppRegistrationTaskBase::PendingAppRegistrationTaskBase(
-    const GURL& launch_url)
-    : launch_url_(launch_url) {}
+    const GURL& install_url)
+    : install_url_(install_url) {}
 
 PendingAppRegistrationTaskBase::~PendingAppRegistrationTaskBase() = default;
 
 int PendingAppRegistrationTask::registration_timeout_in_seconds_ = 40;
 
 PendingAppRegistrationTask::PendingAppRegistrationTask(
-    const GURL& launch_url,
+    const GURL& install_url,
     WebAppUrlLoader* url_loader,
     content::WebContents* web_contents,
     RegistrationCallback callback)
-    : PendingAppRegistrationTaskBase(launch_url),
+    : PendingAppRegistrationTaskBase(install_url),
       url_loader_(url_loader),
       web_contents_(web_contents),
       callback_(std::move(callback)) {
@@ -47,9 +47,9 @@ PendingAppRegistrationTask::PendingAppRegistrationTask(
       base::BindOnce(&PendingAppRegistrationTask::OnRegistrationTimeout,
                      weak_ptr_factory_.GetWeakPtr()));
 
-  // Check to see if there is already a service worker for the launch url.
+  // Check to see if there is already a service worker for the install url.
   service_worker_context_->CheckHasServiceWorker(
-      launch_url,
+      install_url,
       base::BindOnce(&PendingAppRegistrationTask::OnDidCheckHasServiceWorker,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -60,7 +60,7 @@ PendingAppRegistrationTask::~PendingAppRegistrationTask() {
 }
 
 void PendingAppRegistrationTask::OnRegistrationCompleted(const GURL& scope) {
-  if (!content::ServiceWorkerContext::ScopeMatches(scope, launch_url()))
+  if (!content::ServiceWorkerContext::ScopeMatches(scope, install_url()))
     return;
 
   registration_timer_.Stop();
@@ -99,7 +99,7 @@ void PendingAppRegistrationTask::OnWebContentsReady(
 
   // No action is needed when the URL loads.
   // We wait for OnRegistrationCompleted (or registration timeout).
-  url_loader_->LoadUrl(launch_url(), web_contents_,
+  url_loader_->LoadUrl(install_url(), web_contents_,
                        WebAppUrlLoader::UrlComparison::kExact,
                        base::DoNothing());
 }

chrome/browser/web_applications/pending_app_registration_task.h

@@ -31,20 +31,20 @@ class PendingAppRegistrationTaskBase
  public:
   ~PendingAppRegistrationTaskBase() override;
 
-  const GURL& launch_url() const { return launch_url_; }
+  const GURL& install_url() const { return install_url_; }
 
  protected:
-  explicit PendingAppRegistrationTaskBase(const GURL& launch_url);
+  explicit PendingAppRegistrationTaskBase(const GURL& install_url);
 
  private:
-  const GURL launch_url_;
+  const GURL install_url_;
 };
 
 class PendingAppRegistrationTask : public PendingAppRegistrationTaskBase {
  public:
   using RegistrationCallback = base::OnceCallback<void(RegistrationResultCode)>;
 
-  PendingAppRegistrationTask(const GURL& launch_url,
+  PendingAppRegistrationTask(const GURL& install_url,
                              WebAppUrlLoader* url_loader,
                              content::WebContents* web_contents,
                              RegistrationCallback callback);

chrome/browser/web_applications/test/web_app_registration_waiter.cc

@@ -11,8 +11,8 @@ namespace web_app {
 WebAppRegistrationWaiter::WebAppRegistrationWaiter(PendingAppManager* manager)
     : manager_(manager) {
   manager_->SetRegistrationCallbackForTesting(base::BindLambdaForTesting(
-      [this](const GURL& launch_url, RegistrationResultCode code) {
-        CHECK_EQ(launch_url_, launch_url);
+      [this](const GURL& install_url, RegistrationResultCode code) {
+        CHECK_EQ(install_url_, install_url);
         CHECK_EQ(code_, code);
         run_loop_.Quit();
       }));
@@ -23,9 +23,9 @@ WebAppRegistrationWaiter::~WebAppRegistrationWaiter() {
 }
 
 void WebAppRegistrationWaiter::AwaitNextRegistration(
-    const GURL& launch_url,
+    const GURL& install_url,
     RegistrationResultCode code) {
-  launch_url_ = launch_url;
+  install_url_ = install_url;
   code_ = code;
   run_loop_.Run();
 }

chrome/browser/web_applications/test/web_app_registration_waiter.h

@@ -16,13 +16,13 @@ class WebAppRegistrationWaiter {
   explicit WebAppRegistrationWaiter(PendingAppManager* manager);
   ~WebAppRegistrationWaiter();
 
-  void AwaitNextRegistration(const GURL& launch_url,
+  void AwaitNextRegistration(const GURL& install_url,
                              RegistrationResultCode code);
 
  private:
   PendingAppManager* const manager_;
   base::RunLoop run_loop_;
-  GURL launch_url_;
+  GURL install_url_;
   RegistrationResultCode code_;
 };
 

chrome/test/data/web_apps/service_worker_on_second_load.html

+<!DOCTYPE html>
+<html>
+<head>
+  <link rel="manifest" href="service_worker_on_second_load.json">
+  <link rel="icon" href="basic-48.png">
+</head>
+<body>
+  <h1>Service worker on second load</h1>
+  <script>
+    if (localStorage.getItem('seen')) {
+      navigator.serviceWorker.register('/web_apps/service_worker.js');
+    }
+    localStorage.setItem('seen', true);
+  </script>
+</body>
+</html>

chrome/test/data/web_apps/service_worker_on_second_load.json

+{
+  "name": "Service worker on second load",
+  "icons": [
+    {
+      "src": "basic-48.png",
+      "sizes": "48x48",
+      "type": "image/png"
+    },
+    {
+      "src": "basic-192.png",
+      "sizes": "192x192",
+      "type": "image/png"
+    }
+  ],
+  "start_url": "service_worker_on_second_load.html",
+  "display": "standalone"
+}