Bug 122605 – Confusing CGImageRef memory management in ImageBuffer::copyImage

• Component: Images
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

REGRESSION (r113457): CGImageRef leaks inside ImageBuffer::copyImage

[Adam Roben (:aroben)]

Created attachment 213894 [details]
Patch

[WebKit Commit Bot]

Attachment 213894 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp']" exit_code: 1
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Created attachment 213896 [details]
Patch

[Adam Roben (:aroben)]

Unsurprisingly, given that the regression was introduced 18 months ago, this leak occurs in the stock WebKit on OS X 10.8.5.

[Simon Fraser (smfr)]

Adam, what content hits this code path to cause the leak? I would have expected us to notice an image leak.

[Adam Roben (:aroben)]

This seems to be sufficient to trigger the leak:

data:text/html,%3Cimg%20src=%22http://upload.wikimedia.org/wikipedia/commons/b/bd/Test.svg%22%3E

I.e., a simple SVG-as-image.

[Radar WebKit Bug Importer]

<rdar://problem/15198634>

[Adam Roben (:aroben)]

I just realized it's possible that the image is getting cached, which would explain why it's not being dealloced. So the image I'm seeing in Instruments may not be getting leaked. But regardless the error fixed here *does* seem likely to cause a leak, even though we may not know how to trigger it. I'll keep digging to try to get to the bottom of this.

[Adam Roben (:aroben)]

FYI, the codepath I'm seeing in Instruments (which I'm still not 100% certain indicates a leak) is through SVGImageCache: http://trac.webkit.org/browser/branches/safari-536.30-branch/Source/WebCore/svg/graphics/SVGImageCache.cpp#L158

That code was removed in http://trac.webkit.org/changeset/142765 so it's possible that the leaky code in ImageBufferCG never gets hit in ToT.

Here's the full backtrace from Instruments:

   0 libsystem_c.dylib malloc_zone_malloc
   1 CoreFoundation _CFRuntimeCreateInstance
   2 CoreGraphics CGTypeCreateInstance
   3 CoreGraphics CGImageCreate
   4 CoreGraphics CGBitmapContextCreateImage
   5 WebCore WebCore::ImageBuffer::copyImage(WebCore::BackingStoreCopy, WebCore::ScaleBehavior) const
   6 WebCore WebCore::SVGImageCache::lookupOrCreateBitmapImageForRenderer(WebCore::RenderObject const*)
   7 WebCore WebCore::CachedImage::imageForRenderer(WebCore::RenderObject const*)
   8 WebCore WebCore::RenderImageResource::image(int, int) const
   9 WebCore WebCore::RenderImage::paintReplaced(WebCore::PaintInfo&, WebCore::IntPoint const&)
  10 WebCore WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::IntPoint const&)
  11 WebCore WebCore::RenderImage::paint(WebCore::PaintInfo&, WebCore::IntPoint const&)
  12 WebCore WebCore::InlineBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int)
  13 WebCore WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int)
  14 WebCore WebCore::RootInlineBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int)
  15 WebCore WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, WebCore::IntPoint const&) const
  16 WebCore WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&)
  17 WebCore WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&)
  18 WebCore WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::IntPoint const&)
  19 WebCore WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&)
  20 WebCore WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&)
  21 WebCore WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
  22 WebCore WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
  23 WebCore WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
  24 WebCore WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
  25 WebCore WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, unsigned int)
  26 WebCore WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&)
  27 WebKit -[WebFrame(WebInternal) _drawRect:contentsOnly:]
  28 WebKit -[WebHTMLView drawSingleRect:]
  29 WebKit -[WebHTMLView drawRect:]
  30 AppKit -[NSView _drawRect:clip:]
  31 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  32 WebKit -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  33 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  34 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  35 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  36 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  37 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  38 AppKit -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:]
  39 AppKit -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
  40 AppKit -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
  41 AppKit -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
  42 AppKit -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:]
  43 AppKit -[NSView displayIfNeeded]
  44 AppKit _handleWindowNeedsDisplayOrLayoutOrUpdateConstraints
  45 Foundation __NSFireTimer
  46 CoreFoundation __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
  47 CoreFoundation __CFRunLoopDoTimer
  48 CoreFoundation __CFRunLoopRun
  49 CoreFoundation CFRunLoopRunSpecific
  50 HIToolbox RunCurrentEventLoopInMode
  51 HIToolbox ReceiveNextEventCommon
  52 HIToolbox BlockUntilNextEventMatchingListInMode
  53 AppKit _DPSNextEvent
  54 AppKit -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
  55 AppKit -[NSApplication run]
  56 AppKit NSApplicationMain
  57 libdyld.dylib start

[Adam Roben (:aroben)]

Based on the arguments being passed to ImageBuffer::copyImage in SVGImageCache, it seems the leak will be hit. Specifically, http://trac.webkit.org/browser/branches/safari-536.30-branch/Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp#L219 will execute.

I'm not sure why Instruments doesn't count this as a leak.

[Adam Roben (:aroben)]

Of course, since every codepath in ImageBufferCG::copyImage results in a leak, this seems like it should be observable on ToT as well, though perhaps with a different testcase.

[WebKit Commit Bot]

Comment on attachment 213896 [details]
Patch

Rejecting attachment 213896 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 213896, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ubmit
    return self.open(self.click(*args, **kwds))
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://webkit-queues.appspot.com/results/3663121

[Adam Roben (:aroben)]

Hm, any ideas how to work around that commit-queue bug?

[Alexey Proskuryakov]

Comment on attachment 213896 [details]
Patch

Wait, EWS is all yellow - it fails to process the patch because it apparently introduced tons of flaky crashes!

[Adam Roben (:aroben)]

Oh my. Thanks for spotting that, Alexey. I'll try to reproduce the crashes today.

[Adam Roben (:aroben)]

It looks like at least one problem is that BitmapImage::BitmapImage adopts the CGImageRef passed into it rather than retaining it. How strange.

[Adam Roben (:aroben)]

WebKit2::ShareableBitmap::createImage() is aware of this: http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp?rev=156688#L118

[Adam Roben (:aroben)]

Now that I know about BitmapImage::create's strange adoption semantics, and after staring at this code a while longer, I no longer think there was ever a leak here. (Really how could I have doubted mitzpettel?) The code in ImageBuffer::copyImage was carefully crafted not to cause a leak or over-release: sometimes it would seemingly over-retain the image, but it was in fact relying on BitmapImage::create to adopt the extra reference and thus balance things out.

So I've retitled this bug to reflect the reality: ImageBuffer::copyImage contains some very confusing memory management. The patch I'm about to upload changes BitmapImage::create to retain the passed-in CGImage, making the two callers (in ImageBuffer::copyImage and ShareableBitmap::createImage) much less confusing.

[Adam Roben (:aroben)]

Created attachment 213983 [details]
Patch

[Simon Fraser (smfr)]

I do recall this weird refcounting, but I forget if there was a reason for it. Maybe look at some SVN history?

[Adam Roben (:aroben)]

Looks like it's been this way since that BitmapImage constructor was added:

Constructor is added: http://trac.webkit.org/changeset/31830/trunk/WebCore/platform/graphics/cg/ImageCG.cpp#file0
First call to it: http://trac.webkit.org/changeset/31830/trunk/WebCore/platform/graphics/cg/ImageBufferCG.cpp#file0

I'll dig around a little more to see if I can find anyone else who's tried to change it and failed.

[Adam Roben (:aroben)]

The only memory-related change I can see to BitmapImage is http://trac.webkit.org/changeset/51313, which fixed a leak introduced by http://trac.webkit.org/changeset/51212. But it's not really related to this patch. The code to adopt the CGImageRef is unchanged since it was first added back in r31830.

[Simon Fraser (smfr)]

Comment on attachment 213983 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.h:139
> +        RetainPtr<CGImageRef> copyNativeImage(BackingStoreCopy = CopyBackingStore) const;

I don't think this should have 'copy' in the name now that it returns a RetainPtr.

[Adam Roben (:aroben)]

Comment on attachment 213983 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.h:139
>> +        RetainPtr<CGImageRef> copyNativeImage(BackingStoreCopy = CopyBackingStore) const;
> 
> I don't think this should have 'copy' in the name now that it returns a RetainPtr.

In the CopyBackingStore and accelerated cases, this function copies potentially large amounts of data. It seems nice to make that explicit.

If we do rename this, should we rename ImageBuffer::copyImage too? It returns a PassRefPtr<Image>.

[Simon Fraser (smfr)]

(In reply to comment #24)
> (From update of attachment 213983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213983&action=review
> 
> >> Source/WebCore/platform/graphics/ImageBuffer.h:139
> >> +        RetainPtr<CGImageRef> copyNativeImage(BackingStoreCopy = CopyBackingStore) const;
> > 
> > I don't think this should have 'copy' in the name now that it returns a RetainPtr.
> 
> In the CopyBackingStore and accelerated cases, this function copies potentially large amounts of data. It seems nice to make that explicit.

OK, I'll buy that.

[WebKit Commit Bot]

Comment on attachment 213983 [details]
Patch

Clearing flags on attachment: 213983

Committed r157310: <http://trac.webkit.org/changeset/157310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.