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..e9a2f707375bdffcf7a51df3e35f8152b5050d62 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"
@@ -111,6 +112,40 @@ RECT InsetRect(const RECT* rect, int size) {
   return result.ToRECT();
 }
 
+// Custom scoped object for storing DC and a bitmap that was selected into it,
+// and making sure that they are deleted in the right order.
+class ScopedCreateDCWithBitmap {
+ public:
+  explicit ScopedCreateDCWithBitmap(base::win::ScopedCreateDC::Handle hdc)
+      : dc_(hdc) {}
+
+  ~ScopedCreateDCWithBitmap() {
+    // Delete DC before the bitmap, since objects should not be deleted while
+    // selected into a DC.
+    dc_.Close();
+  }
+
+  bool IsValid() const { return dc_.IsValid(); }
+
+  base::win::ScopedCreateDC::Handle Get() const { return dc_.Get(); }
+
+  // Selects |handle| to bitmap into DC. Returns false if handle is not valid.
+  bool SelectBitmap(base::win::ScopedBitmap::element_type handle) {
+    bitmap_.reset(handle);
+    if (!bitmap_.is_valid())
+      return false;
+
+    SelectObject(dc_.Get(), bitmap_.get());
+    return true;
+  }
+
+ private:
+  base::win::ScopedCreateDC dc_;
+  base::win::ScopedBitmap bitmap_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedCreateDCWithBitmap);
+};
+
 }  // namespace
 
 namespace ui {
@@ -664,14 +699,27 @@ void NativeThemeWin::PaintIndirect(SkCanvas* destination_canvas,
   //                  be sped up by doing it only once per part/state and
   //                  keeping a cache of the resulting bitmaps.
 
-  // 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.
-  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;
+
+  ScopedCreateDCWithBitmap 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)) {

[Peter Kasting, 2016/09/26 20:58:33]

Nit: {} not necessary (2 places)

[Rafał Chłodnicki, 2016/09/27 07:00:48]

I much prefer when there are braces when condition takes more than one line. I
believe it was even part of the style guide at some point (can't find that rule
there now).

[Peter Kasting, 2016/09/27 07:05:40]

It was never part of the style guide -- in fact we actually wrote a rule
explicitly clarifying that this was not required style at one point, because
several people had misread the Google style guide and thought it was.  We took
that out later as part of the effort to slim down the Chromium style guide.

Both styles are legal in Chromium code.  The important thing is to be
consistent.  In the case of this file, the only other existing case of a
multi-line conditional with single-line body is line 258, which does not use
braces.

+    return;
+  }
+
+  if (!offscreen_hdc.SelectBitmap(skia::CreateHBitmap(
+          rect.width(), rect.height(), false, nullptr, nullptr))) {
+    return;
+  }
+
   // 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).