Fix GDI leak in NativeThemeWin::PaintIndirect

Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was removed and replaced with a custom scoped object
at call site as it was used only in one place and making it safe to use for
others would be a bit tricky as bitmap needs to be deleted before HDC and there
is no easy access to the bitmap after utility function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
Committed: https://crrev.com/4014935dc5f26ad7517ca8427425995bd67d6c85
Cr-Commit-Position: refs/heads/master@{#421142}

[tomhudson, 2016-09-23 16:29:10]

lgtm

Thanks for finding and fixing this.

[Peter Kasting, 2016-09-23 18:45:00]

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc#...
skia/ext/skia_utils_win.cc:201: DCHECK(bitmap);
Nit: Blank line below this

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc#...
skia/ext/skia_utils_win.cc:213: if (SelectClipRgn(scoped_hdc.Get(), clip) ==
ERROR || !DeleteObject(clip))
Nit: Removing the second set of parnes was good, I wouldn't have removed the
first though.

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.h
File skia/ext/skia_utils_win.h (right):

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.h#n...
skia/ext/skia_utils_win.h:82: SK_API HDC
CreateOffscreenSurface(base::win::ScopedBitmap* bitmap,
Nit: Outparam should go last.

https://codereview.chromium.org/2365903002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_win.cc:667: // Bitmap needs to be deleted together
with DC (before or after).
According to docs I found on the web, an attempt to free the HBITMAP while it's
still selected into a valid DC will fail.  So the destruction order must be DC
first, then bitmap (unless we were to SelectObject() a new HBITMAP into the DC
first).

This seems to contradict the comment here.  The code here happens to get this in
the right order, but I'm worried about future misuse.

Maybe we should nuke CreateOffscreenSurface() and do it inline here.  That way
we can be very clear in the comments about the declaration order of different
things, and we won't risk others getting this wrong.

You'll want to rebase anyway to pick up the changes from
https://codereview.chromium.org/2355423002/ .

[Rafał Chłodnicki, 2016-09-23 19:01:20]

On 2016/09/23 18:45:00, Peter Kasting wrote:
>
https://codereview.chromium.org/2365903002/diff/1/ui/native_theme/native_them...
> ui/native_theme/native_theme_win.cc:667: // Bitmap needs to be deleted
together
> with DC (before or after).
> According to docs I found on the web, an attempt to free the HBITMAP while
it's
> still selected into a valid DC will fail.  So the destruction order must be DC
> first, then bitmap (unless we were to SelectObject() a new HBITMAP into the DC
> first).
> 
> This seems to contradict the comment here.  The code here happens to get this
in
> the right order, but I'm worried about future misuse.
> 
> Maybe we should nuke CreateOffscreenSurface() and do it inline here.  That way
> we can be very clear in the comments about the declaration order of different
> things, and we won't risk others getting this wrong.

I was searching the way for quite some time to figure that one out too. I found
this microsoft example which deletes bitmap before HDC:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd183402(v=vs.85).aspx
And this stack overflow answer:
http://stackoverflow.com/a/15760414/986604
which says that it might not be correct to delete bitmap first but it's handled
correctly anyway.

So yeah, apart from confusing MS example, I guess the correct behavior is to
delete bitmap after HDC.

As a side note... it took me a while to understand why it is leaking. It wasn't
super clear that selected object has to be deleted -- that deleting HDC won't
take care of it itself. HDC after all seems to have some object selected by
default (calling SelectObject returns it), and you don't have to delete this one
apparently. So it's all quite unclear IMO.

But anyways, as it's the only user of that utility function, I can inline it. Or
I can just explicitly call release in right order.

[Peter Kasting, 2016-09-23 19:09:10]

On 2016/09/23 19:01:20, Rafał Chłodnicki wrote:
> On 2016/09/23 18:45:00, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2365903002/diff/1/ui/native_theme/native_them...
> > ui/native_theme/native_theme_win.cc:667: // Bitmap needs to be deleted
> together
> > with DC (before or after).
> > According to docs I found on the web, an attempt to free the HBITMAP while
> it's
> > still selected into a valid DC will fail.  So the destruction order must be
DC
> > first, then bitmap (unless we were to SelectObject() a new HBITMAP into the
DC
> > first).
> > 
> > This seems to contradict the comment here.  The code here happens to get
this
> in
> > the right order, but I'm worried about future misuse.
> > 
> > Maybe we should nuke CreateOffscreenSurface() and do it inline here.  That
way
> > we can be very clear in the comments about the declaration order of
different
> > things, and we won't risk others getting this wrong.
> 
> I was searching the way for quite some time to figure that one out too. I
found
> this microsoft example which deletes bitmap before HDC:
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dd183402(v=vs.85).aspx
> And this stack overflow answer:
> http://stackoverflow.com/a/15760414/986604
> which says that it might not be correct to delete bitmap first but it's
handled
> correctly anyway.

Oh wow that story is scary.  I love Raymond Chen's stuff, he exposes the warts
:)

> So yeah, apart from confusing MS example, I guess the correct behavior is to
> delete bitmap after HDC.

Yeah, I think we should stick with that.

> As a side note... it took me a while to understand why it is leaking. It
wasn't
> super clear that selected object has to be deleted -- that deleting HDC won't
> take care of it itself. HDC after all seems to have some object selected by
> default (calling SelectObject returns it), and you don't have to delete this
one
> apparently. So it's all quite unclear IMO.

I'm wildly speculating here, but I believe the default object isn't dynamically
created (and won't be freed if you free it?).

In theory we could SelectObject() this thing back into the DC before freeing the
DC, but it shouldn't matter in our case as we're going to free the DC and
(contrary to something someone said online that I found) I don't think freeing
the DC ever makes any attempt to free selected objects, except maybe for the
Raymond Chen answer case you linked :)

> But anyways, as it's the only user of that utility function, I can inline it.
Or
> I can just explicitly call release in right order.

I suggest inlining it and seeing what the most convenient-looking way to do it
ends up being.

[Rafał Chłodnicki, 2016-09-23 20:06:08]

On 2016/09/23 19:09:10, Peter Kasting wrote:
> I suggest inlining it and seeing what the most convenient-looking way to do it
> ends up being.

The code of CreateOffscreenSurface is returning nullptr in error cases and that
would normally crash. While inlining, I'm wondering whether I should make the
code also crash (CHECK everything that can fail) or just do a safe return,
possibly just with DCHECKing. That would end up with form elements that are not
properly painted but wouldn't crash the page at least. And in debug build it
would assert.

Not sure what is the best way really.

[Peter Kasting, 2016-09-23 20:15:03]

On 2016/09/23 20:06:08, Rafał Chłodnicki wrote:
> On 2016/09/23 19:09:10, Peter Kasting wrote:
> > I suggest inlining it and seeing what the most convenient-looking way to do
it
> > ends up being.
> 
> The code of CreateOffscreenSurface is returning nullptr in error cases and
that
> would normally crash. While inlining, I'm wondering whether I should make the
> code also crash (CHECK everything that can fail) or just do a safe return,
> possibly just with DCHECKing. That would end up with form elements that are
not
> properly painted but wouldn't crash the page at least. And in debug build it
> would assert.
> 
> Not sure what is the best way really.

Did you see my comment saying you need to update your checkout, and pointing to
https://codereview.chromium.org/2355423002/ ?

[tomhudson, 2016-09-23 20:17:33]

On 2016/09/23 20:06:08, Rafał Chłodnicki wrote:
> The code of CreateOffscreenSurface is returning nullptr in error cases and
that
> would normally crash.

When would you expect it to crash?

If it's in checking IsUser32AndGdi32Available(), we may need an alternate
codepath there to support crbug.com/591500, but as far as I know that's going to
require a *lot* of plumbing.
We also just had crbug.com/648790, which was unexpectedly crashing near here.

[Rafał Chłodnicki, 2016-09-23 21:27:29]

Description was changed from

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

Ideally there would be one scoped object returned that would take care of
everything on its own but there doesn't seem to be one like that and adding
it might not be worth the effort.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
==========

to

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was inlined as it was used only in one place and
making it safe to use for others would be a bit tricky as bitmap needs to be
deleted before HDC and there is no easy access to the bitmap after utility
function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
==========

[Rafał Chłodnicki, 2016-09-23 21:33:25]

On 2016/09/23 20:17:33, tomhudson wrote:
> On 2016/09/23 20:06:08, Rafał Chłodnicki wrote:
> > The code of CreateOffscreenSurface is returning nullptr in error cases and
> that
> > would normally crash.
> 
> When would you expect it to crash?
> 
> If it's in checking IsUser32AndGdi32Available(), we may need an alternate
> codepath there to support crbug.com/591500, but as far as I know that's going
to
> require a *lot* of plumbing.
> We also just had crbug.com/648790, which was unexpectedly crashing near here.

I have overlooked the mention of crbug.com/648790 before, that's why I said it
would crash. Because it did before that fix. :)
Now also leak is fixed.

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

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:670: // calls fail, possibly due to GDI
handle exhaustion.
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.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:688: base::win::ScopedBitmap
hdc_bitmap(skia::CreateHBitmap(
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.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:756: // Delete hdc before bitmap.
Nit: Blank line above this.

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

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:758: hdc_bitmap.reset();
Nit: This statement unnecessary

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

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc#...
skia/ext/skia_utils_win.cc:201: DCHECK(bitmap);
On 2016/09/23 18:45:00, Peter Kasting wrote:
> Nit: Blank line below this

Done.

https://codereview.chromium.org/2365903002/diff/1/skia/ext/skia_utils_win.cc#...
skia/ext/skia_utils_win.cc:213: if (SelectClipRgn(scoped_hdc.Get(), clip) ==
ERROR || !DeleteObject(clip))
On 2016/09/23 18:45:00, Peter Kasting wrote:
> Nit: Removing the second set of parnes was good, I wouldn't have removed the
> first though.

Done.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:670: // calls fail, possibly due to GDI
handle exhaustion.
On 2016/09/24 00:52:34, Peter Kasting wrote:
> 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.

Done.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:688: base::win::ScopedBitmap
hdc_bitmap(skia::CreateHBitmap(
On 2016/09/24 00:52:34, Peter Kasting wrote:
> 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.

Done.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:756: // Delete hdc before bitmap.
On 2016/09/24 00:52:34, Peter Kasting wrote:
> Nit: Blank line above this.
> 
> Nit: I suggest adding "..., since objects should not be deleted while selected
> into a DC."

Done.

https://codereview.chromium.org/2365903002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:758: hdc_bitmap.reset();
On 2016/09/24 00:52:34, Peter Kasting wrote:
> Nit: This statement unnecessary

Done.

[Rafał Chłodnicki, 2016-09-26 10:13:07]

Description was changed from

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was inlined as it was used only in one place and
making it safe to use for others would be a bit tricky as bitmap needs to be
deleted before HDC and there is no easy access to the bitmap after utility
function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
==========

to

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was removed and replaced with a custom scoped object
at call site as it was used only in one place and making it safe to use for
others would be a bit tricky as bitmap needs to be deleted before HDC and there
is no easy access to the bitmap after utility function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
==========

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

LGTM

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:714: !DeleteObject(clip)) {
Nit: {} not necessary (2 places)

[Rafał Chłodnicki, 2016-09-27 07:00:38]

The CQ bit was checked by rchlodnicki@opera.com

[Rafał Chłodnicki, 2016-09-27 07:00:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/2365903002/#ps100001
(title: "fix comment")

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

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:714: !DeleteObject(clip)) {
On 2016/09/26 20:58:33, Peter Kasting wrote:
> Nit: {} not necessary (2 places)

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).

[commit-bot: I haz the power, 2016-09-27 07:00:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2365903002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:714: !DeleteObject(clip)) {
On 2016/09/27 07:00:48, Rafał Chłodnicki wrote:
> On 2016/09/26 20:58:33, Peter Kasting wrote:
> > Nit: {} not necessary (2 places)
> 
> 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).

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.

[commit-bot: I haz the power, 2016-09-27 07:54:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-27 07:56:16]

Description was changed from

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was removed and replaced with a custom scoped object
at call site as it was used only in one place and making it safe to use for
others would be a bit tricky as bitmap needs to be deleted before HDC and there
is no easy access to the bitmap after utility function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712
==========

to

==========
Fix GDI leak in NativeThemeWin::PaintIndirect

There was a GDI leak when hovering input elements. Reaching 10000 GDI objects
would cause process to crash.

Fixed by deleting HBITMAP that was selected onto the HDC. Deleting HDC does not
take care of that.

The skia utility function was removed and replaced with a custom scoped object
at call site as it was used only in one place and making it safe to use for
others would be a bit tricky as bitmap needs to be deleted before HDC and there
is no easy access to the bitmap after utility function returns HDC.

R=pkasting@chromium.org,fmalita@chromium.org,tomhudson@google.com

BUG=649712

Committed: https://crrev.com/4014935dc5f26ad7517ca8427425995bd67d6c85
Cr-Commit-Position: refs/heads/master@{#421142}
==========

[commit-bot: I haz the power, 2016-09-27 07:56:17]

Patchset 6 (id:??) landed as
https://crrev.com/4014935dc5f26ad7517ca8427425995bd67d6c85
Cr-Commit-Position: refs/heads/master@{#421142}