Description
Hin-Chung Lam
2008-12-29 11:00:04 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ć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. 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. I'm looking into this bug 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. Created attachment 26451 [details]
Remove the check for positioned object at RenderBlock::markAllDescendantsWithFloatsForLayout
Created attachment 26521 [details]
corrected the incorrect if statement in RebderBlock::markAllDescendantsWithFloatsForLayout
I couldn't produce a standalone test case for this bug..
Created attachment 26539 [details]
fix the crash for referencing deleted floating objects
removed tabs in ChangeLog
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. 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.
Created attachment 26812 [details]
patch with regression test
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> > Would it be possible to dump the result as text? I don't think that dumping the result as text would weaken the test. Created attachment 27501 [details]
bug fix + regression test
Created attachment 27502 [details]
bug fix + regression test
Created attachment 27504 [details]
bug fix + regression test
Created attachment 27505 [details]
bug fix + test
Comment on attachment 27505 [details]
bug fix + test
Minor style issues. New patch in progress.
Created attachment 27507 [details]
bug fix + test
Created attachment 27508 [details]
bug fix + test
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.
Is it possible to make the same thing happen using "float" as well? I'm just wondering if the entire check should be removed. Created attachment 28335 [details]
bug fix and test
Created attachment 28336 [details]
small bug fix and test
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 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.
Created attachment 28347 [details]
bug fix and test
Fixed the regression test. Also make sure child->isFloatingOrPositioned() is used if floatToRemove is NULL.
Created attachment 28349 [details]
patch and test
Created attachment 28350 [details]
patch and test
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.
Created attachment 28352 [details]
patch + test
fixing style in test..
Comment on attachment 28352 [details]
patch + test
r=me
Assigned to levin for landing. Committed in r41537. 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? I have updated the test and results in another CL: https://bugs.webkit.org/show_bug.cgi?id=24721 |