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.
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.
> 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
(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
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...
Created attachment 105081 [details] Patch
Comment on attachment 105081 [details] Patch Attachment 105081 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9509030
Created attachment 105099 [details] Patch
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.
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.
Created attachment 105116 [details] Patch
works for me
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.
Comment on attachment 105116 [details] Patch Looks good. r=me
Comment on attachment 105116 [details] Patch Clearing flags on attachment: 105116 Committed r93792: <http://trac.webkit.org/changeset/93792>
All reviewed patches have been landed. Closing bug.
> 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.