Prevent making invalid SkImageReps

Ensure that SkImageRep is always constructed with a valid bitmap.
Instead of constructing an invalid SkImageRep then throwing it away when
initializing an SkImage, we add an SkImage::CreateFromBitmap() that will
just return a null SkImage directly if the bitmap is null/uninitialized.

ImageSkia was creating a null ImageSkiaRep when searching for a
representation in order to force use of the nearest one, but it also
uses a boolean value to do the same, so stop making a null ImageSkiaRep.

Then disallow null/empty ImageSkiaRep and also disallow their transport
over mojo.

Remove a few tests that were creating invalid ImageSkiaReps to test that
they work - as they are no longer valid.

Have ImageSkia constructed from ImageSkiaOperations check for the
ImageSkiaRep that they get from their inner (non-null!) ImageSkia, as
that can still return a null ImageSkiaRep. In that case, the resulting
ImageSkiaRep must also be null. And we can do that without trying to
perform operations on the (invalid, null) bitmap inside the null
ImageSkiaRep.

R=sky@chromium.org

Bug: 1155258
Change-Id: Id7953743ca31804fa3d987677bb8c4962919fd35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572896
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837373}

PRESUBMIT.py

@@ -1787,7 +1787,9 @@ def _GetMessageForMatchingType(input_api, affected_file, line_number, line,
   """
   result = []
 
-  if line.endswith(" nocheck"):
+  if input_api.re.search(r"^ *//", line):  # Ignore comments about banned types.
+    return result
+  if line.endswith(" nocheck"):  # A // nocheck comment will bypass this error.
     return result
 
   matched = False

PRESUBMIT_test.py

@@ -2320,6 +2320,10 @@ def testBannedCppFunctions(self):
                ['using std::string;']),
       MockFile('some/cpp/problematic/file2.cc',
                ['set_owned_by_client()']),
+      MockFile('some/cpp/nocheck/file.cc',
+               ['using namespace std;  // nocheck']),
+      MockFile('some/cpp/comment/file.cc',
+               ['  // A comment about `using namespace std;`']),
     ]
 
     results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
@@ -2332,6 +2336,11 @@ def testBannedCppFunctions(self):
     self.assertTrue('some/cpp/ok/file.cc' not in results[1].message)
     self.assertTrue('some/cpp/problematic/file2.cc' in results[0].message)
 
+    self.assertFalse('some/cpp/nocheck/file.cc' in results[0].message)
+    self.assertFalse('some/cpp/nocheck/file.cc' in results[1].message)
+    self.assertFalse('some/cpp/comment/file.cc' in results[0].message)
+    self.assertFalse('some/cpp/comment/file.cc' in results[1].message)
+
   def testBannedBlinkDowncastHelpers(self):
     input_api = MockInputApi()
     input_api.files = [

ash/accelerators/debug_commands.cc

@@ -48,7 +48,7 @@ gfx::ImageSkia CreateWallpaperImage(SkColor fill, SkColor rect) {
   paint.setBlendMode(SkBlendMode::kSrcOver);
   canvas.drawRoundRect(gfx::RectToSkRect(gfx::Rect(image_size)), 100.f, 100.f,
                        paint);
-  return gfx::ImageSkia(gfx::ImageSkiaRep(std::move(bitmap), 1.f));
+  return gfx::ImageSkia::CreateFromBitmap(std::move(bitmap), 1.f);
 }
 
 void HandleToggleWallpaperMode() {

ash/app_list/views/ghost_image_view.cc

@@ -188,7 +188,7 @@ gfx::ImageSkia GhostImageView::GetIconOutline(
                   bitmap.pixmap().height() + kGhostImagePadding * 2),
         rep.scale(), false /* is_opaque */);
     padded_canvas.DrawImageInt(
-        gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, rep.scale())),
+        gfx::ImageSkia::CreateFromBitmap(bitmap, rep.scale()),
         kGhostImagePadding, kGhostImagePadding);
     bitmap = padded_canvas.GetBitmap();
 

ash/display/cursor_window_controller.cc

@@ -366,8 +366,8 @@ void CursorWindowController::UpdateCursorImage() {
 
   const gfx::ImageSkiaRep& image_rep = resized.GetRepresentation(cursor_scale);
   delegate_->SetCursorImage(resized.size(),
-                            gfx::ImageSkia(gfx::ImageSkiaRep(
-                                GetAdjustedBitmap(image_rep), cursor_scale)));
+                            gfx::ImageSkia::CreateFromBitmap(
+                                GetAdjustedBitmap(image_rep), cursor_scale));
   // TODO(danakj): Should this be rounded? Or kept as a floating point?
   hot_point_ = gfx::ToFlooredPoint(
       gfx::ConvertPointToDips(hot_point_in_physical_pixels, cursor_scale));

ash/drag_drop/drag_image_view.cc

@@ -127,7 +127,8 @@ void DragImageView::OnPaint(gfx::Canvas* canvas) {
     SkBitmap scaled = skia::ImageOperations::Resize(
         image_rep.GetBitmap(), skia::ImageOperations::RESIZE_LANCZOS3,
         drag_image_size_pixels.width(), drag_image_size_pixels.height());
-    gfx::ImageSkia image_skia(gfx::ImageSkiaRep(scaled, device_scale));
+    gfx::ImageSkia image_skia =
+        gfx::ImageSkia::CreateFromBitmap(scaled, device_scale);
     canvas->DrawImageInt(image_skia, 0, 0);
   }
 

ash/system/holding_space/holding_space_drag_util.cc

@@ -325,7 +325,7 @@ class DragImageView : public views::View {
                           /*clear_color=*/SK_ColorTRANSPARENT, is_pixel_canvas)
             .context(),
         size()));
-    return gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, scale));
+    return gfx::ImageSkia::CreateFromBitmap(bitmap, scale);
   }
 
   // Returns the drag offset to use when rendering this view as a drag image.

chrome/browser/apps/app_service/app_icon_factory.cc

@@ -226,7 +226,7 @@ SkBitmap DecompressToSkBitmap(const unsigned char* data, size_t size) {
 }
 
 gfx::ImageSkia SkBitmapToImageSkia(SkBitmap bitmap, float icon_scale) {
-  return gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, icon_scale));
+  return gfx::ImageSkia::CreateFromBitmap(bitmap, icon_scale);
 }
 
 // Returns a callback that converts a gfx::Image to an ImageSkia.

chrome/browser/apps/app_service/app_icon_factory_unittest.cc

@@ -195,7 +195,7 @@ class AppIconFactoryTest : public testing::Test {
     ASSERT_TRUE(gfx::PNGCodec::Decode(compressed_data.data(),
                                       compressed_data.size(), &decoded));
 
-    output_image_skia = gfx::ImageSkia(gfx::ImageSkiaRep(decoded, scale));
+    output_image_skia = gfx::ImageSkia::CreateFromBitmap(decoded, scale);
 
 #if BUILDFLAG(IS_CHROMEOS_ASH)
     if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {

chrome/browser/chromeos/crosapi/message_center_ash_unittest.cc

@@ -196,7 +196,8 @@ TEST_F(MessageCenterAshTest, HighDpiImage) {
 
   // Create a high DPI image.
   SkBitmap bitmap = gfx::test::CreateBitmap(2, 4);
-  gfx::ImageSkia high_dpi_image_skia(gfx::ImageSkiaRep(bitmap, 2.0f));
+  gfx::ImageSkia high_dpi_image_skia =
+      gfx::ImageSkia::CreateFromBitmap(bitmap, 2.0f);
   mojo_notification->image = high_dpi_image_skia;
 
   // Display the notification.

chrome/browser/chromeos/login/users/avatar/user_image_manager_test_util.cc

@@ -66,7 +66,7 @@ gfx::ImageSkia ImageLoader::Load() {
 }
 
 void ImageLoader::OnImageDecoded(const SkBitmap& decoded_image) {
-  decoded_image_ = gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, 1.0f));
+  decoded_image_ = gfx::ImageSkia::CreateFromBitmap(decoded_image, 1.0f);
   run_loop_.Quit();
 }
 

chrome/browser/chromeos/remote_apps/remote_apps_manager_browsertest.cc

@@ -126,7 +126,7 @@ gfx::ImageSkia CreateTestIcon(int size, SkColor color) {
   SkBitmap bitmap;
   bitmap.allocN32Pixels(size, size);
   bitmap.eraseColor(color);
-  return gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, 1.0f));
+  return gfx::ImageSkia::CreateFromBitmap(bitmap, 1.0f);
 }
 
 void CheckIconsEqual(const gfx::ImageSkia& expected,

chrome/browser/extensions/api/notifications/notifications_api.cc

@@ -185,7 +185,7 @@ bool NotificationBitmapToGfxImage(
 
   // TODO(dewittj): Handle HiDPI images with more than one scale factor
   // representation.
-  gfx::ImageSkia skia(gfx::ImageSkiaRep(bitmap, 1.0f));
+  gfx::ImageSkia skia = gfx::ImageSkia::CreateFromBitmap(bitmap, 1.0f);
   *return_image = gfx::Image(skia);
   return true;
 }

chrome/browser/search/ntp_icon_source.cc

@@ -164,7 +164,7 @@ const ParsedNtpIconPath ParseNtpIconPath(const std::string& path) {
 void DrawFavicon(const SkBitmap& bitmap, gfx::Canvas* canvas, int size) {
   int x_origin = (size - bitmap.width()) / 2;
   int y_origin = (size - bitmap.height()) / 2;
-  canvas->DrawImageInt(gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, /*scale=*/1.0)),
+  canvas->DrawImageInt(gfx::ImageSkia::CreateFromBitmap(bitmap, /*scale=*/1.f),
                        x_origin, y_origin);
 }
 

chrome/browser/ui/app_list/icon_standardizer_unittest.cc

@@ -87,7 +87,7 @@ TEST_F(CreateStandardIconTest, CircularIconToStandardIcon) {
 
   // Get the standard icon version of the red circle icon.
   gfx::ImageSkia generated_standard_icon = app_list::CreateStandardIconImage(
-      gfx::ImageSkia(gfx::ImageSkiaRep(circle_icon_bitmap, 2.0f)));
+      gfx::ImageSkia::CreateFromBitmap(circle_icon_bitmap, 2.0f));
 
   // Scale the bitmap to fit the size of a standardized circle icon.
   SkBitmap scaled_bitmap = skia::ImageOperations::Resize(
@@ -123,7 +123,7 @@ TEST_F(CreateStandardIconTest, StandardCircularIconToStandardIcon) {
 
   // Get the standard icon version of the red circle icon.
   gfx::ImageSkia standard_icon = app_list::CreateStandardIconImage(
-      gfx::ImageSkia(gfx::ImageSkiaRep(circle_icon_bitmap, 2.0f)));
+      gfx::ImageSkia::CreateFromBitmap(circle_icon_bitmap, 2.0f));
 
   EXPECT_TRUE(AreBitmapsEqual(*standard_icon.bitmap(), circle_icon_bitmap));
 }

chrome/browser/ui/app_list/md_icon_normalizer.cc

@@ -219,8 +219,8 @@ void MaybeResizeAndPad(const gfx::Size& required_size,
   }
 
   // Add padding.
-  gfx::Canvas canvas(required_size, 1, /* transparent */ false);
-  canvas.DrawImageInt(gfx::ImageSkia(gfx::ImageSkiaRep(resized, 1)),
+  gfx::Canvas canvas(required_size, 1, /*transparent=*/false);
+  canvas.DrawImageInt(gfx::ImageSkia::CreateFromBitmap(resized, 1),
                       padding.width(), padding.height());
   *bitmap_out = canvas.GetBitmap();
   return;

chrome/browser/ui/app_list/search/common/url_icon_source.cc

@@ -100,7 +100,7 @@ void UrlIconSource::OnSimpleLoaderComplete(
 
 void UrlIconSource::OnImageDecoded(const SkBitmap& decoded_image) {
   const float scale = decoded_image.width() / icon_size_;
-  icon_ = gfx::ImageSkia(gfx::ImageSkiaRep(decoded_image, scale));
+  icon_ = gfx::ImageSkia::CreateFromBitmap(decoded_image, scale);
   icon_loaded_callback_.Run();
 }
 

chrome/browser/ui/views/elevation_icon_setter.cc

@@ -77,7 +77,7 @@ void ElevationIconSetter::SetButtonIcon(base::OnceClosure callback,
 #endif
     button_->SetImage(
         views::Button::STATE_NORMAL,
-        gfx::ImageSkia(gfx::ImageSkiaRep(icon, device_scale_factor)));
+        gfx::ImageSkia::CreateFromBitmap(icon, device_scale_factor));
     button_->SizeToPreferredSize();
     if (button_->parent())
       button_->parent()->Layout();

chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.cc

@@ -264,7 +264,7 @@ void MediaNotificationContainerImplView::CreateDragImageWidget() {
                         true /* is_pixel_canvas */)
           .context(),
       GetPreferredSize()));
-  gfx::ImageSkia image(gfx::ImageSkiaRep(bitmap, 1.f));
+  gfx::ImageSkia image = gfx::ImageSkia::CreateFromBitmap(bitmap, 1.f);
   image_view->SetImage(image);
 
   drag_image_widget_->Show();

chrome/browser/ui/views/qrcode_generator/qrcode_generator_bubble.cc

@@ -71,7 +71,7 @@ gfx::ImageSkia GetPlaceholderImageSkia(const SkColor color) {
   bitmap.allocN32Pixels(kQRImageSizePx, kQRImageSizePx);
   bitmap.eraseARGB(0xFF, 0xFF, 0xFF, 0xFF);
   bitmap.eraseColor(color);
-  return gfx::ImageSkia(gfx::ImageSkiaRep(bitmap, 1.0f));
+  return gfx::ImageSkia::CreateFromBitmap(bitmap, 1.0f);
 }
 
 // Adds a new small vertical padding row to the current bottom of |layout|.

chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.cc

@@ -30,7 +30,7 @@ gfx::ImageSkia GetBestImageRep(const gfx::ImageSkia& image) {
   }
   // All status icon implementations want the image in pixel coordinates, so use
   // a scale factor of 1.
-  return gfx::ImageSkia(gfx::ImageSkiaRep(best_rep, 1.0f));
+  return gfx::ImageSkia::CreateFromBitmap(best_rep, 1.0f);
 }
 
 }  // namespace

chrome/browser/ui/views/tab_icon_view.cc

@@ -36,8 +36,8 @@ gfx::ImageSkia CreateDefaultFavicon() {
 #if defined(OS_WIN)
   // The default window icon is the application icon, not the default favicon.
   HICON app_icon = GetAppIcon();
-  icon = gfx::ImageSkia(gfx::ImageSkiaRep(
-      IconUtil::CreateSkBitmapFromHICON(app_icon, gfx::Size(16, 16)), 1.0f));
+  icon = gfx::ImageSkia::CreateFromBitmap(
+      IconUtil::CreateSkBitmapFromHICON(app_icon, gfx::Size(16, 16)), 1.0f);
   DestroyIcon(app_icon);
 #else
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();

chrome/test/base/perf/performance_test.cc

@@ -63,7 +63,8 @@ void CreateAndSetWallpaper() {
                         /*isOpaque=*/true);
   SkCanvas canvas(bitmap);
   canvas.drawColor(SK_ColorGREEN);
-  gfx::ImageSkia image(gfx::ImageSkiaRep(std::move(bitmap), 1.f));
+  gfx::ImageSkia image =
+      gfx::ImageSkia::CreateFromBitmap(std::move(bitmap), 1.f);
 
   base::RunLoop run_loop;
   TestWallpaperObserver observer(run_loop.QuitClosure());

components/exo/drag_drop_operation.cc

@@ -276,7 +276,8 @@ void DragDropOperation::OnDragIconCaptured(const SkBitmap& icon_bitmap) {
   DCHECK(icon_);
 
   float scale_factor = origin_->get()->window()->layer()->device_scale_factor();
-  gfx::ImageSkia icon_skia(gfx::ImageSkiaRep(icon_bitmap, scale_factor));
+  gfx::ImageSkia icon_skia =
+      gfx::ImageSkia::CreateFromBitmap(icon_bitmap, scale_factor);
   gfx::Vector2d icon_offset = -icon_->get()->GetBufferOffset();
 
   if (os_exchange_data_) {

components/favicon_base/select_favicon_frames.cc

@@ -240,7 +240,7 @@ gfx::ImageSkia CreateFaviconImageSkia(
 
   if (desired_size_in_dip == 0) {
     size_t index = results[0].index;
-    return gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1.0f));
+    return gfx::ImageSkia::CreateFromBitmap(bitmaps[index], 1.0f);
   }
 
   auto image_source = std::make_unique<FaviconImageSource>();

content/browser/renderer_host/render_widget_host_impl.cc

@@ -2615,7 +2615,7 @@ void RenderWidgetHostImpl::StartDragging(
   }
 
   float scale = GetScaleFactorForView(GetView());
-  gfx::ImageSkia image(gfx::ImageSkiaRep(bitmap, scale));
+  gfx::ImageSkia image = gfx::ImageSkia::CreateFromBitmap(bitmap, scale);
   view->StartDragging(filtered_data, drag_operations_mask, image,
                       bitmap_offset_in_dip, *event_info, this);
 }

ui/gfx/icon_util.cc

@@ -98,7 +98,7 @@ bool BuildResizedImageFamily(const gfx::ImageFamily& image_family,
 // |bitmaps| must be an empty vector, and not NULL.
 // Returns true on success, false on failure. This fails if any image in
 // |image_family| is not a 32-bit ARGB image, or is otherwise invalid.
-bool ConvertImageFamilyToBitmaps(
+void ConvertImageFamilyToBitmaps(
     const gfx::ImageFamily& image_family,
     std::vector<SkBitmap>* bitmaps,
     scoped_refptr<base::RefCountedMemory>* png_bytes) {
@@ -117,8 +117,7 @@ bool ConvertImageFamilyToBitmaps(
 
     SkBitmap bitmap = image.AsBitmap();
     CHECK_EQ(bitmap.colorType(), kN32_SkColorType);
-    if (bitmap.isNull())
-      return false;
+    CHECK(!bitmap.isNull());
 
     // Special case: Icons exactly 256x256 are stored in PNG format.
     if (image.Width() == IconUtil::kLargeIconSize &&
@@ -128,8 +127,6 @@ bool ConvertImageFamilyToBitmaps(
       bitmaps->push_back(bitmap);
     }
   }
-
-  return true;
 }
 
 }  // namespace
@@ -461,8 +458,7 @@ bool IconUtil::CreateIconFileFromImageFamily(
 
   std::vector<SkBitmap> bitmaps;
   scoped_refptr<base::RefCountedMemory> png_bytes;
-  if (!ConvertImageFamilyToBitmaps(resized_image_family, &bitmaps, &png_bytes))
-    return false;
+  ConvertImageFamilyToBitmaps(resized_image_family, &bitmaps, &png_bytes);
 
   // Guaranteed true because BuildResizedImageFamily will provide at least one
   // image < 256x256.

ui/gfx/icon_util_unittest.cc

@@ -207,14 +207,8 @@ TEST_F(IconUtilTest, TestCreateIconFileInvalidParameters) {
   base::FilePath invalid_icon_filename =
       temp_directory_.GetPath().AppendASCII("<>?.ico");
 
-  // Null bitmap.
-  SkBitmap bitmap;
-  image_family.Add(gfx::Image::CreateFrom1xBitmap(bitmap));
-  EXPECT_FALSE(IconUtil::CreateIconFileFromImageFamily(image_family,
-                                                       valid_icon_filename));
-  EXPECT_FALSE(base::PathExists(valid_icon_filename));
-
   // Invalid file name.
+  SkBitmap bitmap;
   image_family.clear();
   bitmap.allocN32Pixels(1, 1);
   image_family.Add(gfx::Image::CreateFrom1xBitmap(bitmap));