Eliminate HICON leaks caused by creating icons from bitmap image.

Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=

Committed: https://crrev.com/280746cce16d03242f855c4c795a62b7349f38d4
Cr-Commit-Position: refs/heads/master@{#366055}

[sky, 2015-11-02 16:41:38]

https://codereview.chromium.org/1406403007/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/1406403007/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:590:
small_window_icon_.Set(small_icon);
Won't this be problematic with the code on 618-623?

Also, you should make CreateHICONFromSkBitmapSizedTo return a scopedHICON to
make ownership clear. I suspect this means adding move semantics to ScopedHICon,
but that's for the better.

[anpol, 2015-11-03 11:02:03]

Description was changed from

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=
==========

to

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=
==========

[anpol, 2015-11-03 11:02:03]

anpol@yandex-team.ru changed reviewers:
+ brettw@chromium.org, cpu@chromium.org

[anpol, 2015-11-03 12:29:16]

anpol@yandex-team.ru changed reviewers:
- cpu@chromium.org

[anpol, 2015-11-03 12:38:42]

On 2015/11/02 16:41:38, sky wrote:
>
https://codereview.chromium.org/1406403007/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:590:
> small_window_icon_.Set(small_icon);
> Won't this be problematic with the code on 618-623?

Yes, those lines were the previous attempt to fix the same memory leak. Removing
them.

> Also, you should make CreateHICONFromSkBitmapSizedTo return a scopedHICON to
> make ownership clear. I suspect this means adding move semantics to
ScopedHICon,
> but that's for the better.

Done.

[anpol, 2015-11-03 12:39:52]

Add move semantics to ScopedGDIObject.

[sky, 2015-11-03 17:23:16]

https://codereview.chromium.org/1406403007/diff/20001/base/win/scoped_gdi_obj...
File base/win/scoped_gdi_object.h (right):

https://codereview.chromium.org/1406403007/diff/20001/base/win/scoped_gdi_obj...
base/win/scoped_gdi_object.h:26: // Constructor that takes ownership of an
object.
This comment is useful as it clearly documents ownership. The other comments you
are adding are obvious from the context and I wouldn't bother adding them.

https://codereview.chromium.org/1406403007/diff/20001/base/win/scoped_gdi_obj...
base/win/scoped_gdi_object.h:30: ScopedGDIObject(ScopedGDIObject&& other) :
object_(NULL) {
If you can, make these nullptr.

https://codereview.chromium.org/1406403007/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1406403007/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:811:
window_icon_.Set(IconUtil::CreateHICONFromSkBitmap(*window_icon.bitmap()));
Seems risky to destroy the old icon (as Set destroys the old) while the HWND is
still referencing it. You should use a temp here to avoid that.

Same comment below.

[anpol, 2015-11-04 11:20:36]

PTAL

[sky, 2015-11-04 16:21:14]

LGTM

[anpol, 2015-11-06 19:39:23]

anpol@yandex-team.ru changed reviewers:
+ grt@chromium.org
- brettw@chromium.org

[anpol, 2015-11-06 19:43:40]

scoped_gdi_object.h is to be reviewed.

[grt (UTC plus 2), 2015-11-06 19:49:20]

https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
File base/win/scoped_gdi_object.h (right):

https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
base/win/scoped_gdi_object.h:19: class ScopedGDIObject {
can you implement this in terms of ScopedGeneric? it seems odd that the original
commit message for SG mentions ScopedGDIObject as a candidate for using it, yet
it doesn't.

[anpol, 2015-11-06 20:28:16]

https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
File base/win/scoped_gdi_object.h (right):

https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
base/win/scoped_gdi_object.h:19: class ScopedGDIObject {
On 2015/11/06 19:49:20, grt wrote:
> can you implement this in terms of ScopedGeneric? it seems odd that the
original
> commit message for SG mentions ScopedGDIObject as a candidate for using it,
yet
> it doesn't.

Looking at ScopedGeneric briefly:

* ScopedGDIObject defines Get/Set while ScopedGeneric defines get/reset.
* ScopedGDIObject defines implicit conversion to/from handle type,
  while ScopedGeneric makes conversion explicit and defines == and !=
  for convenience.

Simply replacing ScopedGDIObject with ScopedGeneric would break existing code
that relies on 'base' library. While we can fix Chromium code, there are other
projects too. Any suggestions?

[grt (UTC plus 2), 2015-11-06 20:37:28]

On 2015/11/06 20:28:16, anpol wrote:
>
https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
> File base/win/scoped_gdi_object.h (right):
> 
>
https://codereview.chromium.org/1406403007/diff/60001/base/win/scoped_gdi_obj...
> base/win/scoped_gdi_object.h:19: class ScopedGDIObject {
> On 2015/11/06 19:49:20, grt wrote:
> > can you implement this in terms of ScopedGeneric? it seems odd that the
> original
> > commit message for SG mentions ScopedGDIObject as a candidate for using it,
> yet
> > it doesn't.
> 
> Looking at ScopedGeneric briefly:
> 
> * ScopedGDIObject defines Get/Set while ScopedGeneric defines get/reset.

It is unfortunate that there are two ways to do the same thing in two
conceptually similar classes. Harmonizing so that as many ScopedFOO classes as
possible have the same API is a good thing.

> * ScopedGDIObject defines implicit conversion to/from handle type,
>   while ScopedGeneric makes conversion explicit and defines == and !=
>   for convenience.
> 
> Simply replacing ScopedGDIObject with ScopedGeneric would break existing code
> that relies on 'base' library. While we can fix Chromium code, there are other
> projects too.

base/ is a part of (and for use by) Chromium. Consumers outside of Chromium must
constantly adjust to changes to "keep up." This is not a concern of ours, and
should not be used as an argument to have senseless duplication of nuanced code
in base/.

> Any suggestions?

How widespread is the use of ScopedGDIObject in Chromium? How large would the
change be to update it?

[anpol, 2015-11-09 10:48:11]

anpol@yandex-team.ru changed reviewers:
+ cpu@chromium.org

[anpol, 2015-11-09 10:48:12]

13 more files updated, PTAL.

[grt (UTC plus 2), 2015-11-09 16:55:39]

https://codereview.chromium.org/1406403007/diff/80001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1406403007/diff/80001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:166: HICON IconUtil::CreateHICONFromSkBitmap(const SkBitmap&
bitmap) {
It seems safest for this to return base::win::ScopedHICON, making it even more
clear that the caller owns the HICON after this call. Is it practical to do so?

https://codereview.chromium.org/1406403007/diff/80001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:322: HICON IconUtil::CreateCursorFromDIB(const gfx::Size&
icon_size,
base::win::ScopedHICON?

[anpol, 2015-11-10 15:40:23]

https://codereview.chromium.org/1406403007/diff/80001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1406403007/diff/80001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:166: HICON IconUtil::CreateHICONFromSkBitmap(const SkBitmap&
bitmap) {
On 2015/11/09 16:55:39, grt wrote:
> It seems safest for this to return base::win::ScopedHICON, making it even more
> clear that the caller owns the HICON after this call. Is it practical to do
so?

Done.

[grt (UTC plus 2), 2015-11-10 16:44:43]

https://codereview.chromium.org/1406403007/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/1406403007/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:77: .Pass();
can this be removed?

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util.cc#ne...
ui/gfx/icon_util.cc:260: scoped_ptr<SkBitmap>
bitmap(IconUtil::CreateSkBitmapFromHICON(icon.get()));
    return make_scoped_ptr(CreateSkBitmapFromHICON(icon.get()));

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util.cc#ne...
ui/gfx/icon_util.cc:369: return
base::win::ScopedHICON(CreateIconIndirect(&ii)).Pass();
can you remove .Pass() here?

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
File ui/gfx/icon_util_unittest.cc (right):

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
ui/gfx/icon_util_unittest.cc:19: using base::win::ScopedHICON;
nit: can you move this into the public: section of IconUtilTest?

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
ui/gfx/icon_util_unittest.cc:53: return ScopedHICON(icon).Pass();
can you remove .Pass() here?

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
ui/gfx/icon_util_unittest.cc:93: EXPECT_TRUE(icon.is_valid());
  EXPECT_TRUE(LoadIconFromFile(icon_filename, kSmallIconWidth,
kSmallIconHeight).is_valid());

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
ui/gfx/icon_util_unittest.cc:148: ScopedHICON icon =
can you use:
  ScopedHICON icon(LoadIconFromFile(icon_filename, icon_size.width(),
icon_size.height()));

https://codereview.chromium.org/1406403007/diff/100001/ui/gfx/icon_util_unitt...
ui/gfx/icon_util_unittest.cc:310: ScopedHICON icon =
IconUtil::CreateHICONFromSkBitmap(bitmap).Pass();
  ScopedHICON icon(IconUtil::CreateHICONFromSkBitmap(bitmap));

[anpol, 2015-11-11 06:12:25]

Removing redundant Pass() calls.

[grt (UTC plus 2), 2015-11-11 17:36:02]

please fix compile errors

[cpu_(ooo_6.6-7.5), 2015-11-11 20:09:36]

Thanks for doing this!

I think grt@ has you covered so at the end I am going to just rubberstamp this
but let me tell you a problem I've run into trying to clean up these leaks.

1- Note that not all icons can / should be destroyed with DestroyIcon.

2- In general you need to preserve the order of association on user32 and gdi
objects. That is, destroying an icon that has been associated to a window after
the window is destroyed is a bad thing. The leak is probably worse (maybe) but
having the scoped objects means you either invert the order or the order is hard
to tell.

Windows might be fortified for these cases of errors but if straitforward we
should make sure do have this in mind.

[anpol, 2015-12-07 14:04:27]

On 2015/11/11 20:09:36, cpu wrote:
> 1- Note that not all icons can / should be destroyed with DestroyIcon.

That's true, and we are not planning to destroy shared icons.

> 2- In general you need to preserve the order of association on user32 and gdi
> objects. That is, destroying an icon that has been associated to a window
after
> the window is destroyed is a bad thing.

I'm not sure about ordering issue. As far as understand there could be
a trouble when the destroyed icon is still referenced by an existing window,
but the reverse is harmless. Actually, a created icon could be referenced by
two or more windows at the same time, so we should call DestroyIcon after all
such windows cease to exist.

Anyway, let me clarify the ownership:

1. HWNDMessageHandler::SetWindowIcons

   When gfx::ImageSkia icon is provided, it gets converted into HICON,
   and that HICON should be owned by for the lifetime of the HWND.

   HWNDMessageHandler is the best owner for the converted HICON,
   as long as it gets destroyed at the same time when HWND is
   destroyed, when message handler receives WM_NC_DESTROY message.

   So we are storing ScopedHICON as a member of HWNDMessageHandler.

2. GlassBrowserFrameView::StopThrobber

   When the throbber is stopped, we should obtain gfx::ImageSkia icon
   from the browser contents, convert it to HICON, own it, and assign
   it into a browser frame HWND.

   I propose to make GlassBrowserFrameView an owner of the converted
   icon, storing ScopedHICON as a member of of that view.
   The view gets destroyed when its frame is receives WM_NC_DESTROY
   message and destroys its child views. Normally this should be OK.

   Probably, we should better transfer HICON ownership to a frame
   window itself. Currently there is no way to do that, because neither
   frame window object provides a public API to SetWindowIcons, nor
   WM_SETICON message provides an option to transfer HICON's ownership.

[anpol, 2015-12-16 17:40:05]

PTAL

[grt (UTC plus 2), 2015-12-16 18:56:20]

lgtm

[cpu_(ooo_6.6-7.5), 2015-12-18 01:17:45]

The CQ bit was checked by cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-12-18 01:17:45]

lgtm

[cpu_(ooo_6.6-7.5), 2015-12-18 01:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1406403007/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-18 01:20:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406403007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406403007/160001

[commit-bot: I haz the power, 2015-12-18 01:25:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-18 01:25:28]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anpol, 2015-12-18 07:38:22]

The CQ bit was checked by anpol@yandex-team.ru

[anpol, 2015-12-18 07:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
sky@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1406403007/#ps180001
(title: "Rebase")

[anpol, 2015-12-18 07:38:23]

The CQ bit was unchecked by anpol@yandex-team.ru

[anpol, 2015-12-18 07:40:27]

The CQ bit was checked by anpol@yandex-team.ru

[commit-bot: I haz the power, 2015-12-18 07:40:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406403007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406403007/180001

[commit-bot: I haz the power, 2015-12-18 08:39:09]

Description was changed from

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=
==========

to

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2015-12-18 08:39:10]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-18 08:40:10]

Description was changed from

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=
==========

to

==========
Eliminate HICON leaks caused by creating icons from bitmap image.

Every icon handle created by CreateHICONFromSkBitmap() should become
immediately owned by its creator. We use base::win::ScopedHICON to
control the icon's lifetime.

R=sky@chromium.org
BUG=

Committed: https://crrev.com/280746cce16d03242f855c4c795a62b7349f38d4
Cr-Commit-Position: refs/heads/master@{#366055}
==========

[commit-bot: I haz the power, 2015-12-18 08:40:11]

Patchset 10 (id:??) landed as
https://crrev.com/280746cce16d03242f855c4c795a62b7349f38d4
Cr-Commit-Position: refs/heads/master@{#366055}