Reland "Use default icons for PE files lacking their own."

This is a reland of 676d0cb1.

TBR=fdoray@chromium.org
Bug: 1032250

Original change's description:
> Use default icons for PE files lacking their own.
>
> PrivateExtractIcon (underlying the icon loading service) may fail to
> supply an icon for PE files that do not have one embedded and would
> display as a blank area when downloaded. This change fetches the
> default for the file extension prior to fetching the file's embedded
> icon. It is difficult to do this only on failure as the ownership of
> IconLoader means it has already been deleted when we learn about the
> failure. It should be relatively cheap to fetch the default icons as
> these will be cached by the OS, and this occurs in the main process.
>
> Tests: Added LoadDefaultIcon in icon_loader_browser_test
> Bug: 1032250
> Change-Id: Ifad1c9f013d0e4fe87aaec5ec0cdeae08d06799e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2242371
> Commit-Queue: Alex Gough <ajgo@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#779542}

Change-Id: Ie7f5e57dbbfb1ebf5d9fc588e15b1a2687212bce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2254823Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781108}

chrome/browser/icon_loader_browsertest.cc

@@ -89,4 +89,19 @@ IN_PROC_BROWSER_TEST_F(IconLoaderBrowserTest, LoadExeIcon) {
   runner.Run();
   EXPECT_TRUE(test_loader.load_succeeded());
 }
+
+const base::FilePath::CharType kNotExistingExeFile[] =
+    FILE_PATH_LITERAL("unlikely-to-exist-file.exe");
+
+IN_PROC_BROWSER_TEST_F(IconLoaderBrowserTest, LoadDefaultExeIcon) {
+  base::RunLoop runner;
+
+  TestIconLoader test_loader(runner.QuitClosure());
+
+  test_loader.TryLoadIcon(base::FilePath(kNotExistingExeFile),
+                          IconLoader::NORMAL);
+
+  runner.Run();
+  EXPECT_TRUE(test_loader.load_succeeded());
+}
 #endif  // OS_WIN

chrome/browser/icon_loader_win.cc

@@ -28,10 +28,13 @@ class IconLoaderHelper {
   static void ExecuteLoadIcon(
       base::FilePath filename,
       chrome::mojom::IconSize size,
+      gfx::Image default_icon,
       scoped_refptr<base::SingleThreadTaskRunner> target_task_runner,
       IconLoader::IconLoadedCallback icon_loaded_callback);
 
-  IconLoaderHelper(base::FilePath filename, chrome::mojom::IconSize size);
+  IconLoaderHelper(base::FilePath filename,
+                   chrome::mojom::IconSize size,
+                   gfx::Image default_icon);
 
  private:
   void StartReadIconRequest();
@@ -52,6 +55,7 @@ class IconLoaderHelper {
   chrome::mojom::IconSize size_;
   // This callback owns the object until work is done.
   IconLoaderHelperCallback finally_;
+  gfx::Image default_icon_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -61,10 +65,12 @@ class IconLoaderHelper {
 void IconLoaderHelper::ExecuteLoadIcon(
     base::FilePath filename,
     chrome::mojom::IconSize size,
+    gfx::Image default_icon,
     scoped_refptr<base::SingleThreadTaskRunner> target_task_runner,
     IconLoader::IconLoadedCallback icon_loaded_callback) {
   // Self-deleting helper manages service lifetime.
-  auto helper = std::make_unique<IconLoaderHelper>(filename, size);
+  auto helper = std::make_unique<IconLoaderHelper>(filename, size,
+                                                   std::move(default_icon));
   auto* helper_raw = helper.get();
   // This callback owns the helper and extinguishes itself once work is done.
   auto finally_callback = base::BindOnce(
@@ -83,8 +89,9 @@ void IconLoaderHelper::ExecuteLoadIcon(
 }
 
 IconLoaderHelper::IconLoaderHelper(base::FilePath filename,
-                                   chrome::mojom::IconSize size)
-    : filename_(filename), size_(size) {
+                                   chrome::mojom::IconSize size,
+                                   gfx::Image default_icon)
+    : filename_(filename), size_(size), default_icon_(std::move(default_icon)) {
   remote_read_icon_ = LaunchIconReaderInstance();
   remote_read_icon_.set_disconnect_handler(base::BindOnce(
       &IconLoaderHelper::OnConnectionError, base::Unretained(this)));
@@ -102,16 +109,55 @@ void IconLoaderHelper::OnConnectionError() {
   if (finally_.is_null())
     return;
 
-  gfx::Image image;
-  std::move(finally_).Run(std::move(image), filename_.value());
+  std::move(finally_).Run(std::move(default_icon_), filename_.value());
 }
 
 void IconLoaderHelper::OnReadIconExecuted(const gfx::ImageSkia& icon,
                                           const base::string16& group) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  gfx::Image image(icon);
-  std::move(finally_).Run(std::move(image), group);
+  if (icon.isNull()) {
+    std::move(finally_).Run(std::move(default_icon_), group);
+  } else {
+    gfx::Image image(icon);
+    std::move(finally_).Run(std::move(image), group);
+  }
+}
+
+// Must be called in a COM context. |group| should be a file extension.
+gfx::Image GetIconForFileExtension(base::string16 group,
+                                   IconLoader::IconSize icon_size) {
+  int size = 0;
+  switch (icon_size) {
+    case IconLoader::SMALL:
+      size = SHGFI_SMALLICON;
+      break;
+    case IconLoader::NORMAL:
+      size = 0;
+      break;
+    case IconLoader::LARGE:
+      size = SHGFI_LARGEICON;
+      break;
+    default:
+      NOTREACHED();
+  }
+
+  gfx::Image image;
+
+  SHFILEINFO file_info = {0};
+  if (SHGetFileInfo(group.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info,
+                    sizeof(file_info),
+                    SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) {
+    const SkBitmap bitmap = IconUtil::CreateSkBitmapFromHICON(file_info.hIcon);
+    if (!bitmap.isNull()) {
+      gfx::ImageSkia image_skia(
+          gfx::ImageSkiaRep(bitmap, display::win::GetDPIScale()));
+      image_skia.MakeThreadSafe();
+      image = gfx::Image(image_skia);
+    }
+    DestroyIcon(file_info.hIcon);
+  }
+  return image;
 }
 
 }  // namespace
@@ -139,9 +185,8 @@ void IconLoader::ReadGroup() {
   group_ = GroupForFilepath(file_path_);
 
   if (group_ == file_path_.value()) {
-    // Calls a Windows API that parses the file so must be sandboxed. Call on
-    // target sequence as we don't need COM in this process.
-    target_task_runner_->PostTask(
+    // Calls a Windows API that parses the file so must be sandboxed.
+    GetReadIconTaskRunner()->PostTask(
         FROM_HERE,
         base::BindOnce(&IconLoader::ReadIconInSandbox, base::Unretained(this)));
   } else {
@@ -153,44 +198,21 @@ void IconLoader::ReadGroup() {
 }
 
 void IconLoader::ReadIcon() {
-  int size = 0;
-  switch (icon_size_) {
-    case IconLoader::SMALL:
-      size = SHGFI_SMALLICON;
-      break;
-    case IconLoader::NORMAL:
-      size = 0;
-      break;
-    case IconLoader::LARGE:
-      size = SHGFI_LARGEICON;
-      break;
-    default:
-      NOTREACHED();
-  }
-
-  gfx::Image image;
-
-  SHFILEINFO file_info = { 0 };
-  if (SHGetFileInfo(group_.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info,
-                    sizeof(file_info),
-                    SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) {
-    const SkBitmap bitmap = IconUtil::CreateSkBitmapFromHICON(file_info.hIcon);
-    if (!bitmap.isNull()) {
-      gfx::ImageSkia image_skia(
-          gfx::ImageSkiaRep(bitmap, display::win::GetDPIScale()));
-      image_skia.MakeThreadSafe();
-      image = gfx::Image(image_skia);
-    }
-    DestroyIcon(file_info.hIcon);
-  }
+  auto image = GetIconForFileExtension(group_, icon_size_);
 
   target_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(std::move(callback_), std::move(image), group_));
+
   delete this;
 }
 
 void IconLoader::ReadIconInSandbox() {
+  // Get default first as loader is deleted before ExecuteLoadIcon
+  // completes.
+  auto path = base::FilePath(group_);
+  auto default_icon = GetIconForFileExtension(path.Extension(), icon_size_);
+
   chrome::mojom::IconSize size = chrome::mojom::IconSize::kNormal;
   switch (icon_size_) {
     case IconLoader::SMALL:
@@ -206,8 +228,10 @@ void IconLoader::ReadIconInSandbox() {
       NOTREACHED();
   }
 
-  IconLoaderHelper::ExecuteLoadIcon(base::FilePath(group_), size,
-                                    target_task_runner_, std::move(callback_));
+  target_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&IconLoaderHelper::ExecuteLoadIcon,
+                                std::move(path), size, std::move(default_icon),
+                                target_task_runner_, std::move(callback_)));
 
   delete this;
 }