Bug 93891 – ASSERTION FAILED: !currBox->needsLayout() loading bing maps

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

I hit an assertion when loading http://binged.it/K9uLcw:


ASSERTION FAILED: !currBox->needsLayout()
/Volumes/SSData/Development/OSX/webkit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp(7431) : void WebCore::RenderBlock::checkPositionedObjectsNeedLayout()
1   0x103f70de2 WebCore::RenderBlock::checkPositionedObjectsNeedLayout()
2   0x1040b7e83 WebCore::RenderObject::checkBlockPositionedObjectsNeedLayout()
3   0x1031ef58c WebCore::RenderObject::setNeedsLayout(bool, WebCore::MarkingBehavior)
4   0x103f49f7c WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit)
5   0x103f488d5 WebCore::RenderBlock::layout()
6   0x1041c265d WebCore::RenderView::layout()
7   0x1034c71af WebCore::FrameView::layout(bool)
8   0x1031b0fce WebCore::Document::updateLayout()
9   0x1031b10a5 WebCore::Document::updateLayoutIgnorePendingStylesheets()
10  0x10334a1de WebCore::DOMWindow::scrollY() const
11  0x10396af95 WebCore::DOMWindow::pageYOffset() const
12  0x10393f0ad WebCore::jsDOMWindowPageYOffset(JSC::ExecState*, JSC::JSValue, JSC::PropertyName)
13  0x102034069 JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
14  0x1020435a2 JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
15  0x1023c52f1 llint_slow_path_get_by_id
16  0x1023ce175 llint_op_get_by_id
17  0x1021e2fd4 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
18  0x1021dfd8f JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
19  0x1020917d8 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
20  0x1038724c2 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
21  0x1042238ce WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext*)
22  0x1042234a3 WebCore::ScheduledAction::execute(WebCore::Document*)
23  0x1042232f4 WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext*)
24  0x10333acb9 WebCore::DOMTimer::fired()
25  0x10458734d WebCore::ThreadTimers::sharedTimerFiredInternal()
26  0x1045870e9 WebCore::ThreadTimers::sharedTimerFired()
27  0x1042cc3e3 WebCore::timerFired(__CFRunLoopTimer*, void*)

[alan]

Created attachment 164312 [details]
reduced test case

[alan]

1, When the div's position attribute is changed from fixed to absolute (c.style.position = "absolute";), it first gets removed and then added back to gPositionedDescendantsMap ListHashSet (RenderBlock::removePositionedObject/insertPositionedObject). 
2, With nested positioned divs, after the position attribute change of the outer div (remove-insert), the inner div gets in front of the outer in the positionedDescendatnsMap. 
3, During a subsequent layout, as RenderBlock::layoutPositionedObjects() iterates through the positioned objects, it first lays out the inner div followed by the outer div.
4, The inner div is marked dirty at RenderBlock::layoutInlineChildren(), while the outer div is being layed out.
5, RenderBlock::layoutPositionedObjects() leaves with the inner div marked dirty.
->ASSERT.

Not sure why we are marking the fixed positioned inner div dirty, when only the outer (non containing) div is changed (but not the containing block), but the actual fix I think is to make sure the ListHashSet of the positioned objects is in the right order. Proposed fix is coming.

[alan]

Created attachment 164314 [details]
Patch

[alan]

alternatively we could limit this change to positioned objects only (gPositionedDescendantsMap) instead of every TrackedRendererListHashSet object.

[Abhishek Arya]

This looks like a regression from http://trac.webkit.org/changeset/125351 when we removed m_positionedObjects from every renderblock, saving 4 bytes, but looks like we need to take care of the order.

[Abhishek Arya]

(In reply to comment #5)
> This looks like a regression from http://trac.webkit.org/changeset/125351 when we removed m_positionedObjects from every renderblock, saving 4 bytes, but looks like we need to take care of the order.

Ok, i take that back, the order wasn't enforced even before r125351 (so not a regression). Also, it is not required for percent height descendant sharing the same code - http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp&exact_package=chromium&q=file:webkit%20RenderBlock.cpp&type=cs&l=2325

[Kenneth Rohde Christiansen]

Comment on attachment 164314 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        removed and then added back to the list of positioned objects (gPositionedDescendantsMap).
> +        The container's new position in the list (appended to the end) may be different from its original position.

I would try to keep these lines a bit shorter

> Source/WebCore/rendering/RenderBlock.cpp:3640
> +            RenderBox* r = *it;

box?

Maybe add a comment like // Insert parent before child, as layoutPositionedObjects relies on that.

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash-expected.txt:4
> +
> +

Could we avoid two newlines?

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:12
> +    <script type="text/javascript">

we normally specify that it is javascript as that is default

[alan]

Created attachment 164323 [details]
Patch

[Abhishek Arya]

Comment on attachment 164323 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        RenderBlock::layoutPositionedObjects()'s logic relies on 'parent first'

You need to add a line explaining why it is ''parent first' ordering and how stuff is marked in RenderBlock::layoutInlineChildren.

> Source/WebCore/rendering/RenderBlock.cpp:3636
> +    bool didInsertDescendant = false;

Move this line after the comment below.

> Source/WebCore/rendering/RenderBlock.cpp:3639
> +    if (descendant->isRenderBlock()) {

I dont think you need to add this virtual call. We want to keep the parent-child order in all cases.

> Source/WebCore/rendering/RenderBlock.cpp:3644
> +                didInsertDescendant = descendantSet->insertBefore(box, descendant).isNewEntry;

didInsertDescendant will always be true since you bail out on 'descendantSet->contains(descendant)' above. So, no need of the isNewEntry check and assert below.

> LayoutTests/ChangeLog:3
> +        ASSERTION FAILED: !currBox->needsLayout() loading bing maps

Please move this assertion failure to the description and keep the same title as webcore/changelog here.

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:1
> +<html>

Please declare a !DOCTYPE

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:3
> +    <div id="toabsolute" style="position: fixed;">

instead of toabsolute, please use a good identifier like 'container'

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:14
> +         var m = document.getElementById("foo");

there is no correlation b/w m and foo, please use a better identifier.

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:15
> +         var c = document.getElementById("toabsolute");

instead of 'c', just use 'container'

> LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html:20
> +         c.offsetHeight;

This layout call is not needed, it should be automatic.

[Abhishek Arya]

Also, the loop is pretty inefficient [99% of the time we are adding descendants at end], you should reverse it and check if the new box is a descendant of the last box in the chain, so basically an insertafter loop.

[alan]

(In reply to comment #10)
> Also, the loop is pretty inefficient [99% of the time we are adding descendants at end], you should reverse it and check if the new box is a descendant of the last box in the chain, so basically an insertafter loop.
Thanks for looking at the code. It is indeed highly inefficient, but probably because of the nature of the positioned elements, most of the inserts dont have parent-child relationship (100 to 1 according to my quick test on some major sites). 
So checking whether the last box is a parent unfortunately does not help to make it run faster either. (unless i misunderstood your comment)
I'll try to find a way to make it better, but given the average number of positioned elements per document, it will probably not be noticeable. (even though I agree, it is really inefficient)

[Tim Horton]

<rdar://problem/12369546>

[alan]

(In reply to comment #10)
> Also, the loop is pretty inefficient [99% of the time we are adding descendants at end], you should reverse it and check if the new box is a descendant of the last box in the chain, so basically an insertafter loop.

How about doing something like this:
Since an insert needs to be preceded by a remove, we can track whether a remove happened and just do the insertBefore only when there's been a remove. Most of the time there's no remove at all and no need to enter the loop to find the right position. It can, however, be a little bit more sophisticated than just a remove-happened-flag and track the removed node's children in a HashSet and check whether a about-to-be-added node needs to be 'inserted before' or a simple append will do.
1, have a WTF::HashMap<const RenderBlock*, HashSet<RenderBlock*>*> to track children of removed parents' (similarly to TrackedDescendantsMap)
2, when a node gets removed, check the following nodes in the HashList whether they are descendants and add them to the HashSet. Most of the time, this loop will break on the next node in the HashList and adds nothing to the HashSet.  
3,  when a new node gets added, check whether the HashSet is empty and look for children there. If a descendant is found (most likely not), then do insertBefore in the HashList instead of append.
4, when the associated RenderBlock is destroyed, delete the children HashSet too. (similarly to removeBlockFromDescendantAndContainerMaps)
(I ignored some details like when remove happens, the HashSet needs to be checked to not to end up with orphan nodes)

In best case scenario (which is mostly the case here), the overhead is a size of a (NULL) pointer, while the worst case is to double the size of gPercentHeightDescendantsMap, which is in itself is small most of the time. This brings in a little bit of a complexity, but the performance is not affected as much as with the always-loop-through solution.

However, it would great to know whether this can be fixed by not mandating the parent-child ordering. I assume if both the parent and the child are in the same positioned list, then the child's layout does not depend on the parent's layout as its containing block is the actual RenderBlock that holds the list of the positioned objects and not the parent. I might me mistaken here by ignoring some cases.

[Abhishek Arya]

(In reply to comment #13)
> (In reply to comment #10)
> > Also, the loop is pretty inefficient [99% of the time we are adding descendants at end], you should reverse it and check if the new box is a descendant of the last box in the chain, so basically an insertafter loop.
> 
> How about doing something like this:
> Since an insert needs to be preceded by a remove, we can track whether a remove happened and just do the insertBefore only when there's been a remove. Most of the time there's no remove at all and no need to enter the loop to find the right position. It can, however, be a little bit more sophisticated than just a remove-happened-flag and track the removed node's children in a HashSet and check whether a about-to-be-added node needs to be 'inserted before' or a simple append will do.
> 1, have a WTF::HashMap<const RenderBlock*, HashSet<RenderBlock*>*> to track children of removed parents' (similarly to TrackedDescendantsMap)
> 2, when a node gets removed, check the following nodes in the HashList whether they are descendants and add them to the HashSet. Most of the time, this loop will break on the next node in the HashList and adds nothing to the HashSet.  
> 3,  when a new node gets added, check whether the HashSet is empty and look for children there. If a descendant is found (most likely not), then do insertBefore in the HashList instead of append.
> 4, when the associated RenderBlock is destroyed, delete the children HashSet too. (similarly to removeBlockFromDescendantAndContainerMaps)
> (I ignored some details like when remove happens, the HashSet needs to be checked to not to end up with orphan nodes)
> 
> In best case scenario (which is mostly the case here), the overhead is a size of a (NULL) pointer, while the worst case is to double the size of gPercentHeightDescendantsMap, which is in itself is small most of the time. This brings in a little bit of a complexity, but the performance is not affected as much as with the always-loop-through solution.
> 
> However, it would great to know whether this can be fixed by not mandating the parent-child ordering. I assume if both the parent and the child are in the same positioned list, then the child's layout does not depend on the parent's layout as its containing block is the actual RenderBlock that holds the list of the positioned objects and not the parent. I might me mistaken here by ignoring some cases.

I dont think the added complexity and memory overhead is worth for this simple case. I still like your original solution but with the loop reversed should be fine here. Lets see if what other cced folks think ?

[Levi Weintraub]

(In reply to comment #14)
> I dont think the added complexity and memory overhead is worth for this simple case. I still like your original solution but with the loop reversed should be fine here. Lets see if what other cced folks think ?

This sounds like the right approach to me. I don't think the approach in c13 is warranted.

[alan]

(In reply to comment #15)
> (In reply to comment #14)
> > I dont think the added complexity and memory overhead is worth for this simple case. I still like your original solution but with the loop reversed should be fine here. Lets see if what other cced folks think ?
> 
> This sounds like the right approach to me. I don't think the approach in c13 is warranted.
Great, thanks for the feedback. I also think that the second approach introduces too much complexity with little benefit.

[Alexey Proskuryakov]

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

[Rick Byers]

Any update on this?  We've had some reports of hitting this assert in chromium (eg. https://code.google.com/p/chromium/issues/detail?id=144608).

[Dave Hyatt]

Created attachment 257378 [details]
Patch

[WebKit Commit Bot]

Attachment 257378 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1310:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1311:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1313:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1322:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1324:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 257378 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1311
> +    // objects that are positioned implicitly like this.  Such objects are rare, and so in typical DHTML menu usage (where everything is

DHTML huehue

> Source/WebCore/rendering/RenderBlock.cpp:2122
> -    }
> +    }    

Whitespace.

[Dave Hyatt]

Created attachment 257588 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 257588 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2122
> +    }    

Whitespace!

[WebKit Commit Bot]

Attachment 257588 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1310:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1311:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1313:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1322:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1324:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Fixed in r187502.