Fix the crash in DesktopBackgroundView::OnPaint

Fix the crash in DesktopBackgroundView::OnPaint


ImageSkia is not RefCountedThreadSafe, just RefCounted. We are accessing wallpaper(ImageSkia) on different threads(FILE and UI). When two threads try to AddRef at the same time, it cause problem. For m23, the current solution is to make a deep copy of imageskia and pass a scoped pointer to other thread. See (http://codereview.chromium.org/11361131/)

BUG=159431
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167700

[bshe, 2012-11-08 21:52:43]

Hi Oshima and Peter.

Could you please take a look? This CL is created on top of oshima's patch. So
the real files that I change are:
	 chrome/browser/chromeos/extensions/wallpaper_private_api.h	
 	 chrome/browser/chromeos/extensions/wallpaper_private_api.cc	 
	 chrome/browser/chromeos/login/wallpaper_manager.h	 View		
	 chrome/browser/chromeos/login/wallpaper_manager.cc
I originally thought to ask for review after Oshima's patch landed. But it maybe
better to get some early feedback.

Thanks!

[pkotwicz, 2012-11-09 17:45:33]

I will let oshima@ review this patch. I think he is more knowledgeable. I will
look at this CL this weekend though.

[oshima, 2012-11-12 16:39:28]

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:292: // post to
another thread.
I think you don't have to do this IF you make
wallpaper_ thread safe because there is no refcount update
by two threads.

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:71: // The decoded
wallpaper.
can you add description which thread uses this image, and how?

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/wallpaper_manager.cc:506: wallpaper.raw_image()));
Can you remove or update TODO for raw_image()? This may break if RawImage
introduces RefCount

[bshe, 2012-11-12 19:12:48]

Thanks for review!

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:292: // post to
another thread.
I may understand this wrong. But it looks to me that we may addref on UI thread
when call  SetWallpaperFromImageSkia(which calls setCustomWallpaper in
desktop_background_controller)
http://code.google.com/searchframe#OAMlx_jo-ck/src/ash/desktop_background/des...

and addref on FILE thread in ResizeAndSaveWallpaper(through CreateResizedImage)
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo...
?

On 2012/11/12 16:39:28, oshima wrote:
> I think you don't have to do this IF you make
> wallpaper_ thread safe because there is no refcount update
> by two threads.

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:71: // The decoded
wallpaper.
On 2012/11/12 16:39:28, oshima wrote:
> can you add description which thread uses this image, and how?

Done.

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:506: wallpaper.raw_image()));
Good point. Added a TODO.

On 2012/11/12 16:39:28, oshima wrote:
> Can you remove or update TODO for raw_image()? This may break if RawImage
> introduces RefCount

[oshima, 2012-11-13 17:46:42]

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:292: // post to
another thread.
On 2012/11/12 19:12:48, bshe wrote:
> I may understand this wrong. But it looks to me that we may addref on UI
thread
> when call  SetWallpaperFromImageSkia(which calls setCustomWallpaper in
> desktop_background_controller)
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ash/desktop_background/des...
> 
> and addref on FILE thread in ResizeAndSaveWallpaper(through
CreateResizedImage)
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo...
> ?
> 
> On 2012/11/12 16:39:28, oshima wrote:
> > I think you don't have to do this IF you make
> > wallpaper_ thread safe because there is no refcount update
> > by two threads.
> 

I see, you're right.

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:493: User::CUSTOMIZED,
shouldn't this be "type"?

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:494:
base::Time::Now().LocalMidnight()
add comment that this isn't used.

[bshe, 2012-11-13 21:18:45]

Done. Thanks for review. Could you please take another look?

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:493: User::CUSTOMIZED,
On 2012/11/13 17:46:42, oshima wrote:
> shouldn't this be "type"?

Aha. You are right. Sorry.

https://codereview.chromium.org/11366143/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:494:
base::Time::Now().LocalMidnight()
On 2012/11/13 17:46:42, oshima wrote:
> add comment that this isn't used.

Done.

[oshima, 2012-11-13 21:43:53]

lgtm

[bshe, 2012-11-13 21:47:58]

Thanks!

+OWNERS

+miket for extension/*
+nkostylev for login/*

[miket_OOO, 2012-11-13 22:44:26]

> +miket for extension/*

LGTM

[Nikita (slow), 2012-11-14 14:03:43]

lgtm

[commit-bot: I haz the power, 2012-11-14 14:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/11366143/8009

[commit-bot: I haz the power, 2012-11-14 18:11:18]

Change committed as 167700