Fix GDI leak in NativeThemeWin::PaintIndirect

ui/native_theme/native_theme_win.cc

Index: ui/native_theme/native_theme_win.cc
diff --git a/ui/native_theme/native_theme_win.cc b/ui/native_theme/native_theme_win.cc
index 6dc57e9dbc0aa3a53ed107c84153bcb225112d2a..a942ef2ec50dce71010524a33773c590a4af1685 100644
--- a/ui/native_theme/native_theme_win.cc
+++ b/ui/native_theme/native_theme_win.cc
@@ -15,6 +15,7 @@
 #include "base/win/scoped_gdi_object.h"
 #include "base/win/scoped_hdc.h"
 #include "base/win/scoped_select_object.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "skia/ext/bitmap_platform_device.h"
 #include "skia/ext/platform_canvas.h"
@@ -667,11 +668,30 @@ void NativeThemeWin::PaintIndirect(SkCanvas* destination_canvas,
   // Create an offscreen canvas that is backed by an HDC.
   // This can fail if we don't have access to GDI or if lower-level Windows
   // calls fail, possibly due to GDI handle exhaustion.

[Peter Kasting, 2016/09/24 00:52:34]

Nit: This comment is now a bit redundant with some of the stuff below.  Dunno if
maybe it should be removed, or broken into pieces or something.

[Rafał Chłodnicki, 2016/09/26 10:12:09]

Done.

-  base::win::ScopedCreateDC offscreen_hdc(
-      skia::CreateOffscreenSurface(rect.width(), rect.height()));
+
+  // If this process doesn't have access to GDI, we'd need to use shared memory
+  // segment instead but that is not supported right now.
+  if (!base::win::IsUser32AndGdi32Available())
+    return;
+
+  base::win::ScopedCreateDC offscreen_hdc(CreateCompatibleDC(nullptr));
   if (!offscreen_hdc.IsValid())
     return;
 
+  skia::InitializeDC(offscreen_hdc.Get());
+  HRGN clip = CreateRectRgn(0, 0, rect.width(), rect.height());
+  if ((SelectClipRgn(offscreen_hdc.Get(), clip) == ERROR) ||
+      !DeleteObject(clip)) {
+    return;
+  }
+
+  base::win::ScopedBitmap hdc_bitmap(skia::CreateHBitmap(

[Peter Kasting, 2016/09/24 00:52:34]

I would avoid using ScopedBitmap directly here.  We still have the issue that
returns before the bottom of the function (e.g. line 701) can free things in the
wrong order.

I suggest writing a custom scoping object in an anonymous namespace in this
file, probably which takes an HDC in its constructor (so you can construct this
in place of the current ScopedCreateDC) and then has a setter for the bitmap.

[Rafał Chłodnicki, 2016/09/26 10:12:09]

Done.

+      rect.width(), rect.height(), false, nullptr, nullptr));
+  if (!hdc_bitmap.is_valid())
+    return;
+
+  SelectObject(offscreen_hdc.Get(), hdc_bitmap.get());
+
   // Will be NULL if lower-level Windows calls fail, or if the backing
   // allocated is 0 pixels in size (which should never happen according to
   // Windows documentation).
@@ -733,6 +753,9 @@ void NativeThemeWin::PaintIndirect(SkCanvas* destination_canvas,
   }
 
   destination_canvas->drawBitmap(offscreen_bitmap, rect.x(), rect.y());
+  // Delete hdc before bitmap.

[Peter Kasting, 2016/09/24 00:52:34]

Nit: Blank line above this.

Nit: I suggest adding "..., since objects should not be deleted while selected
into a DC."

[Rafał Chłodnicki, 2016/09/26 10:12:09]

Done.

+  offscreen_hdc.Close();
+  hdc_bitmap.reset();

[Peter Kasting, 2016/09/24 00:52:34]

Nit: This statement unnecessary

[Rafał Chłodnicki, 2016/09/26 10:12:09]

Done.

 }
 
 HRESULT NativeThemeWin::GetThemePartSize(ThemeName theme_name,