desktop-pwas: Add icon URL fallback for bookmark app sync installs

As part of https://chromium-review.googlesource.com/c/chromium/src/+/2134089
our sync install flow for bookmark apps was changed to fetch the
app manifest from the start_url where previously we used sync data
and only fetched icon URLs.
The new flow is significantly more prone to install failure (due to
network, site config, CPU load). In the event of failure the app won't
be installed with any icons.

This CL adds the legacy "only fetch icons" code path back in the event
of failure to lessen the user impact of install failure.

This CL only affects bookmark app sync install. BMO sync install will
come via unifying the sync install process to use the same code.

Bug: 1090227
Change-Id: I063b0d087ab482fe625a8f37850212e99b9d35bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226006
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776878}

chrome/browser/sync/test/integration/two_client_web_apps_sync_test.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chrome/browser/web_applications/components/app_icon_manager.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
 #include "chrome/browser/web_applications/test/web_app_install_observer.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
@@ -374,6 +375,51 @@ IN_PROC_BROWSER_TEST_P(TwoClientWebAppsSyncTest, SyncWithoutUsingNameFallback) {
             "Basic web app");
 }
 
+IN_PROC_BROWSER_TEST_P(TwoClientWebAppsSyncTest, SyncUsingIconUrlFallback) {
+  // TODO(crbug.com/1090227): Support this behaviour for BMO.
+  if (GetParam() == ProviderType::kWebApps)
+    return;
+
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  Profile* source_profile = GetProfile(0);
+  Profile* dest_profile = GetProfile(1);
+
+  WebAppInstallObserver dest_install_observer(dest_profile);
+
+  // Install app with name.
+  WebApplicationInfo info;
+  info.title = base::UTF8ToUTF16("Blue icon");
+  info.app_url = GURL("https://does-not-exist.org");
+  WebApplicationIconInfo icon_info;
+  icon_info.square_size_px = 192;
+  icon_info.url = embedded_test_server()->GetURL("/web_apps/blue-192.png");
+  info.icon_infos.push_back(icon_info);
+  AppId app_id = InstallApp(info, GetProfile(0));
+  EXPECT_EQ(GetRegistrar(source_profile).GetAppShortName(app_id), "Blue icon");
+
+  // Wait for app to sync across.
+  AppId synced_app_id = dest_install_observer.AwaitNextInstall();
+  EXPECT_EQ(synced_app_id, app_id);
+  EXPECT_EQ(GetRegistrar(dest_profile).GetAppShortName(app_id), "Blue icon");
+
+  // Make sure icon downloaded despite not loading start_url.
+  {
+    base::RunLoop run_loop;
+    WebAppProvider::Get(dest_profile)
+        ->icon_manager()
+        .ReadSmallestIcon(
+            synced_app_id, 192,
+            base::BindLambdaForTesting([&run_loop](const SkBitmap& bitmap) {
+              EXPECT_EQ(bitmap.getColor(0, 0), SK_ColorBLUE);
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          TwoClientWebAppsSyncTest,
                          ::testing::Values(ProviderType::kBookmarkApps,

chrome/browser/web_applications/web_app_install_manager.cc

@@ -219,10 +219,11 @@ void WebAppInstallManager::EnqueueInstallBookmarkAppFromSync(
       web_application_info->open_as_window ? DisplayMode::kStandalone
                                            : DisplayMode::kBrowser));
 
-  OnceInstallCallback task_completed_callback =
-      base::BindOnce(&WebAppInstallManager::OnBookmarkAppInstalledAfterSync,
-                     base::Unretained(this), bookmark_app_id,
-                     std::move(web_application_info), std::move(callback));
+  OnceInstallCallback task_completed_callback = base::BindOnce(
+      &WebAppInstallManager::
+          LoadAndInstallWebAppFromManifestWithFallbackCompleted_ForBookmarkAppSync,
+      base::Unretained(this), bookmark_app_id, std::move(web_application_info),
+      std::move(callback));
 
   base::OnceClosure start_task = base::BindOnce(
       &WebAppInstallTask::LoadAndInstallWebAppFromManifestWithFallback,
@@ -319,29 +320,38 @@ void WebAppInstallManager::MaybeEnqueuePendingBookmarkAppInstalls() {
   pending_bookmark_app_installs_.clear();
 }
 
-void WebAppInstallManager::OnBookmarkAppInstalledAfterSync(
-    const AppId& bookmark_app_id,
-    std::unique_ptr<WebApplicationInfo> web_application_info,
-    OnceInstallCallback callback,
-    const AppId& web_app_id,
-    InstallResultCode code) {
+void WebAppInstallManager::
+    LoadAndInstallWebAppFromManifestWithFallbackCompleted_ForBookmarkAppSync(
+        const AppId& bookmark_app_id,
+        std::unique_ptr<WebApplicationInfo> web_application_info,
+        OnceInstallCallback callback,
+        const AppId& web_app_id,
+        InstallResultCode code) {
   // TODO(loyso): Record |code| for this specific case in
   // Webapp.BookmarkAppInstalledAfterSyncResult UMA.
   if (IsSuccess(code)) {
     DCHECK_EQ(bookmark_app_id, web_app_id);
     std::move(callback).Run(web_app_id, code);
-  } else {
-    // Install failed. Do the fallback install from info.
-    InstallFinalizer::FinalizeOptions options;
-    options.install_source = WebappInstallSource::SYNC;
-    options.locally_installed = kLocallyInstallWebAppsOnSync;
+    return;
+  }
+
+  // Install failed. Do the fallback install from info fetching just icon URLs.
+  auto task = std::make_unique<WebAppInstallTask>(
+      profile(), registrar(), shortcut_manager(), file_handler_manager(),
+      finalizer(), data_retriever_factory_.Run());
 
-    FilterAndResizeIconsGenerateMissing(web_application_info.get(),
-                                        /*icons_map=*/nullptr);
+  InstallFinalizer::FinalizeOptions finalize_options;
+  finalize_options.install_source = WebappInstallSource::SYNC;
+  finalize_options.locally_installed = kLocallyInstallWebAppsOnSync;
 
-    finalizer()->FinalizeInstall(*web_application_info, options,
-                                 std::move(callback));
-  }
+  base::OnceClosure start_task = base::BindOnce(
+      &WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons,
+      base::Unretained(task.get()), EnsureWebContentsCreated(),
+      std::move(web_application_info), finalize_options,
+      base::BindOnce(&WebAppInstallManager::OnQueuedTaskCompleted,
+                     base::Unretained(this), task.get(), std::move(callback)));
+
+  EnqueueTask(std::move(task), std::move(start_task));
 }
 
 void WebAppInstallManager::EnqueueTask(std::unique_ptr<WebAppInstallTask> task,
@@ -399,6 +409,10 @@ void WebAppInstallManager::OnQueuedTaskCompleted(WebAppInstallTask* task,
   OnInstallTaskCompleted(task, std::move(callback), app_id, code);
   task = nullptr;
 
+  // |callback| may have started another task.
+  if (is_running_queued_task_)
+    return;
+
   if (task_queue_.empty()) {
     web_contents_.reset();
     web_contents_ready_ = false;

chrome/browser/web_applications/web_app_install_manager.h

@@ -94,7 +94,8 @@ class WebAppInstallManager final : public InstallManager,
       const AppId& bookmark_app_id,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       OnceInstallCallback callback);
-  void OnBookmarkAppInstalledAfterSync(
+  // On failure will attempt a fallback install only loading icon URLs.
+  void LoadAndInstallWebAppFromManifestWithFallbackCompleted_ForBookmarkAppSync(
       const AppId& bookmark_app_id,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       OnceInstallCallback callback,

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -399,19 +399,14 @@ class WebAppInstallManagerTest : public WebAppTest {
     return result;
   }
 
-  TestDataRetriever* BuildDefaultDataRetriever(const GURL& launch_url) {
-    DCHECK(!data_retriever_);
-    data_retriever_ = std::make_unique<TestDataRetriever>();
-    data_retriever_->BuildDefaultDataToRetrieve(launch_url, launch_url);
-
+  void UseDefaultDataRetriever(const GURL& launch_url) {
     install_manager().SetDataRetrieverFactoryForTesting(
-        base::BindLambdaForTesting([this]() {
-          DCHECK(data_retriever_);
+        base::BindLambdaForTesting([launch_url]() {
+          auto data_retriever = std::make_unique<TestDataRetriever>();
+          data_retriever->BuildDefaultDataToRetrieve(launch_url, launch_url);
           return std::unique_ptr<WebAppDataRetriever>(
-              std::move(data_retriever_));
+              std::move(data_retriever));
         }));
-
-    return data_retriever_.get();
   }
 
   void DestroyManagers() {
@@ -444,9 +439,6 @@ class WebAppInstallManagerTest : public WebAppTest {
   TestWebAppUrlLoader* test_url_loader_ = nullptr;
   // Owned by icon_manager_:
   TestFileUtils* file_utils_ = nullptr;
-  // Initially owned by this test fixture. Passed to |install_manager_| and
-  // becomes nullptr.
-  std::unique_ptr<TestDataRetriever> data_retriever_;
 };
 
 TEST_F(WebAppInstallManagerTest,
@@ -967,29 +959,33 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_LoadSuccess) {
   const auto url2 = GURL("https://example.org/");
 
   url_loader().SetNextLoadUrlResult(url1, WebAppUrlLoader::Result::kUrlLoaded);
-  {
-    TestDataRetriever* data_retriever = BuildDefaultDataRetriever(url1);
-
-    auto web_site_application_info = std::make_unique<WebApplicationInfo>();
-    web_site_application_info->open_as_window = false;
-    web_site_application_info->display_mode = DisplayMode::kBrowser;
-    data_retriever->SetRendererWebApplicationInfo(
-        std::move(web_site_application_info));
-  }
+  install_manager().SetDataRetrieverFactoryForTesting(
+      base::BindLambdaForTesting([&]() {
+        auto data_retriever = std::make_unique<TestDataRetriever>();
+        data_retriever->BuildDefaultDataToRetrieve(url1, url1);
+        auto web_site_application_info = std::make_unique<WebApplicationInfo>();
+        web_site_application_info->open_as_window = false;
+        web_site_application_info->display_mode = DisplayMode::kBrowser;
+        data_retriever->SetRendererWebApplicationInfo(
+            std::move(web_site_application_info));
+        return std::unique_ptr<WebAppDataRetriever>(std::move(data_retriever));
+      }));
   const AppId app_id1 =
       InstallBookmarkAppFromSync(url1, /*server_open_as_window=*/true);
 
   url_loader().SetNextLoadUrlResult(url2, WebAppUrlLoader::Result::kUrlLoaded);
   url_loader().SetAboutBlankResultLoaded();
-  {
-    TestDataRetriever* data_retriever = BuildDefaultDataRetriever(url2);
-
-    auto web_site_application_info = std::make_unique<WebApplicationInfo>();
-    web_site_application_info->open_as_window = true;
-    web_site_application_info->display_mode = DisplayMode::kStandalone;
-    data_retriever->SetRendererWebApplicationInfo(
-        std::move(web_site_application_info));
-  }
+  install_manager().SetDataRetrieverFactoryForTesting(
+      base::BindLambdaForTesting([&]() {
+        auto data_retriever = std::make_unique<TestDataRetriever>();
+        data_retriever->BuildDefaultDataToRetrieve(url2, url2);
+        auto web_site_application_info = std::make_unique<WebApplicationInfo>();
+        web_site_application_info->open_as_window = false;
+        web_site_application_info->display_mode = DisplayMode::kStandalone;
+        data_retriever->SetRendererWebApplicationInfo(
+            std::move(web_site_application_info));
+        return std::unique_ptr<WebAppDataRetriever>(std::move(data_retriever));
+      }));
   const AppId app_id2 =
       InstallBookmarkAppFromSync(url2, /*server_open_as_window=*/false);
 
@@ -1062,25 +1058,30 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_TwoIcons_Success) {
     server_icon2_info.square_size_px = icon_size::k256;
     server_web_app_info->icon_infos.push_back(std::move(server_icon2_info));
   }
-  {
-    TestDataRetriever* data_retriever = BuildDefaultDataRetriever(url);
-
-    // Set the website manifest to be a copy of WebApplicationInfo from sync,
-    // as if they are the same.
-    std::unique_ptr<WebApplicationInfo> site_web_app_info =
-        std::make_unique<WebApplicationInfo>(*server_web_app_info);
-    data_retriever->SetRendererWebApplicationInfo(std::move(site_web_app_info));
-
-    IconsMap site_icons_map;
-    AddIconToIconsMap(icon1_url, icon_size::k128, SK_ColorBLUE,
-                      &site_icons_map);
-    AddIconToIconsMap(icon2_url, icon_size::k256, SK_ColorRED, &site_icons_map);
 
-    data_retriever->SetIcons(std::move(site_icons_map));
-  }
+  install_manager().SetDataRetrieverFactoryForTesting(
+      base::BindLambdaForTesting([&]() {
+        auto data_retriever = std::make_unique<TestDataRetriever>();
+        data_retriever->BuildDefaultDataToRetrieve(url, url);
+        // Set the website manifest to be a copy of WebApplicationInfo from
+        // sync, as if they are the same.
+        std::unique_ptr<WebApplicationInfo> site_web_app_info =
+            std::make_unique<WebApplicationInfo>(*server_web_app_info);
+        data_retriever->SetRendererWebApplicationInfo(
+            std::move(site_web_app_info));
+
+        IconsMap site_icons_map;
+        AddIconToIconsMap(icon1_url, icon_size::k128, SK_ColorBLUE,
+                          &site_icons_map);
+        AddIconToIconsMap(icon2_url, icon_size::k256, SK_ColorRED,
+                          &site_icons_map);
+
+        data_retriever->SetIcons(std::move(site_icons_map));
+        return std::unique_ptr<WebAppDataRetriever>(std::move(data_retriever));
+      }));
 
-  InstallResult result =
-      InstallBookmarkAppFromSync(app_id, std::move(server_web_app_info));
+  InstallResult result = InstallBookmarkAppFromSync(
+      app_id, std::make_unique<WebApplicationInfo>(*server_web_app_info));
   EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result.code);
   EXPECT_EQ(app_id, result.app_id);
 
@@ -1115,6 +1116,11 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_TwoIcons_Fallback) {
   // Induce a load failure:
   url_loader().SetNextLoadUrlResult(
       url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
+  install_manager().SetDataRetrieverFactoryForTesting(
+      base::BindLambdaForTesting([&]() {
+        return std::unique_ptr<WebAppDataRetriever>(
+            std::make_unique<TestDataRetriever>());
+      }));
 
   const AppId app_id = GenerateAppIdFromURL(url);
 
@@ -1166,7 +1172,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_NoIcons) {
   // Induce a load failure:
   url_loader().SetNextLoadUrlResult(
       url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
-  BuildDefaultDataRetriever(url);
+  UseDefaultDataRetriever(url);
   const AppId app_id = GenerateAppIdFromURL(url);
 
   auto web_app_info = std::make_unique<WebApplicationInfo>();
@@ -1199,7 +1205,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_ExpectAppIdFailed) {
                                     WebAppUrlLoader::Result::kUrlLoaded);
 
   // The web site has changed app url:
-  BuildDefaultDataRetriever(GURL{"https://example.org"});
+  UseDefaultDataRetriever(GURL{"https://example.org"});
 
   const AppId expected_app_id = GenerateAppIdFromURL(old_url);
 
@@ -1233,7 +1239,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_QueueNewInstall) {
   const GURL url{"https://example.com/path"};
   url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
 
-  BuildDefaultDataRetriever(url);
+  UseDefaultDataRetriever(url);
   const AppId bookmark_app_id = GenerateAppIdFromURL(url);
 
   auto server_web_application_info = std::make_unique<WebApplicationInfo>();
@@ -1326,7 +1332,7 @@ TEST_F(WebAppInstallManagerTest,
                                       WebAppUrlLoader::Result::kUrlLoaded});
 
   // Prepare web site data for next enqueued full install (the bookmark app).
-  BuildDefaultDataRetriever(url);
+  UseDefaultDataRetriever(url);
 
   auto server_bookmark_app_info = std::make_unique<WebApplicationInfo>();
   server_bookmark_app_info->app_url = url;
@@ -1352,7 +1358,7 @@ TEST_F(WebAppInstallManagerTest,
         EXPECT_EQ(url, web_apps_installed[0]->launch_url());
 
         // Prepare web site data for next enqueued full install (the web app).
-        BuildDefaultDataRetriever(url);
+        UseDefaultDataRetriever(url);
 
         install_manager().InstallWebAppsAfterSync(
             std::move(web_apps_installed),
@@ -1386,7 +1392,7 @@ TEST_F(WebAppInstallManagerTest,
             WebAppUrlLoader::Result::kFailedPageTookTooLong});
 
   // Prepare web site data for next enqueued full install (the bookmark app).
-  BuildDefaultDataRetriever(url);
+  UseDefaultDataRetriever(url);
 
   auto server_bookmark_app_info = std::make_unique<WebApplicationInfo>();
   server_bookmark_app_info->app_url = url;
@@ -1412,7 +1418,7 @@ TEST_F(WebAppInstallManagerTest,
         EXPECT_EQ(url, web_apps_installed[0]->launch_url());
 
         // Prepare web site data for next enqueued full install (the web app).
-        BuildDefaultDataRetriever(url);
+        UseDefaultDataRetriever(url);
 
         install_manager().InstallWebAppsAfterSync(
             std::move(web_apps_installed),

chrome/browser/web_applications/web_app_install_task.cc

@@ -19,7 +19,6 @@
 #include "chrome/browser/web_applications/components/app_shortcut_manager.h"
 #include "chrome/browser/web_applications/components/file_handler_manager.h"
 #include "chrome/browser/web_applications/components/install_bounce_metric.h"
-#include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_data_retriever.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
@@ -596,10 +595,38 @@ void WebAppInstallTask::OnDidCheckForIntentToPlayStore(
                      for_installable_site));
 }
 
+void WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons(
+    content::WebContents* web_contents,
+    std::unique_ptr<WebApplicationInfo> web_application_info,
+    InstallFinalizer::FinalizeOptions finalize_options,
+    InstallManager::OnceInstallCallback callback) {
+  CheckInstallPreconditions();
+
+  Observe(web_contents);
+  install_callback_ = std::move(callback);
+  install_source_ = finalize_options.install_source;
+  background_installation_ = true;
+
+  std::vector<GURL> icon_urls =
+      GetValidIconUrlsToDownload(*web_application_info);
+
+  // Skip downloading the page favicons as everything in is the URL list.
+  data_retriever_->GetIcons(
+      web_contents, icon_urls, /*skip_page_fav_icons=*/true,
+      install_source_ == WebappInstallSource::SYNC
+          ? WebAppIconDownloader::Histogram::kForSync
+          : WebAppIconDownloader::Histogram::kForCreate,
+      base::BindOnce(&WebAppInstallTask::OnIconsRetrieved,
+                     base::Unretained(this), std::move(web_application_info),
+                     finalize_options));
+}
+
 void WebAppInstallTask::OnIconsRetrieved(
     std::unique_ptr<WebApplicationInfo> web_app_info,
-    bool is_locally_installed,
+    InstallFinalizer::FinalizeOptions finalize_options,
     IconsMap icons_map) {
+  DCHECK(background_installation_);
+
   if (ShouldStopInstall())
     return;
 
@@ -608,12 +635,8 @@ void WebAppInstallTask::OnIconsRetrieved(
   // Installing from sync should not change icon links.
   FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map);
 
-  InstallFinalizer::FinalizeOptions options;
-  options.install_source = install_source_;
-  options.locally_installed = is_locally_installed;
-
   install_finalizer_->FinalizeInstall(
-      *web_app_info, options,
+      *web_app_info, finalize_options,
       base::BindOnce(&WebAppInstallTask::OnInstallFinalized,
                      weak_ptr_factory_.GetWeakPtr()));
 }

chrome/browser/web_applications/web_app_install_task.h

@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "chrome/browser/installable/installable_metrics.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/components/web_app_install_utils.h"
@@ -100,6 +101,15 @@ class WebAppInstallTask : content::WebContentsObserver {
       WebappInstallSource install_source,
       InstallManager::OnceInstallCallback callback);
 
+  // Fetches the icon URLs in |web_application_info| to populate the icon
+  // bitmaps. Once fetched uses the contents of |web_application_info| as the
+  // entire web app installation data.
+  void InstallWebAppFromInfoRetrieveIcons(
+      content::WebContents* web_contents,
+      std::unique_ptr<WebApplicationInfo> web_application_info,
+      InstallFinalizer::FinalizeOptions finalize_options,
+      InstallManager::OnceInstallCallback callback);
+
   // Starts a web app installation process using prefilled
   // |web_application_info| which holds all the data needed for installation.
   // InstallManager doesn't fetch a manifest.
@@ -197,7 +207,7 @@ class WebAppInstallTask : content::WebContentsObserver {
       bool should_intent_to_store);
 
   void OnIconsRetrieved(std::unique_ptr<WebApplicationInfo> web_app_info,
-                        bool is_locally_installed,
+                        InstallFinalizer::FinalizeOptions finalize_options,
                         IconsMap icons_map);
   void OnIconsRetrievedShowDialog(
       std::unique_ptr<WebApplicationInfo> web_app_info,