Bug 23024 – Crash beneath addOverhangingFloats() at trazi.appspot.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Hin-Chung Lam]

Originally filed for Chromium: http://code.google.com/p/chromium/issues/detail?id=5715

Crashes in WebCore::RenderObject::enclosingLayer()

Stack trace for the crash of WebKit in Windows:
 	WebKit.dll!WebCore::RenderObject::enclosingLayer()  Line 495	C++
 	WebKit.dll!WebCore::RenderBlock::addOverhangingFloats(WebCore::RenderBlock * child=0x7fa69560, int xoff=0, int yoff=0, bool makeChildPaintOtherFloats=true)  Line 2933 + 0x8 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=false, int & maxFloatBottom=0)  Line 1355 + 0x2b bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=)  Line 660	C++
 	WebKit.dll!WebCore::RenderObject::contentWidth()  Line 570 + 0x56 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::maxTopMargin(bool positive=false)  Line 84 + 0x4a bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 571	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=true, int & maxFloatBottom=0)  Line 1334	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=)  Line 660	C++
 	WebKit.dll!WebCore::RenderObject::setAnimatableStyle(WTF::PassRefPtr<WebCore::RenderStyle> style={...})  Line 2209	C++
 	WebKit.dll!WebCore::RenderView::pushLayoutState(WebCore::RenderBox * renderer=0x7f473110, const WebCore::IntSize & offset={...})  Line 112 + 0x31 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutPositionedObjects(bool relayoutChildren=true)  Line 1435 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=)  Line 587 + 0xf bytes	C++
>	WebKit.dll!std::_Pop_heap<WebCore::TimerHeapIterator,int,WebCore::TimerHeapElement>(WebCore::TimerHeapIterator _First={...}, WebCore::TimerHeapIterator _Last={...}, WebCore::TimerHeapIterator _Dest={...}, WebCore::TimerHeapElement _Val={...}, int * __formal=0x100dab20)  Line 2096 + 0x24 bytes	C++
 	WebKit.dll!WebCore::ScrollView::visibleContentRect(bool includeScrollbars=)  Line 172 + 0x77 bytes	C++
 	WebKit.dll!WebCore::RenderView::viewHeight()  Line 564	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 571	C++
 	WebKit.dll!WebCore::RenderView::layout()  Line 121	C++
 	WebKit.dll!WebCore::FrameView::layout(bool allowSubtree=true)  Line 562	C++
 	WebKit.dll!WebCore::Document::updateLayout()  Line 1209 + 0x9 bytes	C++
 	WebKit.dll!WebCore::Document::updateLayoutIgnorePendingStylesheets()  Line 1242	C++
 	WebKit.dll!WebCore::Element::offsetWidth()  Line 293	C++
 	WebKit.dll!WebCore::jsElementOffsetWidth(JSC::ExecState * exec=0x7fb6813c, const JSC::Identifier & __formal={...}, const JSC::PropertySlot & slot={...})  Line 212 + 0x8 bytes	C++
 	WebKit.dll!JSC::JSValue::get(JSC::ExecState * exec=0x7fb6813c, const JSC::Identifier & propertyName={...}, JSC::PropertySlot & slot={...})  Line 485 + 0x12 bytes	C++
 	WebKit.dll!JSC::Interpreter::cti_op_get_by_id_second(void * * args=0x00000000)  Line 4519	C++
 	WebKit.dll!JSC::Interpreter::execute(JSC::FunctionBodyNode * functionBodyNode=0x7f1b0e70, JSC::ExecState * callFrame=0x7f651c44, JSC::JSFunction * function=0x05e9c620, JSC::JSObject * thisObj=0x7f27c3c0, const JSC::ArgList & args={...}, JSC::ScopeChainNode * scopeChain=0x7f602948, JSC::JSValue * * exception=0x7fec1b88)  Line 980	C++
 	WebKit.dll!JSC::JSFunction::call(JSC::ExecState * exec=0x00000000, JSC::JSValue * thisValue=0x00000000, const JSC::ArgList & args={...})  Line 83	C++
 	WebKit.dll!JSC::call(JSC::ExecState * exec=0x7f651c44, JSC::JSValue * functionObject=0x05e9c620, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue * thisValue=0x00000000, const JSC::ArgList & args={...})  Line 40	C++
 	WebKit.dll!WebCore::JSAbstractEventListener::handleEvent(WebCore::Event * event=, bool isWindowEvent=)  Line 115 + 0x1d bytes	C++
 	WebKit.dll!WTF::Vector<WebCore::String,0>::Vector<WebCore::String,0>(const WTF::Vector<WebCore::String,0> & other={...})  Line 568	C++
 	WebKit.dll!WebCore::EventTargetNode::handleLocalEvents(WebCore::Event * event=0x7f266ea0, bool useCapture=false)  Line 219 + 0xf bytes	C++
 	WebKit.dll!WebCore::EventTargetNode::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event> prpEvent={...}, int & ec=-661981563)  Line 353	C++
 	WebKit.dll!WebCore::EventTargetNode::dispatchEvent(WTF::PassRefPtr<WebCore::Event> e={...}, int & ec=0)  Line 273 + 0xb bytes	C++
 	WebKit.dll!WebCore::EventTargetNode::dispatchMouseEvent(const WebCore::AtomicString & eventType={...}, int button=0, int detail=1, int pageX=692, int pageY=166, int screenX=1462, int screenY=263, bool ctrlKey=false, bool altKey=false, bool shiftKey=false, bool metaKey=false, bool isSimulated=false, WebCore::Node * relatedTargetArg=0x00000000, WTF::PassRefPtr<WebCore::Event> underlyingEvent={...})  Line 581	C++
 	WebKit.dll!WebCore::EventTargetNode::dispatchMouseEvent(const WebCore::PlatformMouseEvent & event={...}, const WebCore::AtomicString & eventType={...}, int detail=1, WebCore::Node * relatedTarget=0x00000000)  Line 490	C++
 	WebKit.dll!WebCore::EventHandler::dispatchMouseEvent(const WebCore::AtomicString & eventType={...}, WebCore::Node * targetNode=0x7f3fc0c0, bool cancelable=true, int clickCount=1, const WebCore::PlatformMouseEvent & mouseEvent={...}, bool setUnder=false)  Line 1566 + 0x28 bytes	C++
 	WebKit.dll!WebCore::EventHandler::handleMouseReleaseEvent(const WebCore::PlatformMouseEvent & mouseEvent=)  Line 1297 + 0x24 bytes	C++
 	WebKit.dll!WebView::handleMouseEvent(unsigned int message=0, unsigned int wParam=0, long lParam=10879668)  Line 1302	C++
 	WebKit.dll!WebViewWndProc(HWND__ * hWnd=, unsigned int message=, unsigned int wParam=, long lParam=)  Line 1732	C++

[Robert Blaut]

Confirmed in WebKit r39524 on Mac OS X 10.5.

[Robert Blaut]

What steps will reproduce the problem?
1. Open URL http://trazi.appspot.com/?trazi=sobna+vrata
2. On top right part of google map there is button 'Povećaj kartu'
3. Click button 'Povećaj kartu'

What is the expected result?

Google map should fill almost whole document window.

What happens instead?

WebKit crashes.

[Robert Blaut]

Created attachment 26337 [details]
crash log

Reproducible in Safari 3.2.1, so not a recent regression. A short debugging session suggests that a float is deleted but not removed from the floating objects list.

[Hin-Chung Lam]

I'm looking into this bug

[Hin-Chung Lam]

Tracking the stack trace shows that RenderBlock::m_floatingObjects contains a FloatingObject referencing a deleted RenderObject in FloatingObject::m_renderer.

RenderObject::removeFromObjectList() should remove itself from referencing RenderBlock, the correct (trimmed) stack trace should be:
  WebCore::RenderBlock::removeFloatingObject(WebCore::RenderObject * o=0x06cc5c94)
  WebCore::RenderBlock::markAllDescendantsWithFloatsForLayout(WebCore::RenderObject * floatToRemove=0x06cc5c94)
  WebCore::RenderObject::removeFromObjectLists()

However RenderBlock::removeFloatingObject didn't get called from RenderBlock::markAllDescendantsWithFloatsForLayout, the reason being that child->isFloatingOrPositioned() is true for a child actually containing floatToRemove, child->isPositioned() is true while floatToRemove is referenced as a floating object in one of its descendants.

Tracing into how the RenderObject is referenced as a floating object in RenderBlock shows the following (trimmed) stack trace:
  WebCore::RenderBlock::addIntrudingFloats(WebCore::RenderBlock * prev=0x06fa3fec, int xoff=0, int yoff=0)
  WebCore::RenderBlock::clearFloats()
  WebCore::RenderBlock::layoutBlock(bool relayoutChildren=true)
  WebCore::RenderBlock::layout()

Looking into RenderBlock::clearFloats shows that inserting into m_floatingObjects is guarded when isFloatingOrPositioned() returns true. So this is what happened:

1. RenderBlock::addIntrudingFloats adds the RenderObject to RenderBlock::m_floatingObjects while isPositioned() = false
2. A javascript callback is invoked by WebCore::XMLHttpRequest that sets position to absolute, i.e. isPositioned() = true for that particular object
3. Pressing the button triggers RenderObject::removeFromObjectList() but didn't get removed because the isPositioned() for a RenderBlock that contains it is true
4. References a deleted floating object and it crashes

A feasible solution would be changing child->isFloatingOrPositioned() to child->isFloating() inside RenderBlock::markAllDescendantsWithFloatsForLayout. The change fixed the crash and didn't break layout tests, don't know the performance impact though.

[Hin-Chung Lam]

Created attachment 26451 [details]
Remove the check for positioned object at RenderBlock::markAllDescendantsWithFloatsForLayout

[Hin-Chung Lam]

Created attachment 26521 [details]
corrected the incorrect if statement in RebderBlock::markAllDescendantsWithFloatsForLayout

I couldn't produce a standalone test case for this bug..

[Hin-Chung Lam]

Created attachment 26539 [details]
fix the crash for referencing deleted floating objects

removed tabs in ChangeLog

[Darin Adler]

Comment on attachment 26539 [details]
fix the crash for referencing deleted floating objects

The fix looks great. Normally we require a regression test with every bug fix. How did you discover this bug? Can you write a regression test for it?

> +        Reviewed by NOBODY (OOPS!).
> +
> +       https://bugs.webkit.org/show_bug.cgi?id=23024
> +
> +       Remove an incorrect criteria in the if statement that leaves references to deleted floating object in RenderBlock.

These aren't indented right. Need 8 spaces instead of 7.

I'm going to say review- because of the lack of a regression test.

[Hin-Chung Lam]

Created attachment 26809 [details]
crash sample

I'm able to trigger the bug but this test case. I was looking into stack traces to have found the bug.

[Hin-Chung Lam]

Created attachment 26812 [details]
patch with regression test

[Alexey Proskuryakov]

See also: <http://trac.webkit.org/projects/webkit/changeset/40313>

[Hin-Chung Lam]

This particular crash still happens after applying the patch below.

(In reply to comment #13)
> See also: <http://trac.webkit.org/projects/webkit/changeset/40313>
> 

[Holger Freyther]

Would it be possible to dump the result as text? I don't think that dumping the result as text would weaken the test.

[Hin-Chung Lam]

Created attachment 27501 [details]
bug fix + regression test

[Hin-Chung Lam]

Created attachment 27502 [details]
bug fix + regression test

[Hin-Chung Lam]

Created attachment 27504 [details]
bug fix + regression test

[Hin-Chung Lam]

Created attachment 27505 [details]
bug fix + test

[David Levin]

Comment on attachment 27505 [details]
bug fix + test

Minor style issues.  New patch in progress.

[Hin-Chung Lam]

Created attachment 27507 [details]
bug fix + test

[Hin-Chung Lam]

Created attachment 27508 [details]
bug fix + test

[Dave Hyatt]

Comment on attachment 27508 [details]
bug fix + test

I think the layout test here should not just be a text dump.  We want to see the correct rendering resulting from the dynamic style change.  Can you tweak the test to generate full results? Thanks!  Patch looks fine.

[Dave Hyatt]

Is it possible to make the same thing happen using "float" as well?  I'm just wondering if the entire check should be removed.

[Hin-Chung Lam]

Created attachment 28335 [details]
bug fix and test

[Hin-Chung Lam]

Created attachment 28336 [details]
small bug fix and test

[Hin-Chung Lam]

Comment on attachment 28336 [details]
small bug fix and test

I have made the test to dump a full tree. Also the whole check of child->isFloatingOrPositioned() is erroneous.

[Dave Hyatt]

Comment on attachment 28336 [details]
small bug fix and test

I'm concerned about the performance implications of simply removing this check, especially in the case where no floatToRemove is passed in.  I'm not convinced the check is wrong in the case where no floatToRemove is passed in, since in that case the function is being called from places like block layout, where float/position status hasn't been changing.

Also your layout test seems dependent on a timeout without waiting until done, so you need to fix the test to only move on after the timeout has happened.

[Hin-Chung Lam]

Created attachment 28347 [details]
bug fix and test

Fixed the regression test. Also make sure child->isFloatingOrPositioned() is used if floatToRemove is NULL.

[Hin-Chung Lam]

Created attachment 28349 [details]
patch and test

[Hin-Chung Lam]

Created attachment 28350 [details]
patch and test

[Hin-Chung Lam]

Created attachment 28351 [details]
patch + test

Fixing some style problems in the patch. Added a criteria to skip the if statement. Also updated the test.

[Hin-Chung Lam]

Created attachment 28352 [details]
patch + test

fixing style in test..

[Dave Hyatt]

Comment on attachment 28352 [details]
patch + test

r=me

[David Levin]

Assigned to levin for landing.

[David Levin]

Committed in r41537.

[Darin Fisher (:fishd, Google)]

pixel results are missing for this test.  but, perhaps it should really be using dumpAsText since the test is only to verify that we don't crash?

[Hin-Chung Lam]

I have updated the test and results in another CL:
https://bugs.webkit.org/show_bug.cgi?id=24721