137014 – NSButtonCell leak allocated under WebCore::paintToggleButton

RESOLVED FIXED 137014

NSButtonCell leak allocated under WebCore::paintToggleButton

https://bugs.webkit.org/show_bug.cgi?id=137014

Summary NSButtonCell leak allocated under WebCore::paintToggleButton

Joseph Pecoraro

Reported 2014-09-22 16:12:09 PDT

Saw this leak in Safari. Leak: 0x6000000f2080 size=128 zone: DefaultMallocZone_0x10921a000 NSButtonCell ObjC AppKit 0x7bccb8c8 0x00007fff 0x00000000 0x00000000 ...{............ 0x80000010 0x08080000 0x00a87120 0x00006000 ........ q...`.. 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x7baa6910 0x00007fff 0x00000080 0x004b0190 .i.{..........K. 0x483c5100 0x00000000 0x00c69a40 0x00006080 .Q<H....@....`.. 0x00471500 0x00006000 0x00000000 0x00000000 ..G..`.......... Call stack: [thread 0x7fff7b1d4300]: | start | NSApplicationMain | -[NSApplication run] | -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] | -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] | _DPSNextEvent | _BlockUntilNextEventMatchingListInModeWithFilter | ReceiveNextEventCommon | RunCurrentEventLoopInMode | CFRunLoopRunSpecific | __CFRunLoopRun | __CFRunLoopDoObservers | __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ | __83-[NSWindow _postWindowNeedsDisplayOrLayoutOrUpdateConstraintsUnlessPostingDisabled]_block_invoke1523 | _handleWindowNeedsDisplayOrLayoutOrUpdateConstraints | -[NSWindow displayIfNeeded] | -[NSView displayIfNeeded] | -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] | -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[WebHTMLView(WebPrivate) _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _drawRect:clip:] | -[WebHTMLView drawRect:] | -[WebHTMLView drawSingleRect:] | -[WebFrame(WebInternal) _drawRect:contentsOnly:] | WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) | WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::LayoutRect const&, unsigned int, WebCore::RenderObject*, unsigned int) | WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) | WebCore::RenderLayer::paintForegroundForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow> const&, WebCore::GraphicsContext*, WebCore::GraphicsContext*, WebCore::LayoutRect const&, bool, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*, bool, bool) | WebCore::RenderLayer::paintForegroundForFragmentsWithPhase(WebCore::PaintPhase, WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow> const&, WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*) | WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool) | WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool) | WebCore::RenderTable::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderTable::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderTableSection::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderTableSection::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderTableSection::paintCell(WebCore::RenderTableCell*, WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, WebCore::LayoutPoint const&) const | WebCore::RootInlineBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) | WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) | WebCore::InlineElementBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) | WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderBox::paintBoxDecorations(WebCore::PaintInfo&, WebCore::LayoutPoint const&) | WebCore::RenderTheme::paint(WebCore::RenderObject const&, WebCore::ControlStates*, WebCore::PaintInfo const&, WebCore::LayoutRect const&) | WebCore::ThemeMac::paint(WebCore::ControlPart, WebCore::ControlStates*, WebCore::GraphicsContext*, WebCore::FloatRect const&, float, WebCore::ScrollView*) | WebCore::paintToggleButton(WebCore::ControlPart, WebCore::ControlStates*, WebCore::GraphicsContext*, WebCore::FloatRect const&, float, WebCore::ScrollView*) | _objc_rootAlloc | class_createInstance | calloc | malloc_zone_calloc

Attachments
[PATCH] Proposed Fix (1.42 KB, patch) 2014-09-22 16:14 PDT, Joseph Pecoraro	joepeck: commit-queue-	Details Formatted Diff Diff
[PATCH] Proposed Fix - RetainPtr (4.83 KB, patch) 2014-09-23 12:06 PDT, Joseph Pecoraro	ap: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2014-09-22 16:14:07 PDT

Created attachment 238502 [details] [PATCH] Proposed Fix Proposed fix. I haven't tested yet though.

Alexey Proskuryakov

Comment 2 2014-09-22 20:12:04 PDT

Comment on attachment 238502 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=238502&action=review > Source/WebCore/platform/mac/ThemeMac.mm:397 > + toggleButtonCell = [createToggleButtonCell(buttonType) autorelease]; Can we adoptNS it into a RetainPtr? That's better in every way than an autorelease: - releasing is faster; - avoids mysterious crashes in autorelease pool drain function.

Darin Adler

Comment 3 2014-09-23 09:26:48 PDT

Comment on attachment 238502 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=238502&action=review >> Source/WebCore/platform/mac/ThemeMac.mm:397 >> + toggleButtonCell = [createToggleButtonCell(buttonType) autorelease]; > > Can we adoptNS it into a RetainPtr? That's better in every way than an autorelease: > > - releasing is faster; > - avoids mysterious crashes in autorelease pool drain function. Even better, the create function itself should return a RetainPtr. Generally speaking we’d like to put objects into RetainPtr as close to their allocation as possible. The adopt call should in the same expression as the alloc method call, or as close to that as we can get.

Joseph Pecoraro

Comment 4 2014-09-23 12:06:13 PDT

Created attachment 238557 [details] [PATCH] Proposed Fix - RetainPtr

Alexey Proskuryakov

Comment 5 2014-09-23 18:58:41 PDT

Comment on attachment 238557 [details] [PATCH] Proposed Fix - RetainPtr View in context: https://bugs.webkit.org/attachment.cgi?id=238557&action=review > Source/WebCore/platform/mac/ThemeMac.mm:366 > static NSButtonCell *radioCell; > if (!radioCell) > - radioCell = createToggleButtonCell(RadioPart); > + radioCell = createToggleButtonCell(RadioPart).leakRef(); A simpler way to write this is: static NSButtonCell *radioCell = createToggleButtonCell(RadioPart).leakRef(); > Source/WebCore/platform/mac/ThemeMac.mm:376 > static NSButtonCell *checkboxCell; > if (!checkboxCell) > - checkboxCell = createToggleButtonCell(CheckboxPart); > + checkboxCell = createToggleButtonCell(CheckboxPart).leakRef(); Ditto. > Source/WebCore/platform/mac/ThemeMac.mm:392 > + RetainPtr<NSButtonCell> toggleButtonCell = static_cast<NSButtonCell*>(controlStates->platformControl()); Misplaced star, should be static_cast<NSButtonCell *>. Looking at the function as a whole, I suspect that there is a better way to write it (not something for this patch). What happens now: - controlStates object may or may not have an NSButtonCell; - when it does, we use it; - when it doesn't, we create a new one; - and at the end, we sometimes put the new cell into controlStates, and sometimes don't. This is mysterious.

Joseph Pecoraro

Comment 6 2015-01-13 17:36:10 PST

Heh, finally landed this. https://trac.webkit.org/r178397

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Joseph Pecoraro

Reported

2014-09-22 16:12 PDT

Modified

2015-01-13 17:36 PST History

CC List

4 users Show

URL

Keywords

Depends on

Blocks