Bug 66488 – [chromium] Leaking SkBitmaps for background images

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Tony Gentilcore]

It appears that images used as background images will never be collected for the lifetime of a renderer.

One problem is that NativeImageSkia is a SkBitmap, but SkBitmap's dtor isn't virtual. So we leak fields like the m_resizedImage SkBitmap.

However, just making SkBitmaps dtor virtual doesn't seem to fix the problem. There is something else going on w/ the assignment operator or copy constructors that I am still trying to figure out. Any other set of eyes on this would be appreciated.

[John Bates]

Hey Tony,

Just FYI, I have a CL in flight that touches this code.
https://bugs.webkit.org/show_bug.cgi?id=65587

From what I know about Skia's object model, deriving from their classes is going to be problematic in many ways. Ie: memset(this, 0, sizeof(*this)) and assignment is memcpy(... sizeof(*this)). Since sizeof(*this) can include bits of the derived members, it's failtastic.

I think we need to find out who creates and destroys the NativeImageSkia objects and refactor that code so that NativeImageSkia has a SkBitmap member instead of deriving from it.

[Tony Gentilcore]

> I think we need to find out who creates and destroys the NativeImageSkia objects and refactor that code so that NativeImageSkia has a SkBitmap member instead of deriving from it.

Hi John,

Yeah, that seems like the right approach. Since this ends up being a pretty noticeable leak, I was hoping there would be an easier way to patch this, but it looks like an involved refactor instead.

Since you are pretty familiar with this area, do you want to pick this up?

-Tony

[John Bates]

(In reply to comment #2)
> > I think we need to find out who creates and destroys the NativeImageSkia objects and refactor that code so that NativeImageSkia has a SkBitmap member instead of deriving from it.
> 
> Hi John,
> 
> Yeah, that seems like the right approach. Since this ends up being a pretty noticeable leak, I was hoping there would be an easier way to patch this, but it looks like an involved refactor instead.
> 
> Since you are pretty familiar with this area, do you want to pick this up?

No problem, I'll look into it once I get my other patch landed. Did you gather any more clues about where the leak was occurring?

> 
> -Tony

[John Bates]

SkBitmap can't have virtuals unless it's refactored to not do the memcpy(sizeof) tricks, since that clobbers the vtable. The symptoms I saw were things like crashes in the NativeImageSkia destructor (because it probably was a copy, and should have been a plain SkBitmap vtable).

Patch coming shortly to make SkBitmap a member...

[John Bates]

Created attachment 105081 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105081 [details]
Patch

Attachment 105081 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9509030

[John Bates]

Created attachment 105099 [details]
Patch

[Stephen White]

Comment on attachment 105099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105099&action=review

Looks good.  r=me

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:67
> +    SkBitmap& bitmap() { return m_image; }

I'm not a fan of returning a non-const reference (although that's not webkit coding style or anything, just personal preference).  Out of curiosity, how many call sites break with only the const ref variant available?

> Source/WebCore/platform/graphics/skia/PatternSkia.cpp:62
> +    const NativeImageSkia* bm = m_tileImage->nativeImageForCurrentFrame();

Nit:  Might make sense to rename "bm" to "image" now that it's a NativeImageSkia and not a bitmap.

[John Bates]

Comment on attachment 105099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105099&action=review

New patch next...

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:67
>> +    SkBitmap& bitmap() { return m_image; }
> 
> I'm not a fan of returning a non-const reference (although that's not webkit coding style or anything, just personal preference).  Out of curiosity, how many call sites break with only the const ref variant available?

There are a few calls in ImageDecoderSkia.cpp (reset, copyTo, setConfig, allocPixels, setIsOpaque). The non-const ref is just trying to mimic the original is-a behavior. I can't say I'm a fan either. It's just that the alternative of wrapping all the used SkBitmap methods seems too difficult to maintain.

>> Source/WebCore/platform/graphics/skia/PatternSkia.cpp:62
>> +    const NativeImageSkia* bm = m_tileImage->nativeImageForCurrentFrame();
> 
> Nit:  Might make sense to rename "bm" to "image" now that it's a NativeImageSkia and not a bitmap.

Done.

[John Bates]

Created attachment 105116 [details]
Patch

[Mike Reed]

works for me

[Stephen White]

Comment on attachment 105099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105099&action=review

>>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:67
>>> +    SkBitmap& bitmap() { return m_image; }
>> 
>> I'm not a fan of returning a non-const reference (although that's not webkit coding style or anything, just personal preference).  Out of curiosity, how many call sites break with only the const ref variant available?
> 
> There are a few calls in ImageDecoderSkia.cpp (reset, copyTo, setConfig, allocPixels, setIsOpaque). The non-const ref is just trying to mimic the original is-a behavior. I can't say I'm a fan either. It's just that the alternative of wrapping all the used SkBitmap methods seems too difficult to maintain.

One option would be to return a non-const pointer instead of ref.  That makes it marginally clearer that it may be modified.

[Stephen White]

Comment on attachment 105116 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 105116 [details]
Patch

Clearing flags on attachment: 105116

Committed r93792: <http://trac.webkit.org/changeset/93792>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[John Bates]

> One option would be to return a non-const pointer instead of ref.  That makes it marginally clearer that it may be modified.

I was thinking that would send the message that you can hold on to the pointer. With a reference, you'd usually have to deliberately deref into a pointer if you wanted to hold onto it.