desktop-pwas: Treat icon processing the same for sync install as regular install

This CL removes some icon processing logic that we do differently for
sync initiated installs as they are unnecessary and contains the source
of a DCHECK failure.

We were DCHECK failing sync installation for sites that included icons
with no size specified because we were storing this as size 0. During
sync installation we would include this size 0 as part of the icons to
generate. This produced an invalid icon that failed the DCHECK in
ConvertWebAppToExtension() and caused the app sync install to fail
entirely.

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

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

@@ -7,14 +7,20 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/installable/installable_metrics.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/ui/browser.h"
+#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/install_manager.h"
 #include "chrome/browser/web_applications/test/web_app_install_observer.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/web_application_info.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/test_utils.h"
 
 namespace web_app {
@@ -246,6 +252,42 @@ IN_PROC_BROWSER_TEST_P(TwoClientWebAppsSyncTest, AppFieldsChangeDoesNotSync) {
   EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
 }
 
+// Tests that we don't crash when syncing an icon info with no size.
+// Context: https://crbug.com/1058283
+// Disabled because it takes too long to complete on the trybots.
+IN_PROC_BROWSER_TEST_P(TwoClientWebAppsSyncTest, DISABLED_SyncFaviconOnly) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  Profile* sourceProfile = GetProfile(0);
+  Profile* destProfile = GetProfile(1);
+
+  WebAppInstallObserver destInstallObserver(destProfile);
+
+  // Install favicon only page as web app.
+  AppId app_id;
+  {
+    Browser* browser = CreateBrowser(sourceProfile);
+    ui_test_utils::NavigateToURL(
+        browser, embedded_test_server()->GetURL("/web_apps/favicon_only.html"));
+    chrome::SetAutoAcceptWebAppDialogForTesting(true, true);
+    WebAppInstallObserver installObserver(sourceProfile);
+    chrome::ExecuteCommand(browser, IDC_CREATE_SHORTCUT);
+    app_id = installObserver.AwaitNextInstall();
+    chrome::SetAutoAcceptWebAppDialogForTesting(false, false);
+  }
+  EXPECT_EQ(GetRegistrar(sourceProfile).GetAppShortName(app_id),
+            "Favicon only");
+  EXPECT_EQ(GetRegistrar(sourceProfile).GetAppIconInfos(app_id).size(), 1u);
+
+  // Wait for app to sync across.
+  AppId synced_app_id = destInstallObserver.AwaitNextInstall();
+  EXPECT_EQ(synced_app_id, app_id);
+  EXPECT_EQ(GetRegistrar(destProfile).GetAppShortName(app_id), "Favicon only");
+  EXPECT_EQ(GetRegistrar(destProfile).GetAppIconInfos(app_id).size(), 1u);
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          TwoClientWebAppsSyncTest,
                          ::testing::Values(ProviderType::kBookmarkApps,

chrome/browser/web_applications/components/web_app_install_utils.cc

@@ -132,8 +132,7 @@ std::vector<GURL> GetValidIconUrlsToDownload(
 }
 
 void FilterAndResizeIconsGenerateMissing(WebApplicationInfo* web_app_info,
-                                         const IconsMap* icons_map,
-                                         bool is_for_sync) {
+                                         const IconsMap* icons_map) {
   // Ensure that all icons that are in web_app_info are present, by generating
   // icons for any sizes which have failed to download. This ensures that the
   // created manifest for the web app does not contain links to icons
@@ -141,18 +140,7 @@ void FilterAndResizeIconsGenerateMissing(WebApplicationInfo* web_app_info,
   std::vector<SkBitmap> square_icons;
   if (icons_map)
     FilterSquareIconsFromMap(*icons_map, &square_icons);
-  if (!is_for_sync)
-    FilterSquareIconsFromBitmaps(web_app_info->icon_bitmaps, &square_icons);
-
-  std::set<SquareSizePx> sizes_to_generate = SizesToGenerate();
-  if (is_for_sync) {
-    // Ensure that all icon widths in the web app info icon array are present in
-    // the sizes to generate set. This ensures that we will have all of the
-    // icon sizes from when the app was originally added, even if icon URLs are
-    // no longer accessible.
-    for (const auto& icon : web_app_info->icon_infos)
-      sizes_to_generate.insert(icon.square_size_px);
-  }
+  FilterSquareIconsFromBitmaps(web_app_info->icon_bitmaps, &square_icons);
 
   base::char16 icon_letter =
       web_app_info->title.empty()
@@ -162,7 +150,7 @@ void FilterAndResizeIconsGenerateMissing(WebApplicationInfo* web_app_info,
   // TODO(https://crbug.com/1029223): Don't resize before writing to disk, it's
   // not necessary and would simplify this code path to remove.
   std::map<SquareSizePx, SkBitmap> size_to_icon = ResizeIconsAndGenerateMissing(
-      square_icons, sizes_to_generate, icon_letter,
+      square_icons, SizesToGenerate(), icon_letter,
       &web_app_info->generated_icon_color);
 
   for (std::pair<const SquareSizePx, SkBitmap>& item : size_to_icon) {

chrome/browser/web_applications/components/web_app_install_utils.h

@@ -58,8 +58,7 @@ using IconsMap = std::map<GURL, std::vector<SkBitmap>>;
 // other devices and could potentially create a never ending sync cycle. If
 // |is_for_sync| is true then icon links won't be changed.
 void FilterAndResizeIconsGenerateMissing(WebApplicationInfo* web_app_info,
-                                         const IconsMap* icons_map,
-                                         bool is_for_sync);
+                                         const IconsMap* icons_map);
 
 // Record an app banner added to homescreen event to ensure banners are not
 // shown for this app.

chrome/browser/web_applications/manifest_update_task.cc

@@ -185,8 +185,7 @@ void ManifestUpdateTask::OnAllIconsRead(
 
   DCHECK(web_application_info_.has_value());
   FilterAndResizeIconsGenerateMissing(&web_application_info_.value(),
-                                      &downloaded_icons_map,
-                                      /*is_for_sync=*/false);
+                                      &downloaded_icons_map);
 
   // TODO: compare in a BEST_EFFORT blocking PostTaskAndReply.
   if (IsUpdateNeededForIconContents(disk_icon_bitmaps)) {

chrome/browser/web_applications/web_app_install_task.cc

@@ -194,8 +194,7 @@ void WebAppInstallTask::InstallWebAppFromInfo(
   CheckInstallPreconditions();
 
   FilterAndResizeIconsGenerateMissing(web_application_info.get(),
-                                      /*icons_map*/ nullptr,
-                                      /*is_for_sync*/ false);
+                                      /*icons_map*/ nullptr);
 
   install_source_ = install_source;
   background_installation_ = true;
@@ -627,9 +626,7 @@ void WebAppInstallTask::OnIconsRetrieved(
   DCHECK(web_app_info);
 
   // Installing from sync should not change icon links.
-  FilterAndResizeIconsGenerateMissing(
-      web_app_info.get(), &icons_map,
-      /*is_for_sync=*/install_source_ == WebappInstallSource::SYNC);
+  FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map);
 
   InstallFinalizer::FinalizeOptions options;
   options.install_source = install_source_;
@@ -653,8 +650,7 @@ void WebAppInstallTask::OnIconsRetrievedShowDialog(
   // The old BookmarkApp Sync System uses |WebAppInstallTask::OnIconsRetrieved|.
   // The new WebApp USS System has no sync wars and it doesn't need to preserve
   // icons. |is_for_sync| is always false for USS.
-  FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map,
-                                      /*is_for_sync=*/false);
+  FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map);
 
   if (background_installation_) {
     DCHECK(!dialog_callback_);
@@ -679,8 +675,7 @@ void WebAppInstallTask::OnIconsRetrievedFinalizeUpdate(
   DCHECK(web_app_info);
 
   // TODO(crbug.com/926083): Abort update if icons fail to download.
-  FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map,
-                                      /*is_for_sync=*/false);
+  FilterAndResizeIconsGenerateMissing(web_app_info.get(), &icons_map);
 
   install_finalizer_->FinalizeUpdate(
       *web_app_info, base::BindOnce(&WebAppInstallTask::OnInstallFinalized,

chrome/test/data/web_apps/favicon_only.html

+<!DOCTYPE html>
+<html>
+<head>
+  <title>Favicon only</title>
+  <link rel="icon" href="basic-48.png">
+</head>
+<body>
+  <h1>Favicon only</h1>
+</body>
+</html>