Bug 53384 – RenderFullScreen::createFullScreenStyle() leaks

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Leak bot output, at <http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r77071%20(14433)/DumpRenderTree10-leaks.txt>, shows this leak:


Leak: 0x11baec1d0  size=32  zone: DefaultMallocZone_0x105977000	string ''
	Call stack: [thread 0x7fff70b12c20]:
	| 0x2
	| start
	| main
	| dumpRenderTree(int, char const**)
	| runTestingServerLoop()
	| runTest(std::string const&)
	| -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
	| CFRunLoopRunSpecific
	| __CFRunLoopRun
	| __CFRunLoopDoSources0
	| MultiplexerSource::perform()
	| URLConnectionClient::processEvents()
	| URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long)
	| URLConnectionClient::_clientDidReceiveData(__CFData const*, URLConnectionClient::ClientConnectionEventQueue*)
	| _NSURLConnectionDidReceiveData
	| -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:]
	| WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
	| WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool)
	| WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool)
	| WebCore::MainResourceLoader::addData(char const*, int, bool)
	| WebCore::DocumentLoader::receivedData(char const*, int)
	| WebCore::DocumentLoader::commitLoad(char const*, int)
	| WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int)
	| -[WebDataSource(WebInternal) _receivedData:]
	| -[WebHTMLRepresentation receivedData:withDataSource:]
	| -[WebFrame(WebInternal) _commitData:]
	| WebCore::DocumentLoader::commitData(char const*, int)
	| WebCore::DocumentWriter::addData(char const*, int, bool)
	| WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, int, bool)
	| WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&)
	| WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
	| WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
	| WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
	| WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition<WTF::OneBasedNumber> const&)
	| WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition<WTF::OneBasedNumber> const&)
	| WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&)
	| WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS)
	| WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS)
	| WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS)
	| WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue)
	| JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue)
	| JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*)
	| JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
	| 0x56847de001b8
	| WebCore::jsElementPrototypeFunctionWebkitRequestFullScreen(JSC::ExecState*)
	| WebCore::Element::webkitRequestFullScreen(unsigned short)
	| WebCore::Document::webkitRequestFullScreenForElement(WebCore::Element*, unsigned short)
	| WebChromeClient::enterFullScreenForElement(WebCore::Element*)
	| CallUIDelegate(WebView*, objc_selector*, objc_object*, objc_object*)
	| CallDelegate(WebView*, objc_object*, objc_selector*, objc_object*, objc_object*)
	| -[UIDelegate webView:enterFullScreenForElement:listener:]
	| -[WebKitFullScreenListener webkitWillEnterFullScreen]
	| WebCore::Document::webkitWillEnterFullScreenForElement(WebCore::Element*)
	| WebCore::Document::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::attach()
	| WebCore::Node::createRendererIfNeeded()
	| WebCore::RenderFullScreen::createFullScreenStyle()
	| WebCore::RenderStyle::createDefaultStyle()
	| WebCore::RenderStyle::RenderStyle(bool)
	| WebCore::DataRef<WebCore::StyleFlexibleBoxData>::init()
	| WebCore::StyleFlexibleBoxData::create()
	| WTF::RefCounted<WebCore::StyleFlexibleBoxData>::operator new(unsigned long)
	| WTF::fastMalloc(unsigned long)
	| malloc
	| malloc_zone_malloc 
Leak: 0x11baec1f0  size=32  zone: DefaultMallocZone_0x105977000	string ''

[Simon Fraser (smfr)]

Here's another one:


Leak: 0x11bae9ec0  size=336  zone: DefaultMallocZone_0x105977000	
	0x1bac3940 0x00000001 0x00000130 0x00000000 	@9......0.......
	0xdba00aea 0x00000000 0x02c91d70 0x00000001 	........p.......
	0x1baea010 0x00000001 0x00000001 0x00000000 	................
	0x1bae9538 0x00000001 0x00000000 0x00000000 	8...............
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000320 0x00000258 	........ ...X...
	0x00000000 0x00000000 0x00000320 0x00000258 	........ ...X...
	...
	Call stack: [thread 0x7fff70b12c20]:
	| 0x2
	| start
	| main
	| dumpRenderTree(int, char const**)
	| runTestingServerLoop()
	| runTest(std::string const&)
	| -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
	| CFRunLoopRunSpecific
	| __CFRunLoopRun
	| __CFRunLoopDoSources0
	| MultiplexerSource::perform()
	| URLConnectionClient::processEvents()
	| URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long)
	| URLConnectionClient::_clientDidReceiveData(__CFData const*, URLConnectionClient::ClientConnectionEventQueue*)
	| _NSURLConnectionDidReceiveData
	| -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:]
	| WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
	| WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool)
	| WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool)
	| WebCore::MainResourceLoader::addData(char const*, int, bool)
	| WebCore::DocumentLoader::receivedData(char const*, int)
	| WebCore::DocumentLoader::commitLoad(char const*, int)
	| WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int)
	| -[WebDataSource(WebInternal) _receivedData:]
	| -[WebHTMLRepresentation receivedData:withDataSource:]
	| -[WebFrame(WebInternal) _commitData:]
	| WebCore::DocumentLoader::commitData(char const*, int)
	| WebCore::DocumentWriter::addData(char const*, int, bool)
	| WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, int, bool)
	| WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&)
	| WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
	| WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
	| WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
	| WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition<WTF::OneBasedNumber> const&)
	| WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition<WTF::OneBasedNumber> const&)
	| WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&)
	| WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS)
	| WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS)
	| WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS)
	| WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue)
	| JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue)
	| JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*)
	| JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
	| 0x56847de001b8
	| WebCore::jsElementPrototypeFunctionWebkitRequestFullScreen(JSC::ExecState*)
	| WebCore::Element::webkitRequestFullScreen(unsigned short)
	| WebCore::Document::webkitRequestFullScreenForElement(WebCore::Element*, unsigned short)
	| WebChromeClient::enterFullScreenForElement(WebCore::Element*)
	| CallUIDelegate(WebView*, objc_selector*, objc_object*, objc_object*)
	| CallDelegate(WebView*, objc_object*, objc_selector*, objc_object*, objc_object*)
	| -[UIDelegate webView:enterFullScreenForElement:listener:]
	| -[WebKitFullScreenListener webkitWillEnterFullScreen]
	| WebCore::Document::webkitWillEnterFullScreenForElement(WebCore::Element*)
	| WebCore::Document::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	| WebCore::Element::attach()
	| WebCore::Node::createRendererIfNeeded()
	| WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>)
	| WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
	| WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
	| WebCore::RenderBoxModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
	| WebCore::RenderLayer::operator new(unsigned long, WebCore::RenderArena*)
	| WebCore::RenderArena::allocate(unsigned long)
	| malloc
	| malloc_zone_malloc

[Jer Noble]

Looks like the last line of createFullScreenStyle() should be changed from:

    return fullscreenStyle;

to:

    return fullscreenStyle.release();

As per the "Mixing RefPtr and PassRefPtr" section of http://webkit.org/coding/RefPtr.html.

[Sam Weinig]

*** Bug 53385 has been marked as a duplicate of this bug. ***

[Jer Noble]

*** Bug 56419 has been marked as a duplicate of this bug. ***

[Jer Noble]

<rdar://problem/9138229>

[Adam Roben (:aroben)]

(In reply to comment #2)
> Looks like the last line of createFullScreenStyle() should be changed from:
> 
>     return fullscreenStyle;
> 
> to:
> 
>     return fullscreenStyle.release();
> 
> As per the "Mixing RefPtr and PassRefPtr" section of http://webkit.org/coding/RefPtr.html.

That would be a good change to make. But I don't think it will have any effect on this leak. All it will do is save an unnecessary ref()/deref() pair from occurring.

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #2)
> > Looks like the last line of createFullScreenStyle() should be changed from:
> > 
> >     return fullscreenStyle;
> > 
> > to:
> > 
> >     return fullscreenStyle.release();
> > 
> > As per the "Mixing RefPtr and PassRefPtr" section of http://webkit.org/coding/RefPtr.html.
> 
> That would be a good change to make. But I don't think it will have any effect on this leak. All it will do is save an unnecessary ref()/deref() pair from occurring.

Okay, I'll keep investigating then.

[Jer Noble]

Replacing RenderStyle::createDefaultStyle() with RenderStyle::create() drops the number of leaks found while running the Layout/fullscreen tests from 25 -> 12.   I haven't yet been able to track down exactly what is causing the remaining leaks yet.

[Jer Noble]

(continuing)...though that's probably due to the contents of the leaked object being smaller, due to less initialization being done.

[Jer Noble]

Created attachment 85975 [details]
Patch

`run-layout-tests -l fullscreen` now results in 1 total leak found.

[Adam Roben (:aroben)]

Comment on attachment 85975 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::Document::setFullScreenRenderer): release() the return value.
> +        * rendering/RenderFullScreen.cpp:
> +        (RenderFullScreen::createFullScreenStyle): Destroy the current renderer when a new one is set.

This comments are backward.

[Jer Noble]

Comment on attachment 85975 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        (RenderFullScreen::createFullScreenStyle): Destroy the current renderer when a new one is set.
> 
> This comments are backward.

::facepalm::  Fixed.

[Jer Noble]

Created attachment 85980 [details]
Patch

[Jer Noble]

The remaining leak is: 


    Call stack: [thread 0x7fff74efa8d0]: 
     0x2 
     start 
     main 
     dumpRenderTree(int, char const**) 
     prepareConsistentTestingEnvironment() 
     setDefaultsToConsistentValuesForTesting() 
     resetDefaultsToConsistentValues() 
     LayoutTestController::setSerializeHTTPLoads(bool) 
     objc_msgSend 
     lookUpMethod 
     prepareForMethodLookup 
     _class_initialize 
     +[WebView initialize] 
     JSC::initializeThreading() 
     pthread_once 
     JSC::initializeThreadingOnce() 
     JSC::JSGlobalData::storeVPtrs() 
     JSC::ExecutableBase::createStructure(JSC::JSValue) 
     JSC::Structure::create(JSC::JSValue, JSC::TypeInfo const&, unsigned int, JSC::ClassInfo const*) 
     WTF::RefCounted<JSC::Structure>::operator new(unsigned long) 
     WTF::fastMalloc(unsigned long) 
     malloc 
     malloc_zone_malloc 

So, unlikely to be related to full screen.

[WebKit Commit Bot]

Comment on attachment 85980 [details]
Patch

Clearing flags on attachment: 85980

Committed r81291: <http://trac.webkit.org/changeset/81291>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81291 might have broken Windows 7 Release (Tests)
The following tests are not passing:
fast/events/popup-blocking-timers.html