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}

chrome/browser/icon_loader_browsertest.cc

@@ -86,4 +86,20 @@ 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

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;
 }