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 631 matching lines @@
                                    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.

[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).
   sk_sp<SkSurface> offscreen_surface =
       skia::MapPlatformSurface(offscreen_hdc.Get());
   if (!offscreen_surface)
     return;
 
   SkCanvas* offscreen_canvas = offscreen_surface->getCanvas();
   DCHECK(offscreen_canvas);
@@ skipping 41 matching lines @@
     } else if (SkGetPackedA32(pixels[i]) == 0) {
       // Pixel was touched but has incorrect alpha of 0, make it fully opaque.
       pixels[i] = SkPackARGB32(0xFF,
                                SkGetPackedR32(pixels[i]),
                                SkGetPackedG32(pixels[i]),
                                SkGetPackedB32(pixels[i]));
     }
   }
 
   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,
                                          HDC hdc,
                                          int part_id,
                                          int state_id,
                                          RECT* rect,
                                          int ts,
                                          SIZE* size) const {
   HANDLE handle = GetThemeHandle(theme_name);
@@ skipping 1326 matching lines @@
     break;
   case LAST:
     NOTREACHED();
     break;
   }
   theme_handles_[theme_name] = handle;
   return handle;
 }
 
 }  // namespace ui