| Summary: | NSButtonCell leak allocated under WebCore::paintToggleButton | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | WebCore Misc. | Assignee: | Joseph Pecoraro <joepeck> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, bdakin, dbates, joepeck | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 238502 [details]
[PATCH] Proposed Fix
Proposed fix. I haven't tested yet though.
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. 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. Created attachment 238557 [details]
[PATCH] Proposed Fix - RetainPtr
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. Heh, finally landed this. https://trac.webkit.org/r178397 |
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