Fix GDI leak in NativeThemeWin::PaintIndirect

ui/native_theme/native_theme_win.cc

 // Copyright (c) 2012 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/native_theme/native_theme_win.h"
 
 #include <windows.h>
 #include <stddef.h>
 #include <uxtheme.h>
 #include <vsstyle.h>
 #include <vssym32.h>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #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"
 #include "skia/ext/skia_utils_win.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkColorPriv.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 #include "third_party/skia/include/core/SkShader.h"
@@ skipping 76 matching lines @@
   double ratio = fmod(animated_seconds, interval) / interval;
   return static_cast<int>(animation_width * ratio) - object_width;
 }
 
 RECT InsetRect(const RECT* rect, int size) {
   gfx::Rect result(*rect);
   result.Inset(size, 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 {
 
 bool NativeThemeWin::IsThemingActive() const {
   return is_theme_active_ && is_theme_active_();
 }
 
 bool NativeThemeWin::IsUsingHighContrastTheme() const {
   if (is_using_high_contrast_valid_)
@@ skipping 533 matching lines @@
                                    HDC destination_hdc,
                                    Part part,
                                    State state,
                                    const gfx::Rect& rect,
                                    const ExtraParams& extra) const {
   // TODO(asvitkine): This path is pretty inefficient - for each paint operation
   //                  it creates a new offscreen bitmap Skia canvas. This can
   //                  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).
   sk_sp<SkSurface> offscreen_surface =
       skia::MapPlatformSurface(offscreen_hdc.Get());
   if (!offscreen_surface)
     return;
 
   SkCanvas* offscreen_canvas = offscreen_surface->getCanvas();
   DCHECK(offscreen_canvas);
@@ skipping 1387 matching lines @@
     break;
   case LAST:
     NOTREACHED();
     break;
   }
   theme_handles_[theme_name] = handle;
   return handle;
 }
 
 }  // namespace ui