REGRESSION (r113457): CGImageRef leaks inside ImageBuffer::copyImage
Created attachment 213894 [details] Patch
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.
Created attachment 213896 [details] Patch
Unsurprisingly, given that the regression was introduced 18 months ago, this leak occurs in the stock WebKit on OS X 10.8.5.
Adam, what content hits this code path to cause the leak? I would have expected us to notice an image leak.
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.
<rdar://problem/15198634>
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.
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
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.
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.
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
Hm, any ideas how to work around that commit-queue bug?
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!
Oh my. Thanks for spotting that, Alexey. I'll try to reproduce the crashes today.
It looks like at least one problem is that BitmapImage::BitmapImage adopts the CGImageRef passed into it rather than retaining it. How strange.
WebKit2::ShareableBitmap::createImage() is aware of this: http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp?rev=156688#L118
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.
Created attachment 213983 [details] Patch
I do recall this weird refcounting, but I forget if there was a reason for it. Maybe look at some SVN history?
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.
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.
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.
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>.
(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.
Comment on attachment 213983 [details] Patch Clearing flags on attachment: 213983 Committed r157310: <http://trac.webkit.org/changeset/157310>
All reviewed patches have been landed. Closing bug.