Abort thumbnail loading early for unsupported file types.

Previously, thumbnail loading would fail (intended) but took an amount
of time proportional to the size of the input file (unintended).

Bug: 1284757
Change-Id: I76d7da4e927478dce9a3fbe82b344dd0314bead2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3368312
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/main@{#956275}

WATCHLISTS

@@ -1128,6 +1128,7 @@
                   'ash/system/holding_space|'\
                   'chrome/browser/lacros/.*holding_space.*|'\
                   'chrome/browser/ui/ash/holding_space|'\
+                  'chrome/browser/ui/ash/thumbnail_loader.*|'\
                   'chromeos/crosapi/mojom/.*holding_space.*|'\
                   'tools/metrics/histograms/metadata/holding_space'
     },

ash/system/holding_space/holding_space_tray_icon_preview.cc

@@ -175,10 +175,15 @@ class HoldingSpaceTrayIconPreview::ImageLayerOwner : public ui::LayerOwner,
     if (image_skia_.isNull())
       return;
 
-    // Paint `image_skia_`.
+    // Copy `image_skia_` since retrieving a representation at the appropriate
+    // scale may result in a series of events in which `image_skia_` is deleted.
+    // Note that `gfx::ImageSkia`'s shared storage makes this a cheap copy.
+    gfx::ImageSkia image_skia(image_skia_);
+
+    // Paint `image_skia`.
     ui::PaintRecorder recorder(context, layer()->size());
     gfx::Canvas* canvas = recorder.canvas();
-    canvas->DrawImageInt(image_skia_, 0, 0);
+    canvas->DrawImageInt(image_skia, 0, 0);
   }
 
   void OnDeviceScaleFactorChanged(float old_device_scale_factor,

chrome/browser/ui/ash/OWNERS

@@ -12,7 +12,7 @@ per-file keyboard_*=yhanada@chromium.org
 per-file *wallpaper*=file://ash/wallpaper/OWNERS
 per-file *capture_mode*=file://ash/capture_mode/OWNERS
 per-file recording_service_browsertest.cc=file://ash/capture_mode/OWNERS
-per-file thumbnail_loader.*=file://chrome/browser/ui/ash/holding_space/OWNERS
+per-file thumbnail_loader*=file://chrome/browser/ui/ash/holding_space/OWNERS
 
 # Must be updated when a new OS settings section is added.
 per-file chrome_new_window_client*=file://chrome/browser/resources/settings/chromeos/OWNERS

chrome/browser/ui/ash/thumbnail_loader.cc

@@ -26,11 +26,13 @@
 #include "extensions/common/api/messaging/serialization_format.h"
 #include "extensions/common/extension.h"
 #include "net/base/data_url.h"
+#include "net/base/mime_util.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/data_decoder/public/cpp/data_decoder.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "storage/browser/file_system/file_system_context.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
+#include "third_party/re2/src/re2/re2.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/skia_conversions.h"
@@ -45,6 +47,132 @@ namespace {
 // loader extension.
 constexpr char kNativeMessageHostName[] = "com.google.ash_thumbnail_loader";
 
+// Returns whether the given `file_path` is supported by the `ThumbnailLoader`.
+bool IsSupported(const base::FilePath& file_path) {
+  constexpr std::array<std::pair<const char*, const char*>, 24>
+      kFileMatchPatterns = {{
+          // Document types ----------------------------------------------------
+          {
+              /*extension=*/"(?i)\\.pdf$",
+              /*mime_type=*/"(?i)application\\/pdf",
+          },
+          // Image types -------------------------------------------------------
+          {
+              /*extension=*/"(?i)\\.jpe?g$",
+              /*mime_type=*/"(?i)image\\/jpeg",
+          },
+          {
+              /*extension=*/"(?i)\\.bmp$",
+              /*mime_type=*/"(?i)image\\/bmp",
+          },
+          {
+              /*extension=*/"(?i)\\.gif$",
+              /*mime_type=*/"(?i)image\\/gif",
+          },
+          {
+              /*extension=*/"(?i)\\.ico$",
+              /*mime_type=*/"(?i)image\\/x\\-icon",
+          },
+          {
+              /*extension=*/"(?i)\\.png$",
+              /*mime_type=*/"(?i)image\\/png",
+          },
+          {
+              /*extension=*/"(?i)\\.webp$",
+              /*mime_type=*/"(?i)image\\/webp",
+          },
+          {
+              /*extension=*/"(?i)\\.tiff?$",
+              /*mime_type=*/"(?i)image\\/tiff",
+          },
+          {
+              /*extension=*/"(?i)\\.svg$",
+              /*mime_type=*/"(?i)image\\/svg\\+xml",
+          },
+          // Raw types ---------------------------------------------------------
+          {
+              /*extension=*/"(?i)\\.arw$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.cr2$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.dng$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.nef$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.nrw$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.orf$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.raf$",
+              /*mime_type=*/nullptr,
+          },
+          {
+              /*extension=*/"(?i)\\.rw2$",
+              /*mime_type=*/nullptr,
+          },
+          // Video types -------------------------------------------------------
+          {
+              /*extension=*/"(?i)\\.3gpp?$",
+              /*mime_type=*/"(?i)video\\/3gpp",
+          },
+          {
+              /*extension=*/"(?i)\\.avi$",
+              /*mime_type=*/"(?i)video\\/x\\-msvideo",
+          },
+          {
+              /*extension=*/"(?i)\\.mov$",
+              /*mime_type=*/"(?i)video\\/quicktime",
+          },
+          {
+              /*extension=*/"\\.mkv$",
+              /*mime_type=*/"video\\/x\\-matroska",
+          },
+          {
+              /*extension=*/"(?i)\\.m(p4|4v|pg|peg|pg4|peg4)$",
+              /*mime_type=*/"(?i)video\\/mp(4|eg)",
+          },
+          {
+              /*extension=*/"(?i)\\.og(m|v|x)$",
+              /*mime_type=*/"(?i)(application|video)\\/ogg",
+          },
+          {
+              /*extension=*/"(?i)\\.webm$",
+              /*mime_type=*/"(?i)video\\/webm",
+          },
+      }};
+
+  // First attempt to match based on `mime_type`.
+  std::string mime_type;
+  if (net::GetMimeTypeFromFile(file_path, &mime_type)) {
+    for (const auto& file_match_pattern : kFileMatchPatterns) {
+      if (file_match_pattern.second &&
+          re2::RE2::FullMatch(mime_type, file_match_pattern.second)) {
+        return true;
+      }
+    }
+  }
+
+  // Then attempt to match based on `file_path` extension.
+  for (const auto& file_match_pattern : kFileMatchPatterns) {
+    if (re2::RE2::FullMatch(file_path.Extension(), file_match_pattern.first))
+      return true;
+  }
+
+  return false;
+}
+
 using ThumbnailDataCallback = base::OnceCallback<void(const std::string& data)>;
 
 // Handles a parsed message sent from image loader extension in response to a
@@ -192,9 +320,9 @@ ThumbnailLoader::~ThumbnailLoader() {
 }
 
 ThumbnailLoader::ThumbnailRequest::ThumbnailRequest(
-    const base::FilePath& item_path,
+    const base::FilePath& file_path,
     const gfx::Size& size)
-    : item_path(item_path), size(size) {}
+    : file_path(file_path), size(size) {}
 
 ThumbnailLoader::ThumbnailRequest::~ThumbnailRequest() = default;
 
@@ -204,14 +332,14 @@ base::WeakPtr<ThumbnailLoader> ThumbnailLoader::GetWeakPtr() {
 
 void ThumbnailLoader::Load(const ThumbnailRequest& request,
                            ImageCallback callback) {
-  // Get the item's last modified time - this will be used for cache lookup in
+  // Get the file's last modified time - this will be used for cache lookup in
   // the image loader extension.
   GURL source_url = extensions::Extension::GetBaseURLFromExtensionId(
       file_manager::kImageLoaderExtensionId);
   file_manager::util::GetMetadataForPath(
       file_manager::util::GetFileSystemContextForSourceURL(profile_,
                                                            source_url),
-      request.item_path,
+      request.file_path,
       storage::FileSystemOperation::GET_METADATA_FIELD_IS_DIRECTORY |
           storage::FileSystemOperation::GET_METADATA_FIELD_LAST_MODIFIED,
       base::BindOnce(&ThumbnailLoader::LoadForFileWithMetadata,
@@ -237,9 +365,15 @@ void ThumbnailLoader::LoadForFileWithMetadata(
     return;
   }
 
+  // Short-circuit if unsupported.
+  if (!IsSupported(request.file_path)) {
+    std::move(callback).Run(/*bitmap=*/nullptr, base::File::FILE_ERROR_ABORT);
+    return;
+  }
+
   GURL thumbnail_url;
   if (!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
-          profile_, request.item_path,
+          profile_, request.file_path,
           extensions::Extension::GetBaseURLFromExtensionId(
               file_manager::kImageLoaderExtensionId),
           &thumbnail_url)) {

chrome/browser/ui/ash/thumbnail_loader.h

@@ -38,11 +38,11 @@ class ThumbnailLoader {
 
   // Thumbnail request data that will be forwarded to the image loader.
   struct ThumbnailRequest {
-    ThumbnailRequest(const base::FilePath& item_path, const gfx::Size& size);
+    ThumbnailRequest(const base::FilePath& file_path, const gfx::Size& size);
     ~ThumbnailRequest();
 
-    // The absolute item file path.
-    const base::FilePath item_path;
+    // The absolute file path.
+    const base::FilePath file_path;
 
     // The desired bitmap size.
     const gfx::Size size;

chrome/browser/ui/ash/thumbnail_loader_browsertest.cc

@@ -29,7 +29,7 @@ namespace {
 
 // References to paths set up in the test mount point during the browser test
 // setup.
-enum class TestPath { kNonExistent, kEmptyDir, kJpg, kBrokenJpg, kPng };
+enum class TestPath { kNonExistent, kEmptyDir, kJpg, kBrokenJpg, kPng, kBin };
 
 // Copies |bitmap| into |copy| and runs |callback|.
 void CopyBitmapAndRunClosure(base::OnceClosure callback,
@@ -135,6 +135,8 @@ class ThumbnailLoaderTest : public InProcessBrowserTest {
         return mount_point()->GetRootPath().AppendASCII("broken.jpg");
       case TestPath::kPng:
         return mount_point()->GetRootPath().AppendASCII("image.png");
+      case TestPath::kBin:
+        return mount_point()->GetRootPath().AppendASCII("random.bin");
     }
   }
 
@@ -151,6 +153,8 @@ class ThumbnailLoaderTest : public InProcessBrowserTest {
                                GetTestPath(TestPath::kJpg)));
     ASSERT_TRUE(base::CopyFile(GetTestDataFilePath("broken.jpg"),
                                GetTestPath(TestPath::kBrokenJpg)));
+    ASSERT_TRUE(base::CopyFile(GetTestDataFilePath("random.bin"),
+                               GetTestPath(TestPath::kBin)));
   }
 
   std::unique_ptr<ScopedExternalMountPoint> test_mount_point_;
@@ -158,7 +162,7 @@ class ThumbnailLoaderTest : public InProcessBrowserTest {
   std::unique_ptr<ash::ThumbnailLoader> thumbnail_loader_;
 };
 
-IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, LoadNonExistentItem) {
+IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, LoadNonExistentFile) {
   ash::ThumbnailLoader* loader = GetThumbnailLoader();
   ASSERT_TRUE(loader);
 
@@ -242,6 +246,24 @@ IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, LoadPng) {
   EXPECT_EQ(48, bitmap.height());
 }
 
+IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, LoadUnsupportedFiletype) {
+  ash::ThumbnailLoader* loader = GetThumbnailLoader();
+  ASSERT_TRUE(loader);
+
+  ash::ThumbnailLoader::ThumbnailRequest request(GetTestPath(TestPath::kBin),
+                                                 gfx::Size(48, 48));
+
+  base::RunLoop run_loop;
+  loader->Load(request,
+               base::BindLambdaForTesting(
+                   [&](const SkBitmap* bitmap, base::File::Error error) {
+                     EXPECT_FALSE(bitmap);
+                     EXPECT_EQ(error, base::File::FILE_ERROR_ABORT);
+                     run_loop.Quit();
+                   }));
+  run_loop.Run();
+}
+
 IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, RepeatedLoads) {
   ash::ThumbnailLoader* loader = GetThumbnailLoader();
   ASSERT_TRUE(loader);
@@ -282,7 +304,7 @@ IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, RepeatedLoads) {
   EXPECT_FALSE(gfx::test::AreBitmapsEqual(bitmap1, bitmap3));
 }
 
-IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, ConcurentLoads) {
+IN_PROC_BROWSER_TEST_F(ThumbnailLoaderTest, ConcurrentLoads) {
   ash::ThumbnailLoader* loader = GetThumbnailLoader();
   ASSERT_TRUE(loader);