Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1406403007

Cr-Commit-Position: refs/heads/master@{#366055}

base/win/scoped_gdi_object.h

@@ -7,64 +7,32 @@
 
 #include <windows.h>
 
-#include "base/basictypes.h"
-#include "base/logging.h"
+#include "base/scoped_generic.h"
 
 namespace base {
 namespace win {
 
-// Like ScopedHandle but for GDI objects.
-template<class T>
-class ScopedGDIObject {
- public:
-  ScopedGDIObject() : object_(NULL) {}
-  explicit ScopedGDIObject(T object) : object_(object) {}
-
-  ~ScopedGDIObject() {
-    Close();
-  }
-
-  T Get() {
-    return object_;
-  }
-
-  void Set(T object) {
-    if (object_ && object != object_)
-      Close();
-    object_ = object;
-  }
-
-  ScopedGDIObject& operator=(T object) {
-    Set(object);
-    return *this;
-  }
-
-  T release() {
-    T object = object_;
-    object_ = NULL;
-    return object;
-  }
+namespace internal {
 
-  operator T() { return object_; }
-
- private:
-  void Close() {
-    if (object_)
-      DeleteObject(object_);
-  }
-
-  T object_;
-  DISALLOW_COPY_AND_ASSIGN(ScopedGDIObject);
+template <class T>
+struct ScopedGDIObjectTraits {
+  static T InvalidValue() { return nullptr; }
+  static void Free(T object) { DeleteObject(object); }
 };
 
 // An explicit specialization for HICON because we have to call DestroyIcon()
 // instead of DeleteObject() for HICON.
-template<>
-void inline ScopedGDIObject<HICON>::Close() {
-  if (object_)
-    DestroyIcon(object_);
+template <>
+void inline ScopedGDIObjectTraits<HICON>::Free(HICON icon) {
+  DestroyIcon(icon);
 }
 
+}  // namespace internal
+
+// Like ScopedHandle but for GDI objects.
+template <class T>
+using ScopedGDIObject = ScopedGeneric<T, internal::ScopedGDIObjectTraits<T>>;
+
 // Typedefs for some common use cases.
 typedef ScopedGDIObject<HBITMAP> ScopedBitmap;
 typedef ScopedGDIObject<HRGN> ScopedRegion;

chrome/browser/ui/views/frame/glass_browser_frame_view.cc

@@ -65,13 +65,16 @@ const int kNewTabCaptionMaximizedSpacing = 16;
 
 // Converts the |image| to a Windows icon and returns the corresponding HICON
 // handle. |image| is resized to desired |width| and |height| if needed.
-HICON CreateHICONFromSkBitmapSizedTo(const gfx::ImageSkia& image,
-                                     int width,
-                                     int height) {
-  if (width == image.width() && height == image.height())
-    return IconUtil::CreateHICONFromSkBitmap(*image.bitmap());
-  return IconUtil::CreateHICONFromSkBitmap(skia::ImageOperations::Resize(
-      *image.bitmap(), skia::ImageOperations::RESIZE_BEST, width, height));
+base::win::ScopedHICON CreateHICONFromSkBitmapSizedTo(
+    const gfx::ImageSkia& image,
+    int width,
+    int height) {
+  return IconUtil::CreateHICONFromSkBitmap(
+      width == image.width() && height == image.height()
+          ? *image.bitmap()
+          : skia::ImageOperations::Resize(*image.bitmap(),
+                                          skia::ImageOperations::RESIZE_BEST,
+                                          width, height));
 }
 
 }  // namespace
@@ -616,17 +619,31 @@ void GlassBrowserFrameView::StopThrobber() {
   if (throbber_running_) {
     throbber_running_ = false;
 
+    base::win::ScopedHICON previous_small_icon;
+    base::win::ScopedHICON previous_big_icon;
     HICON small_icon = nullptr;
     HICON big_icon = nullptr;
 
     // Check if hosted BrowserView has a window icon to use.
     if (browser_view()->ShouldShowWindowIcon()) {
       gfx::ImageSkia icon = browser_view()->GetWindowIcon();
       if (!icon.isNull()) {
-        small_icon = CreateHICONFromSkBitmapSizedTo(
-            icon, GetSystemMetrics(SM_CXSMICON), GetSystemMetrics(SM_CYSMICON));
-        big_icon = CreateHICONFromSkBitmapSizedTo(
-            icon, GetSystemMetrics(SM_CXICON), GetSystemMetrics(SM_CYICON));
+        // Keep previous icons alive as long as they are referenced by the HWND.
+        previous_small_icon = small_window_icon_.Pass();
+        previous_big_icon = big_window_icon_.Pass();
+
+        // Take responsibility for eventually destroying the created icons.
+        small_window_icon_ =
+            CreateHICONFromSkBitmapSizedTo(icon, GetSystemMetrics(SM_CXSMICON),
+                                           GetSystemMetrics(SM_CYSMICON))
+                .Pass();
+        big_window_icon_ =
+            CreateHICONFromSkBitmapSizedTo(icon, GetSystemMetrics(SM_CXICON),
+                                           GetSystemMetrics(SM_CYICON))
+                .Pass();
+
+        small_icon = small_window_icon_.get();
+        big_icon = big_window_icon_.get();
       }
     }
 
@@ -643,21 +660,13 @@ void GlassBrowserFrameView::StopThrobber() {
     // This will reset the icon which we set in the throbber code.
     // WM_SETICON with null icon restores the icon for title bar but not
     // for taskbar. See http://crbug.com/29996
-    HICON previous_small_icon = reinterpret_cast<HICON>(
-        SendMessage(views::HWNDForWidget(frame()), WM_SETICON,
-                    static_cast<WPARAM>(ICON_SMALL),
-                    reinterpret_cast<LPARAM>(small_icon)));
-
-    HICON previous_big_icon = reinterpret_cast<HICON>(
-        SendMessage(views::HWNDForWidget(frame()), WM_SETICON,
-                    static_cast<WPARAM>(ICON_BIG),
-                    reinterpret_cast<LPARAM>(big_icon)));
-
-    if (previous_small_icon)
-      ::DestroyIcon(previous_small_icon);
+    SendMessage(views::HWNDForWidget(frame()), WM_SETICON,
+                static_cast<WPARAM>(ICON_SMALL),
+                reinterpret_cast<LPARAM>(small_icon));
 
-    if (previous_big_icon)
-      ::DestroyIcon(previous_big_icon);
+    SendMessage(views::HWNDForWidget(frame()), WM_SETICON,
+                static_cast<WPARAM>(ICON_BIG),
+                reinterpret_cast<LPARAM>(big_icon));
   }
 }
 

chrome/browser/ui/views/frame/glass_browser_frame_view.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_VIEWS_FRAME_GLASS_BROWSER_FRAME_VIEW_H_
 
 #include "base/compiler_specific.h"
+#include "base/win/scoped_gdi_object.h"
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/window/non_client_view.h"
@@ -106,6 +107,12 @@ class GlassBrowserFrameView : public BrowserNonClientFrameView,
   // The bounds of the ClientView.
   gfx::Rect client_view_bounds_;
 
+  // The small icon created from the bitmap image of the window icon.
+  base::win::ScopedHICON small_window_icon_;
+
+  // The big icon created from the bitmap image of the window icon.
+  base::win::ScopedHICON big_window_icon_;
+
   // Whether or not the window throbber is currently animating.
   bool throbber_running_;
 

chrome/browser/ui/views/frame/taskbar_decorator_win.cc

@@ -59,11 +59,11 @@ void SetOverlayIcon(HWND hwnd, scoped_ptr<SkBitmap> bitmap) {
     SkCanvas offscreen_canvas(offscreen_bitmap);
     offscreen_canvas.clear(SK_ColorTRANSPARENT);
     offscreen_canvas.drawBitmap(sk_icon, 0, kOverlayIconSize - resized_height);
-    icon.Set(IconUtil::CreateHICONFromSkBitmap(offscreen_bitmap));
-    if (!icon.Get())
+    icon = IconUtil::CreateHICONFromSkBitmap(offscreen_bitmap).Pass();
+    if (!icon.is_valid())
       return;
   }
-  taskbar->SetOverlayIcon(hwnd, icon, L"");
+  taskbar->SetOverlayIcon(hwnd, icon.get(), L"");
 }
 
 }  // namespace

chrome/browser/ui/views/panels/panel_frame_view.cc

@@ -357,18 +357,19 @@ void PanelFrameView::SetWindowCornerStyle(panel::CornerStyle corner_style) {
   // window region if the region really differs.
   HWND native_window = views::HWNDForWidget(panel_view_->window());
   base::win::ScopedRegion current_region(::CreateRectRgn(0, 0, 0, 0));
-  ::GetWindowRgn(native_window, current_region);
+  ::GetWindowRgn(native_window, current_region.get());
 
   gfx::Path window_mask;
   GetWindowMask(size(), &window_mask);
   base::win::ScopedRegion new_region;
   if (!window_mask.isEmpty())
-    new_region.Set(gfx::CreateHRGNFromSkPath(window_mask));
+    new_region.reset(gfx::CreateHRGNFromSkPath(window_mask));
 
   const bool has_current_region = current_region != NULL;
   const bool has_new_region = new_region != NULL;
   if (has_current_region != has_new_region ||
-      (has_current_region && !::EqualRgn(current_region, new_region))) {
+      (has_current_region &&
+       !::EqualRgn(current_region.get(), new_region.get()))) {
     // SetWindowRgn takes ownership of the new_region.
     ::SetWindowRgn(native_window, new_region.release(), TRUE);
   }

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

@@ -82,7 +82,7 @@ void StatusIconWin::ResetIcon() {
   InitIconData(&icon_data);
   icon_data.uFlags = NIF_MESSAGE;
   icon_data.uCallbackMessage = message_id_;
-  icon_data.hIcon = icon_.Get();
+  icon_data.hIcon = icon_.get();
   // If we have an image, then set the NIF_ICON flag, which tells
   // Shell_NotifyIcon() to set the image for the status icon it creates.
   if (icon_data.hIcon)
@@ -98,8 +98,8 @@ void StatusIconWin::SetImage(const gfx::ImageSkia& image) {
   NOTIFYICONDATA icon_data;
   InitIconData(&icon_data);
   icon_data.uFlags = NIF_ICON;
-  icon_.Set(IconUtil::CreateHICONFromSkBitmap(*image.bitmap()));
-  icon_data.hIcon = icon_.Get();
+  icon_ = IconUtil::CreateHICONFromSkBitmap(*image.bitmap()).Pass();
+  icon_data.hIcon = icon_.get();
   BOOL result = Shell_NotifyIcon(NIM_MODIFY, &icon_data);
   if (!result)
     LOG(WARNING) << "Error setting status tray icon image";
@@ -131,12 +131,12 @@ void StatusIconWin::DisplayBalloon(
 
   base::win::Version win_version = base::win::GetVersion();
   if (!icon.isNull() && win_version != base::win::VERSION_PRE_XP) {
-    balloon_icon_.Set(IconUtil::CreateHICONFromSkBitmap(*icon.bitmap()));
+    balloon_icon_ = IconUtil::CreateHICONFromSkBitmap(*icon.bitmap()).Pass();
     if (win_version >= base::win::VERSION_VISTA) {
-      icon_data.hBalloonIcon = balloon_icon_.Get();
+      icon_data.hBalloonIcon = balloon_icon_.get();
       icon_data.dwInfoFlags = NIIF_USER | NIIF_LARGE_ICON;
     } else {
-      icon_data.hIcon = balloon_icon_.Get();
+      icon_data.hIcon = balloon_icon_.get();
       icon_data.uFlags |= NIF_ICON;
       icon_data.dwInfoFlags = NIIF_USER;
     }

chrome/browser/ui/views/tabs/window_finder_win.cc

@@ -109,16 +109,16 @@ class TopMostFinder : public BaseWindowFinder {
     }
 
     // hwnd is at the point. Make sure the point is within the windows region.
-    if (GetWindowRgn(hwnd, tmp_region_.Get()) == ERROR) {
+    if (GetWindowRgn(hwnd, tmp_region_.get()) == ERROR) {
       // There's no region on the window and the window contains the point. Stop
       // iterating.
       return true;
     }
 
     // The region is relative to the window's rect.
-    BOOL is_point_in_region = PtInRegion(tmp_region_.Get(),
-        screen_loc_.x() - r.left, screen_loc_.y() - r.top);
-    tmp_region_ = CreateRectRgn(0, 0, 0, 0);
+    BOOL is_point_in_region = PtInRegion(
+        tmp_region_.get(), screen_loc_.x() - r.left, screen_loc_.y() - r.top);
+    tmp_region_.reset(CreateRectRgn(0, 0, 0, 0));
     // Stop iterating if the region contains the point.
     return !!is_point_in_region;
   }

content/browser/renderer_host/pepper/pepper_truetype_font_win.cc

@@ -76,25 +76,26 @@ int32_t PepperTrueTypeFontWin::Initialize(
   }
   // TODO(bbudge) support widths (extended, condensed).
 
-  font_.Set(CreateFont(0 /* height */,
-                       0 /* width */,
-                       0 /* escapement */,
-                       0 /* orientation */,
-                       desc->weight,  // our weight enum matches Windows.
-                       (desc->style & PP_TRUETYPEFONTSTYLE_ITALIC) ? 1 : 0,
-                       0 /* underline */,
-                       0 /* strikeout */,
-                       desc->charset,       // our charset enum matches Windows.
-                       OUT_OUTLINE_PRECIS,  // truetype and other outline fonts
-                       CLIP_DEFAULT_PRECIS,
-                       DEFAULT_QUALITY,
-                       pitch_and_family,
-                       base::UTF8ToUTF16(desc->family).c_str()));
-  if (!font_.Get())
+  font_.reset(CreateFont(
+      0 /* height */,
+      0 /* width */,
+      0 /* escapement */,
+      0 /* orientation */,
+      desc->weight,  // our weight enum matches Windows.
+      (desc->style & PP_TRUETYPEFONTSTYLE_ITALIC) ? 1 : 0,
+      0 /* underline */,
+      0 /* strikeout */,
+      desc->charset,       // our charset enum matches Windows.
+      OUT_OUTLINE_PRECIS,  // truetype and other outline fonts
+      CLIP_DEFAULT_PRECIS,
+      DEFAULT_QUALITY,
+      pitch_and_family,
+      base::UTF8ToUTF16(desc->family).c_str()));
+  if (!font_.is_valid())
     return PP_ERROR_FAILED;
 
   LOGFONT font_desc;
-  if (!::GetObject(font_.Get(), sizeof(LOGFONT), &font_desc))
+  if (!::GetObject(font_.get(), sizeof(LOGFONT), &font_desc))
     return PP_ERROR_FAILED;
 
   switch (font_desc.lfPitchAndFamily & 0xF0) {  // Top 4 bits are family.
@@ -125,7 +126,7 @@ int32_t PepperTrueTypeFontWin::Initialize(
   // doesn't fill in the name field of the LOGFONT structure.
   base::win::ScopedCreateDC hdc(::CreateCompatibleDC(NULL));
   if (hdc.IsValid()) {
-    base::win::ScopedSelectObject select_object(hdc.Get(), font_.Get());
+    base::win::ScopedSelectObject select_object(hdc.Get(), font_.get());
     WCHAR name[LF_FACESIZE];
     GetTextFace(hdc.Get(), LF_FACESIZE, name);
     desc->family = base::UTF16ToUTF8(name);
@@ -135,14 +136,14 @@ int32_t PepperTrueTypeFontWin::Initialize(
 }
 
 int32_t PepperTrueTypeFontWin::GetTableTags(std::vector<uint32_t>* tags) {
-  if (!font_.Get())
+  if (!font_.is_valid())
     return PP_ERROR_FAILED;
 
   base::win::ScopedCreateDC hdc(::CreateCompatibleDC(NULL));
   if (!hdc.IsValid())
     return PP_ERROR_FAILED;
 
-  base::win::ScopedSelectObject select_object(hdc.Get(), font_.Get());
+  base::win::ScopedSelectObject select_object(hdc.Get(), font_.get());
 
   // Get the whole font header.
   static const DWORD kFontHeaderSize = 12;
@@ -181,14 +182,14 @@ int32_t PepperTrueTypeFontWin::GetTable(uint32_t table_tag,
                                         int32_t offset,
                                         int32_t max_data_length,
                                         std::string* data) {
-  if (!font_.Get())
+  if (!font_.is_valid())
     return PP_ERROR_FAILED;
 
   base::win::ScopedCreateDC hdc(::CreateCompatibleDC(NULL));
   if (!hdc.IsValid())
     return PP_ERROR_FAILED;
 
-  base::win::ScopedSelectObject select_object(hdc.Get(), font_.Get());
+  base::win::ScopedSelectObject select_object(hdc.Get(), font_.get());
 
   // Tags are byte swapped on Windows.
   table_tag = base::ByteSwap(table_tag);

content/common/cursors/webcursor_aurawin.cc

@@ -30,7 +30,8 @@ ui::PlatformCursor WebCursor::GetPlatformCursor() {
       custom_size,
       hotspot,
       !custom_data.empty() ? &custom_data[0] : NULL,
-      custom_data.size());
+      custom_data.size())
+      .release();
   return custom_cursor_;
 }
 

printing/emf_win.cc

@@ -104,12 +104,12 @@ class RasterBitmap {
     gfx::Rect bitmap_rect(raster_size);
     gfx::CreateBitmapHeader(raster_size.width(), raster_size.height(),
                             &header_.bmiHeader);
-    bitmap_.Set(::CreateDIBSection(context_.Get(), &header_, DIB_RGB_COLORS,
+    bitmap_.reset(CreateDIBSection(context_.Get(), &header_, DIB_RGB_COLORS,
                                    &bits, NULL, 0));
-    if (!bitmap_)
+    if (!bitmap_.is_valid())
       NOTREACHED() << "Raster bitmap creation for printing failed";
 
-    saved_object_ = ::SelectObject(context_.Get(), bitmap_);
+    saved_object_ = ::SelectObject(context_.Get(), bitmap_.get());
     RECT rect = bitmap_rect.ToRECT();
     ::FillRect(context_.Get(), &rect,
                static_cast<HBRUSH>(::GetStockObject(WHITE_BRUSH)));