Bug 137014 – NSButtonCell leak allocated under WebCore::paintToggleButton

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 238502 [details]
[PATCH] Proposed Fix

Proposed fix. I haven't tested yet though.

[Alexey Proskuryakov]

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 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]

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

[Alexey Proskuryakov]

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]

Heh, finally landed this.

https://trac.webkit.org/r178397