Bug 12879

Summary: leaks in BidiRun::operator new seen while running WebKit unit tests
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: Layout and RenderingAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P1    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
patch darin: review+

Description Geoffrey Garen 2007-02-24 10:55:35 PST 
This bug is also in Radar as <rdar://4987649>

* STEPS TO REPRODUCE
run-webkit-tests --leaks --verbose --nthly=500

	Call stack: [thread 1a4acb3]: | 0x0 | start | _start | main | dumpRenderTree | runTest | -[NSRunLoop runMode:beforeDate:] | CFRunLoopRunSpecific | _sendCallbacks | -[NSURLConnection(NSURLConnectionInternal_ClientThread) _sendCallbacks] | -[NSURLConnection(NSURLConnectionInternal_ClientThread) _sendDidFinishLoadingCallback:] | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) | WebCore::MainResourceLoader::didFinishLoading() | WebCore::FrameLoader::finishedLoading() | WebCore::DocumentLoader::finishedLoading() | WebCore::FrameLoader::end() | WebCore::FrameLoader::endIfNotLoading() | WebCore::Document::finishParsing() | WebCore::HTMLTokenizer::finish() | WebCore::HTMLTokenizer::end() | WebCore::HTMLParser::finished() | WebCore::Document::finishedParsing() | WebCore::FrameLoader::finishedParsing() | WebCore::FrameLoader::checkCompleted() | WebCore::FrameLoader::checkEmitLoadEvent() | WebCore::Document::implicitClose() | WebCore::EventTargetNode::dispatchWindowEvent(WebCore::AtomicString const&, bool, bool) | WebCore::Document::handleWindowEvent(WebCore::Event*, bool) | KJS::JSAbstractEventListener::handleEvent(WebCore::Event*, bool) | KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::DeclaredFunctionImp::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::ExprStatementNode::execute(KJS::ExecState*) | KJS::FunctionCallResolveNode::evaluate(KJS::ExecState*) | KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::DeclaredFunctionImp::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::ExprStatementNode::execute(KJS::ExecState*) | KJS::FunctionCallResolveNode::evaluate(KJS::ExecState*) | KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::DeclaredFunctionImp::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::ExprStatementNode::execute(KJS::ExecState*) | KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) | KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::DOMEventTargetNodePrototypeFunction::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | WebCore::EventTargetNode::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, int&, bool) | WebCore::EventTargetNode::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, int&, bool, WebCore::EventTarget*) | WebCore::EventTargetNode::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event>, int&, bool) | WebCore::HTMLSelectElement::defaultEventHandler(WebCore::Event*) | WebCore::HTMLSelectElement::listBoxDefaultEventHandler(WebCore::Event*) | WebCore::Element::focus() | WebCore::Element::updateFocusAppearance() | WebCore::RenderLayer::scrollRectToVisible(WebCore::IntRect const&, WebCore::RenderLayer::ScrollAlignment const&, WebCore::RenderLayer::ScrollAlignment const&) | WebCore::FrameView::scrollPointRecursively(int, int) | WebCore::ScrollView::scrollPointRecursively(int, int) | -[NSView scrollPoint:] | -[NSClipView _scrollPoint:fromView:] | -[NSClipView _scrollTo:animate:] | -[NSScrollView scrollClipView:toPoint:] | -[NSClipView _immediateScrollToPoint:] | -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] | -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] | -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] | CFArrayApplyFunction | -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] | CFArrayApplyFunction | -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] | CFArrayApplyFunction | -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:] | -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] | -[NSView _drawRect:clip:] | -[WebHTMLView drawRect:] | -[WebHTMLView drawSingleRect:] | -[WebCoreFrameBridge drawRect:] | WebCore::Frame::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) | WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, WebCore::PaintRestriction, WebCore::RenderObject*) | WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, bool, WebCore::PaintRestriction, WebCore::RenderObject*) | WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, bool, WebCore::PaintRestriction, WebCore::RenderObject*) | WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintContents(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintChildren(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintContents(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintChildren(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paintContents(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderFlow::paintLines(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RootInlineBox::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::InlineFlowBox::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::InlineBox::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderListBox::paintObject(WebCore::RenderObject::PaintInfo&, int, int) | WebCore::RenderListBox::paintItemForeground(WebCore::RenderObject::PaintInfo&, int, int, int) | WebCore::RenderBlock::bidiReorderCharacters(WebCore::Document*, WebCore::RenderStyle*, WTF::Vector<unsigned short, (unsigned long)1024>&) | WebCore::RenderBlock::bidiReorderLine(WebCore::BidiIterator const&, WebCore::BidiIterator const&, WebCore::BidiState&) | WebCore::appendRun(WebCore::BidiState&) | WebCore::appendRunsForObject(int, int, WebCore::RenderObject*, WebCore::BidiState&) | WebCore::BidiRun::operator new(unsigned long, WebCore::RenderArena*) | WebCore::RenderArena::allocate(unsigned long) | malloc | malloc_zone_malloc

-------------------------------------------

<GMT15-Feb-2007 17:28:54GMT> Geoff Garen:
To reproduce, you have to remove the command to ignore this leak from  run-webkit-tests.

<GMT23-Feb-2007 17:14:10GMT> Geoff Garen:
A counter confirms that DRT exits without deallocating all BidiRuns.

<GMT23-Feb-2007 17:14:41GMT> Geoff Garen:
Reproducible with fast/forms/focus2.html.
Comment 1 Geoffrey Garen 2007-02-24 10:56:26 PST 
Created attachment 13363 [details]
patch

I'd like Mitz or Hyatt to take a look at this.
Comment 2 Dave Hyatt 2007-02-24 13:51:28 PST 
Comment on attachment 13363 [details]
patch

Translation of this insulting ChangeLog:

+        The ownership model in bidi.cpp is the stuff of legend. 

"I don't understand line layout."

+        A seemingly random assortment of functions allocates BidiRuns and puts them in a global data
+        structure. 

"I don't understand line layout."

+        Another seemingly random assortment of functions uses the BidiRuns
+        in the global data structure. 

"I don't understand line layout."

+        Anybody calling these functions is responsible 
+        for knowing when runs may be allocated and when they may be used, and 
+        calling deleteBidiRuns() at the appropriate time.
+        

"In extremely obscure cases that don't come up on the Web, we happen to leak.  But I'll insult the whole
model because it's an easy target."

Seriously, that display:compact leaks is hardly call for all this drama.  It's an obscure unused feature that other browsers don't even implement.  That it has bugs is utterly unsurprising and not even high priority.
Comment 3 mitz 2007-02-24 14:28:05 PST 
(In reply to comment #2)
> "In extremely obscure cases that don't come up on the Web, we happen to leak. 

bidiReorderCharacters() leaks *every time you paint* a list box or a file upload widget. That's a recent regression that was introduced under my not-so-watchful eye, and I thank Geoff for spotting it :-)
Comment 4 Darin Adler 2007-02-24 14:38:57 PST 
Comment on attachment 13363 [details]
patch

r=me
Comment 5 Dave Hyatt 2007-02-24 16:42:32 PST 
(In reply to comment #3)
> (In reply to comment #2)
> > "In extremely obscure cases that don't come up on the Web, we happen to leak. 
> 
> bidiReorderCharacters() leaks *every time you paint* a list box or a file
> upload widget. That's a recent regression that was introduced under my
> not-so-watchful eye, and I thank Geoff for spotting it :-)
> 

So it's just a regression and not some systemic flaw.
Comment 6 Geoffrey Garen 2007-02-24 18:27:11 PST 
Committed revision 19843.