Fix memory leak on Chromium OS Heapcheck and Chromium OS (valgrind)

Fix memory leak on  Chromium OS Heapcheck and Chromium OS (valgrind)


Memcheck:Leak
   fun:_Znw*
   fun:_ZN3ash27DesktopBackgroundController16InstallComponentEPN4aura10RootWindowE
   fun:_ZN3ash27DesktopBackgroundController29InstallComponentForAllWindowsEv
   fun:_ZN3ash27DesktopBackgroundController29SetDesktopBackgroundImageModeEv
   fun:_ZN3ash27DesktopBackgroundController20CreateEmptyWallpaperEv
   fun:_ZN3ash12_GLOBAL__N_126DummyUserWallpaperDelegate19InitializeWallpaperEv
   fun:_ZN3ash5Shell4InitEv
   fun:_ZN3ash5Shell14CreateInstanceEPNS_13ShellDelegateE
   fun:_ZN3ash4test11AshTestBase5SetUpEv

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28valgrind%29%284%29/builds/12953


BUG=141563, 141676, 142042
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151746

[bshe, 2012-08-15 00:43:40]

Hi Scott.

My CL http://codereview.chromium.org/10834308/ caused memory leaks on some
tests. Looks like the memory leak happens when test finished before wallpaper
animation complete. In that case, component is not set to root window
(kWindowDesktopComponent) property. So the destructor is not called when we set
kWindowDesktopComponent to NULL.

This CL should fix the memory leak problem. Could you please take a look?

Thanks!

+thakis FYI

[sky, 2012-08-15 15:05:10]

https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:242: if
(window->GetProperty(internal::kComponentWrapper) &&
Why the double check here? Shouldn't you only need one? Also, I think this would
be less error prone if the wrapper owned the window until release was invoked on
the wrapper. That way you shouldn't have to do anything special here.

[bshe, 2012-08-15 15:12:42]

https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:242: if
(window->GetProperty(internal::kComponentWrapper) &&
Some of the tests destroying window without even creating a wrapper. So I have
the double check here. I will make the wrapper own the window as you suggested.
Thanks!

On 2012/08/15 15:05:11, sky wrote:
> Why the double check here? Shouldn't you only need one? Also, I think this
would
> be less error prone if the wrapper owned the window until release was invoked
on
> the wrapper. That way you shouldn't have to do anything special here.

[bshe, 2012-08-15 17:51:11]

On 2012/08/15 15:12:42, bshe wrote:
>
https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
> File ash/desktop_background/desktop_background_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10854153/diff/1/ash/desktop_background...
> ash/desktop_background/desktop_background_controller.cc:242: if
> (window->GetProperty(internal::kComponentWrapper) &&
> Some of the tests destroying window without even creating a wrapper. So I have
> the double check here. I will make the wrapper own the window as you
suggested.
> Thanks!
> 
> On 2012/08/15 15:05:11, sky wrote:
> > Why the double check here? Shouldn't you only need one? Also, I think this
> would
> > be less error prone if the wrapper owned the window until release was
invoked
> on
> > the wrapper. That way you shouldn't have to do anything special here.

sky@
Done. I use a scoped ptr to manage the ownership. Could you please take another
look? Thanks!

[sky, 2012-08-15 20:45:41]

LGTM

https://chromiumcodereview.appspot.com/10854153/diff/11002/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/10854153/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.cc:55: LOG(ERROR) << "===taken
ownership===";
remove this.

[bshe, 2012-08-15 20:49:39]

Thanks

https://chromiumcodereview.appspot.com/10854153/diff/11002/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/10854153/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.cc:55: LOG(ERROR) << "===taken
ownership===";
On 2012/08/15 20:45:42, sky wrote:
> remove this.

Aha. Sorry, I missed it.