Eliminate HICON leaks caused by creating icons from bitmap image.

ui/gfx/icon_util_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/icon_util.h"
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gfx_paths.h"
 #include "ui/gfx/icon_util_unittests_resource.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_family.h"
 
+using base::win::ScopedHICON;

[grt (UTC plus 2), 2015/11/10 16:44:43]

nit: can you move this into the public: section of IconUtilTest?

+
 namespace {
 
 static const char kSmallIconName[] = "icon_util/16_X_16_icon.ico";
 static const char kLargeIconName[] = "icon_util/128_X_128_icon.ico";
 static const char kTempIconFilename[] = "temp_test_icon.ico";
 
 }  // namespace
 
 class IconUtilTest : public testing::Test {
  public:
   void SetUp() override {
     gfx::RegisterPathProvider();
     ASSERT_TRUE(PathService::Get(gfx::DIR_TEST_DATA, &test_data_directory_));
     ASSERT_TRUE(temp_directory_.CreateUniqueTempDir());
   }
 
   static const int kSmallIconWidth = 16;
   static const int kSmallIconHeight = 16;
   static const int kLargeIconWidth = 128;
   static const int kLargeIconHeight = 128;
 
   // Given a file name for an .ico file and an image dimensions, this
   // function loads the icon and returns an HICON handle.
-  HICON LoadIconFromFile(const base::FilePath& filename,
-                         int width, int height) {
+  ScopedHICON LoadIconFromFile(const base::FilePath& filename,
+                               int width,
+                               int height) {
     HICON icon = static_cast<HICON>(LoadImage(NULL,
                                     filename.value().c_str(),
                                     IMAGE_ICON,
                                     width,
                                     height,
                                     LR_LOADTRANSPARENT | LR_LOADFROMFILE));
-    return icon;
+    return ScopedHICON(icon).Pass();

[grt (UTC plus 2), 2015/11/10 16:44:43]

can you remove .Pass() here?

   }
 
   SkBitmap CreateBlackSkBitmap(int width, int height) {
     SkBitmap bitmap;
     bitmap.allocN32Pixels(width, height);
     // Setting the pixels to transparent-black.
     memset(bitmap.getPixels(), 0, width * height * 4);
     return bitmap;
   }
 
@@ skipping 17 matching lines @@
 
   // Determine how many icons to expect, based on |max_icon_size|.
   int expected_num_icons = 0;
   for (size_t i = 0; i < IconUtil::kNumIconDimensions; ++i) {
     if (IconUtil::kIconDimensions[i] > max_icon_size)
       break;
     ++expected_num_icons;
   }
 
   // First, use the Windows API to load the icon, a basic validity test.
-  HICON icon = LoadIconFromFile(icon_filename, kSmallIconWidth,
-                                kSmallIconHeight);
-  EXPECT_NE(static_cast<HICON>(NULL), icon);
-  if (icon != NULL)
-    ::DestroyIcon(icon);
+  ScopedHICON icon =
+      LoadIconFromFile(icon_filename, kSmallIconWidth, kSmallIconHeight).Pass();
+  EXPECT_TRUE(icon.is_valid());

[grt (UTC plus 2), 2015/11/10 16:44:43]

  EXPECT_TRUE(LoadIconFromFile(icon_filename, kSmallIconWidth,
kSmallIconHeight).is_valid());

+  icon.reset();
 
   // Read the file completely into memory.
   std::string icon_data;
   ASSERT_TRUE(base::ReadFileToString(icon_filename, &icon_data));
   ASSERT_GE(icon_data.length(), sizeof(IconUtil::ICONDIR));
 
   // Ensure that it has exactly the expected number and sizes of icons, in the
   // expected order. This matches each entry of the loaded file's icon directory
   // with the corresponding element of kIconDimensions.
   // Also extracts the 256x256 entry as png_entry.
@@ skipping 33 matching lines @@
     EXPECT_EQ(256, bitmap.height());
   }
 }
 
 // The following test case makes sure IconUtil::SkBitmapFromHICON fails
 // gracefully when called with invalid input parameters.
 TEST_F(IconUtilTest, TestIconToBitmapInvalidParameters) {
   base::FilePath icon_filename =
       test_data_directory_.AppendASCII(kSmallIconName);
   gfx::Size icon_size(kSmallIconWidth, kSmallIconHeight);
-  HICON icon = LoadIconFromFile(icon_filename,
-                                icon_size.width(),
-                                icon_size.height());
-  ASSERT_TRUE(icon != NULL);
+  ScopedHICON icon =

[grt (UTC plus 2), 2015/11/10 16:44:43]

can you use:
  ScopedHICON icon(LoadIconFromFile(icon_filename, icon_size.width(),
icon_size.height()));

+      LoadIconFromFile(icon_filename, icon_size.width(), icon_size.height())
+          .Pass();
+  ASSERT_TRUE(icon.is_valid());
 
   // Invalid size parameter.
   gfx::Size invalid_icon_size(kSmallIconHeight, 0);
-  EXPECT_EQ(IconUtil::CreateSkBitmapFromHICON(icon, invalid_icon_size),
+  EXPECT_EQ(IconUtil::CreateSkBitmapFromHICON(icon.get(), invalid_icon_size),
             static_cast<SkBitmap*>(NULL));
 
   // Invalid icon.
   EXPECT_EQ(IconUtil::CreateSkBitmapFromHICON(NULL, icon_size),
             static_cast<SkBitmap*>(NULL));
 
   // The following code should succeed.
   scoped_ptr<SkBitmap> bitmap;
-  bitmap.reset(IconUtil::CreateSkBitmapFromHICON(icon, icon_size));
+  bitmap.reset(IconUtil::CreateSkBitmapFromHICON(icon.get(), icon_size));
   EXPECT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
-  ::DestroyIcon(icon);
 }
 
 // The following test case makes sure IconUtil::CreateHICONFromSkBitmap fails
 // gracefully when called with invalid input parameters.
 TEST_F(IconUtilTest, TestBitmapToIconInvalidParameters) {
-  HICON icon = NULL;
+  ScopedHICON icon;
   scoped_ptr<SkBitmap> bitmap;
 
   // Wrong bitmap format.
   bitmap.reset(new SkBitmap);
   ASSERT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
   bitmap->setInfo(SkImageInfo::MakeA8(kSmallIconWidth, kSmallIconHeight));
-  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap);
-  EXPECT_EQ(icon, static_cast<HICON>(NULL));
+  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap).Pass();
+  EXPECT_FALSE(icon.is_valid());
 
   // Invalid bitmap size.
   bitmap.reset(new SkBitmap);
   ASSERT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
   bitmap->setInfo(SkImageInfo::MakeN32Premul(0, 0));
-  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap);
-  EXPECT_EQ(icon, static_cast<HICON>(NULL));
+  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap).Pass();
+  EXPECT_FALSE(icon.is_valid());
 
   // Valid bitmap configuration but no pixels allocated.
   bitmap.reset(new SkBitmap);
   ASSERT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
   bitmap->setInfo(SkImageInfo::MakeN32Premul(kSmallIconWidth,
                                              kSmallIconHeight));
-  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap);
-  EXPECT_TRUE(icon == NULL);
+  icon = IconUtil::CreateHICONFromSkBitmap(*bitmap).Pass();
+  EXPECT_FALSE(icon.is_valid());
 }
 
 // The following test case makes sure IconUtil::CreateIconFileFromImageFamily
 // fails gracefully when called with invalid input parameters.
 TEST_F(IconUtilTest, TestCreateIconFileInvalidParameters) {
   scoped_ptr<SkBitmap> bitmap;
   gfx::ImageFamily image_family;
   base::FilePath valid_icon_filename = temp_directory_.path().AppendASCII(
       kTempIconFilename);
   base::FilePath invalid_icon_filename = temp_directory_.path().AppendASCII(
@@ skipping 62 matching lines @@
   EXPECT_FALSE(base::PathExists(icon_filename));
 }
 
 // This test case makes sure that when we load an icon from disk and convert
 // the HICON into a bitmap, the bitmap has the expected format and dimensions.
 TEST_F(IconUtilTest, TestCreateSkBitmapFromHICON) {
   scoped_ptr<SkBitmap> bitmap;
   base::FilePath small_icon_filename = test_data_directory_.AppendASCII(
       kSmallIconName);
   gfx::Size small_icon_size(kSmallIconWidth, kSmallIconHeight);
-  HICON small_icon = LoadIconFromFile(small_icon_filename,
-                                      small_icon_size.width(),
-                                      small_icon_size.height());
-  ASSERT_NE(small_icon, static_cast<HICON>(NULL));
-  bitmap.reset(IconUtil::CreateSkBitmapFromHICON(small_icon, small_icon_size));
+  ScopedHICON small_icon =
+      LoadIconFromFile(small_icon_filename, small_icon_size.width(),
+                       small_icon_size.height())
+          .Pass();
+  ASSERT_TRUE(small_icon.is_valid());
+  bitmap.reset(
+      IconUtil::CreateSkBitmapFromHICON(small_icon.get(), small_icon_size));
   ASSERT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
   EXPECT_EQ(bitmap->width(), small_icon_size.width());
   EXPECT_EQ(bitmap->height(), small_icon_size.height());
   EXPECT_EQ(bitmap->colorType(), kN32_SkColorType);
-  ::DestroyIcon(small_icon);
 
   base::FilePath large_icon_filename = test_data_directory_.AppendASCII(
       kLargeIconName);
   gfx::Size large_icon_size(kLargeIconWidth, kLargeIconHeight);
-  HICON large_icon = LoadIconFromFile(large_icon_filename,
-                                      large_icon_size.width(),
-                                      large_icon_size.height());
-  ASSERT_NE(large_icon, static_cast<HICON>(NULL));
-  bitmap.reset(IconUtil::CreateSkBitmapFromHICON(large_icon, large_icon_size));
+  ScopedHICON large_icon =
+      LoadIconFromFile(large_icon_filename, large_icon_size.width(),
+                       large_icon_size.height())
+          .Pass();
+  ASSERT_TRUE(large_icon.is_valid());
+  bitmap.reset(
+      IconUtil::CreateSkBitmapFromHICON(large_icon.get(), large_icon_size));
   ASSERT_NE(bitmap.get(), static_cast<SkBitmap*>(NULL));
   EXPECT_EQ(bitmap->width(), large_icon_size.width());
   EXPECT_EQ(bitmap->height(), large_icon_size.height());
   EXPECT_EQ(bitmap->colorType(), kN32_SkColorType);
-  ::DestroyIcon(large_icon);
 }
 
 // This test case makes sure that when an HICON is created from an SkBitmap,
 // the returned handle is valid and refers to an icon with the expected
 // dimensions color depth etc.
 TEST_F(IconUtilTest, TestBasicCreateHICONFromSkBitmap) {
   SkBitmap bitmap = CreateBlackSkBitmap(kSmallIconWidth, kSmallIconHeight);
-  HICON icon = IconUtil::CreateHICONFromSkBitmap(bitmap);
-  EXPECT_NE(icon, static_cast<HICON>(NULL));
+  ScopedHICON icon = IconUtil::CreateHICONFromSkBitmap(bitmap).Pass();

[grt (UTC plus 2), 2015/11/10 16:44:43]

  ScopedHICON icon(IconUtil::CreateHICONFromSkBitmap(bitmap));

+  EXPECT_TRUE(icon.is_valid());
   ICONINFO icon_info;
-  ASSERT_TRUE(::GetIconInfo(icon, &icon_info));
+  ASSERT_TRUE(GetIconInfo(icon.get(), &icon_info));
   EXPECT_TRUE(icon_info.fIcon);
 
   // Now that have the icon information, we should obtain the specification of
   // the icon's bitmap and make sure it matches the specification of the
   // SkBitmap we started with.
   //
   // The bitmap handle contained in the icon information is a handle to a
   // compatible bitmap so we need to call ::GetDIBits() in order to retrieve
   // the bitmap's header information.
   BITMAPINFO bitmap_info;
   ::ZeroMemory(&bitmap_info, sizeof(BITMAPINFO));
   bitmap_info.bmiHeader.biSize = sizeof(BITMAPINFO);
   HDC hdc = ::GetDC(NULL);
   int result = ::GetDIBits(hdc,
                            icon_info.hbmColor,
                            0,
                            kSmallIconWidth,
                            NULL,
                            &bitmap_info,
                            DIB_RGB_COLORS);
   ASSERT_GT(result, 0);
   EXPECT_EQ(bitmap_info.bmiHeader.biWidth, kSmallIconWidth);
   EXPECT_EQ(bitmap_info.bmiHeader.biHeight, kSmallIconHeight);
   EXPECT_EQ(bitmap_info.bmiHeader.biPlanes, 1);
   EXPECT_EQ(bitmap_info.bmiHeader.biBitCount, 32);
   ::ReleaseDC(NULL, hdc);
-  ::DestroyIcon(icon);
 }
 
 // This test case makes sure that CreateIconFileFromImageFamily creates a
 // valid .ico file given an ImageFamily, and appropriately creates all icon
 // sizes from the given input.
 TEST_F(IconUtilTest, TestCreateIconFileFromImageFamily) {
   gfx::ImageFamily image_family;
   base::FilePath icon_filename =
       temp_directory_.path().AppendASCII(kTempIconFilename);
 
@@ skipping 75 matching lines @@
 }
 
 // This tests that kNumIconDimensionsUpToMediumSize has the correct value.
 TEST_F(IconUtilTest, TestNumIconDimensionsUpToMediumSize) {
   ASSERT_LE(IconUtil::kNumIconDimensionsUpToMediumSize,
             IconUtil::kNumIconDimensions);
   EXPECT_EQ(IconUtil::kMediumIconSize,
             IconUtil::kIconDimensions[
                 IconUtil::kNumIconDimensionsUpToMediumSize - 1]);
 }