Bug 23024

Summary: Crash beneath addOverhangingFloats() at trazi.appspot.com
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: Layout and RenderingAssignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Critical CC: hyatt, mitz, webkit
Priority: P1 Keywords: NeedsReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://trazi.appspot.com/?trazi=sobna+vrata
Attachments:
Description Flags
crash log
none
Remove the check for positioned object at RenderBlock::markAllDescendantsWithFloatsForLayout
none
corrected the incorrect if statement in RebderBlock::markAllDescendantsWithFloatsForLayout
none
fix the crash for referencing deleted floating objects
darin: review-
crash sample
none
patch with regression test
none
bug fix + regression test
none
bug fix + regression test
none
bug fix + regression test
none
bug fix + test
none
bug fix + test
none
bug fix + test
hyatt: review-
bug fix and test
none
small bug fix and test
hyatt: review-
bug fix and test
none
patch and test
none
patch and test
none
patch + test
none
patch + test hyatt: review+

Description Hin-Chung Lam 2008-12-29 11:00:04 PST
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++
Comment 1 Robert Blaut 2008-12-31 09:12:53 PST
Confirmed in WebKit r39524 on Mac OS X 10.5.
Comment 2 Robert Blaut 2008-12-31 09:13:57 PST
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&#263;aj kartu'
3. Click button 'Pove&#263;aj kartu'

What is the expected result?

Google map should fill almost whole document window.

What happens instead?

WebKit crashes.
Comment 3 Robert Blaut 2008-12-31 09:14:19 PST
Created attachment 26337 [details]
crash log
Comment 4 mitz 2008-12-31 23:19:41 PST
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.
Comment 5 Hin-Chung Lam 2009-01-05 15:25:04 PST
I'm looking into this bug
Comment 6 Hin-Chung Lam 2009-01-05 21:52:20 PST
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.
Comment 7 Hin-Chung Lam 2009-01-05 22:05:05 PST
Created attachment 26451 [details]
Remove the check for positioned object at RenderBlock::markAllDescendantsWithFloatsForLayout
Comment 8 Hin-Chung Lam 2009-01-07 23:33:50 PST
Created attachment 26521 [details]
corrected the incorrect if statement in RebderBlock::markAllDescendantsWithFloatsForLayout

I couldn't produce a standalone test case for this bug..
Comment 9 Hin-Chung Lam 2009-01-08 14:57:40 PST
Created attachment 26539 [details]
fix the crash for referencing deleted floating objects

removed tabs in ChangeLog
Comment 10 Darin Adler 2009-01-10 14:35:50 PST
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.
Comment 11 Hin-Chung Lam 2009-01-16 14:49:21 PST
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.
Comment 12 Hin-Chung Lam 2009-01-16 15:39:18 PST
Created attachment 26812 [details]
patch with regression test
Comment 13 Alexey Proskuryakov 2009-01-28 11:58:03 PST
See also: <http://trac.webkit.org/projects/webkit/changeset/40313>
Comment 14 Hin-Chung Lam 2009-01-29 11:32:44 PST
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>
> 

Comment 15 Holger Freyther 2009-02-02 06:26:07 PST
Would it be possible to dump the result as text? I don't think that dumping the result as text would weaken the test.
Comment 16 Hin-Chung Lam 2009-02-09 16:36:14 PST
Created attachment 27501 [details]
bug fix + regression test
Comment 17 Hin-Chung Lam 2009-02-09 16:47:00 PST
Created attachment 27502 [details]
bug fix + regression test
Comment 18 Hin-Chung Lam 2009-02-09 16:53:56 PST
Created attachment 27504 [details]
bug fix + regression test
Comment 19 Hin-Chung Lam 2009-02-09 17:28:37 PST
Created attachment 27505 [details]
bug fix + test
Comment 20 David Levin 2009-02-09 17:45:03 PST
Comment on attachment 27505 [details]
bug fix + test

Minor style issues.  New patch in progress.
Comment 21 Hin-Chung Lam 2009-02-09 17:49:44 PST
Created attachment 27507 [details]
bug fix + test
Comment 22 Hin-Chung Lam 2009-02-09 17:51:31 PST
Created attachment 27508 [details]
bug fix + test
Comment 23 Dave Hyatt 2009-02-10 11:46:21 PST
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.
Comment 24 Dave Hyatt 2009-02-10 11:47:01 PST
Is it possible to make the same thing happen using "float" as well?  I'm just wondering if the entire check should be removed.

Comment 25 Hin-Chung Lam 2009-03-05 16:13:12 PST
Created attachment 28335 [details]
bug fix and test
Comment 26 Hin-Chung Lam 2009-03-05 16:15:58 PST
Created attachment 28336 [details]
small bug fix and test
Comment 27 Hin-Chung Lam 2009-03-05 16:17:21 PST
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.
Comment 28 Dave Hyatt 2009-03-05 18:52:36 PST
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.
Comment 29 Hin-Chung Lam 2009-03-05 22:04:34 PST
Created attachment 28347 [details]
bug fix and test

Fixed the regression test. Also make sure child->isFloatingOrPositioned() is used if floatToRemove is NULL.
Comment 30 Hin-Chung Lam 2009-03-05 23:24:07 PST
Created attachment 28349 [details]
patch and test
Comment 31 Hin-Chung Lam 2009-03-05 23:25:47 PST
Created attachment 28350 [details]
patch and test
Comment 32 Hin-Chung Lam 2009-03-05 23:29:00 PST
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.
Comment 33 Hin-Chung Lam 2009-03-05 23:31:16 PST
Created attachment 28352 [details]
patch + test

fixing style in test..
Comment 34 Dave Hyatt 2009-03-09 12:43:02 PDT
Comment on attachment 28352 [details]
patch + test

r=me
Comment 35 David Levin 2009-03-09 12:48:23 PDT
Assigned to levin for landing.

Comment 36 David Levin 2009-03-09 14:05:49 PDT
Committed in r41537.
Comment 37 Darin Fisher (:fishd, Google) 2009-03-20 10:07:12 PDT
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?
Comment 38 Hin-Chung Lam 2009-03-20 12:57:02 PDT
I have updated the test and results in another CL:
https://bugs.webkit.org/show_bug.cgi?id=24721