Bug 191473 – [css-grid] Crash on debug changing the style of a positioned element

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Javier Fernandez]

Created attachment 354352 [details]
Test case to reproduce the issue

Load the attached test case. The browser crashes with the following backtrace:


ASSERTION FAILED: m_gridItemArea.contains(&item)
#0  WTF::jscSignalHandler (sig=1, info=0xffffffff, ucontext=0x7f5c17d19540) at ../../Source/WTF/wtf/threads/Signals.cpp:285
#1  <signal handler called>
#2  0x00007f5c19795a6a in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:255
#3  0x00007f5c298992bf in WebCore::Grid::gridItemArea (this=0x7f4795e006f0, item=...) at ../../Source/WebCore/rendering/Grid.cpp:92
#4  0x00007f5c29899715 in WebCore::Grid::gridItemSpan (this=0x7f4795e006f0, gridItem=..., direction=WebCore::ForColumns) at ../../Source/WebCore/rendering/Grid.cpp:145
#5  0x00007f5c29a0865c in WebCore::RenderGrid::gridAreaBreadthForChildIncludingAlignmentOffsets (this=0x7f4795e00600, child=..., direction=WebCore::ForColumns)
    at ../../Source/WebCore/rendering/RenderGrid.cpp:942
#6  0x00007f5c29a08160 in WebCore::RenderGrid::layoutGridItems (this=0x7f4795e00600) at ../../Source/WebCore/rendering/RenderGrid.cpp:870
#7  0x00007f5c29a04e33 in WebCore::RenderGrid::layoutBlock (this=0x7f4795e00600, relayoutChildren=false) at ../../Source/WebCore/rendering/RenderGrid.cpp:275
#8  0x00007f5c298f14db in WebCore::RenderBlock::layout (this=0x7f4795e00600) at ../../Source/WebCore/rendering/RenderBlock.cpp:600
#9  0x00007f5c298a4719 in WebCore::RenderElement::layoutIfNeeded (this=0x7f4795e00600) at ../../Source/WebCore/rendering/RenderElement.h:123
#10 0x00007f5c2994baf3 in WebCore::RenderBlockFlow::layoutLineBoxes (this=0x7f47bee007a8, relayoutChildren=false, repaintLogicalTop=..., repaintLogicalBottom=...)
    at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1708
#11 0x00007f5c29902f1d in WebCore::RenderBlockFlow::layoutInlineChildren (this=0x7f47bee007a8, relayoutChildren=false, repaintLogicalTop=..., repaintLogicalBottom=...)
    at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:672
#12 0x00007f5c299022c0 in WebCore::RenderBlockFlow::layoutBlock (this=0x7f47bee007a8, relayoutChildren=false, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:503
#13 0x00007f5c298f14db in WebCore::RenderBlock::layout (this=0x7f47bee007a8) at ../../Source/WebCore/rendering/RenderBlock.cpp:600
#14 0x00007f5c299032c5 in WebCore::RenderBlockFlow::layoutBlockChild (this=0x7f5c00700768, child=..., marginInfo=..., previousFloatLogicalBottom=..., maxFloatLogicalBottom=...)
    at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:730
#15 0x00007f5c29902e29 in WebCore::RenderBlockFlow::layoutBlockChildren (this=0x7f5c00700768, relayoutChildren=false, maxFloatLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:653
#16 0x00007f5c299022e4 in WebCore::RenderBlockFlow::layoutBlock (this=0x7f5c00700768, relayoutChildren=false, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:505
#17 0x00007f5c298f14db in WebCore::RenderBlock::layout (this=0x7f5c00700768) at ../../Source/WebCore/rendering/RenderBlock.cpp:600
#18 0x00007f5c29b50c52 in WebCore::RenderView::layout (this=0x7f5c00700768) at ../../Source/WebCore/rendering/RenderView.cpp:241

[Javier Fernandez]

Created attachment 354414 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 354414 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        mark the RenderGrid as dirty, since it become a grid item and the grid plamement logic has to be

"plamement"

> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:12
> +cell.setAttribute("class", "absolutelyPositioned");

cell.classList.add()

> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:14
> +window.scrollBy(98, 28);

Why the scroll?

> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:15
> +cell.setAttribute("class", "nonExistent");

cell.classList.remove()

[Javier Fernandez]

Comment on attachment 354414 [details]
Patch

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

>> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:12
>> +cell.setAttribute("class", "absolutelyPositioned");
> 
> cell.classList.add()

Done.

>> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:14
>> +window.scrollBy(98, 28);
> 
> Why the scroll?

It was just a way to force a layout before the style change, but I'll use an 'offsetLeft' call instead to avoid confusion.

>> LayoutTests/fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html:15
>> +cell.setAttribute("class", "nonExistent");
> 
> cell.classList.remove()

Done

[Javier Fernandez]

Created attachment 354419 [details]
Patch

[EWS Watchlist]

Comment on attachment 354419 [details]
Patch

Attachment 354419 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9932316

New failing tests:
fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html

[EWS Watchlist]

Created attachment 354422 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354419 [details]
Patch

Attachment 354419 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9932641

New failing tests:
fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html

[EWS Watchlist]

Created attachment 354429 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354419 [details]
Patch

Attachment 354419 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9933277

New failing tests:
fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html

[EWS Watchlist]

Created attachment 354432 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354419 [details]
Patch

Attachment 354419 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9934629

New failing tests:
fast/css-grid-layout/grid-crash-out-of-flow-positioned-element.html

[EWS Watchlist]

Created attachment 354443 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 354476 [details]
Patch

[alan]

Here is a test reduction:
<div style="display: grid;">foo<div id=foobar style="position: absolute"></div></div>
<script>
document.body.offsetLeft;
foobar.style.position = "static";
</script>
What happens here is as follows:
Render tree before the style change ->
B---YGL- -+  RenderView at (0,0) size 903x0 renderer->(0x12a3b0000) layout->[normal child][positioned child]
B-----L- -+    HTML RenderBlock at (0,0) size 903x0 renderer->(0x12a3456c0) node->(0x12a3ef888) layout->[self][normal child]
B------- -+      BODY RenderBody at (0,8) size 887x0 renderer->(0x1317f9000) node->(0x12a3ef958) layout->[self][normal child]
B------- -+        DIV RenderGrid at (0,0) size 887x0 renderer->(0x12a3b0600) node->(0x12a3ef9c0) layout->[self][normal child]
B---YG-- -+*         RenderBlock at (0,0) size 0x0 renderer->(0x1317f9120) layout->[self][normal child]
I------- -+            #text RenderText renderer->(0x12a331d00) node->(0x12a3abd20) length->(3) "foo" layout->[self]
BA----L- -+            DIV RenderBlock at (0,0) size 0x0 renderer->(0x1317f9240) node->(0x12a3ef410) layout->[self]

and after the style change:
B---YGL- -+  RenderView at (0,0) size 903x0 renderer->(0x12a3b0000) layout->[normal child][positioned child]
B-----L- -+    HTML RenderBlock at (0,0) size 903x0 renderer->(0x12a3456c0) node->(0x12a3ef888) layout->[normal child]
B------- -+      BODY RenderBody at (8,8) size 887x0 renderer->(0x1317f9000) node->(0x12a3ef958) layout->[normal child]
B------- -+        DIV RenderGrid at (0,0) size 887x0 renderer->(0x12a3b0600) node->(0x12a3ef9c0) layout->[normal child]
B---YG-- -+*         RenderBlock at (0,0) size 0x0 renderer->(0x1317f9360) layout->[self][normal child]
I------- -+            #text RenderText renderer->(0x12a331d00) node->(0x12a3abd20) length->(3) "foo" layout->[self]
B------- -+          DIV RenderBlock at (8,26) size 0x0 renderer->(0x1317f9240) node->(0x12a3ef410) layout->[self]

When the "foobar" div is no longer out-of-flow positioned, it becomes a direct in-flow child of the grid container -> a grid item.
You just have to make sure that your cached is updated whenever the grid container gains a new direct in-flow child.

[alan]

Created attachment 354477 [details]
Test reduction

[alan]

This is essentially about: childFlowStateChangesAndAffectsParentBlock() when the child becomes in-flow, it starts "affecting" the parent block (grid container).
I bet when the opposite flow happens (going from inflow to out-of-flow), the grid cache becomes stale the same way.

[Javier Fernandez]

We already have a few Web Platform Tests tests to cover the case of style changes affecting the in-flow/out-of-flow position:

abspos/absolute-positioning-changing-containing-block-001.html    
abspos/grid-item-absolute-positioning-dynamic-001.html

We added some logic in the RenderBox::styleDidChange to deal precisely with this issue, so that the grid is marked as dirty if a box changes from/to absolute positioning (updateGridPositionAfterStyleChange() function)


The problem is that we only do this for box which parent is a Grid container. It doesn't need to be a grid item, actually, but we considered that such style change would only affect the grid if the box's parent is a Grid container.

In the case we are considering now, we don't run the logic mentioned above because the box becoming a grid item was not a direct children of the grid, but it's added as a grid container children (as a grid item, but that's not relevant for this issue) when it becomes in-flow.

In my opinion the root cause of the issue is that the out-of-flow box is not inserted in the render tree as a direct child of the grid container (this is Blink's current behavior, BTW)

[Javier Fernandez]

Regarding keeping the grid items cache updated, we would need to detect render tree changes that may affect the grid container. It seems that now we have the following APIs for this purpose:

 - RenderTreeBuilder::attachToRenderGrid
 - RenderTreeBuilder::detachToRenderGrid
   
However, in this case it seems that the mentioned style changes, making an out-of-flow box become in-flow, triggers the execution of moveAllChildrenToInternal function, which effectively changes the render tree structure. This is a different point of failure for keeping update the grid items cache. See the following backtrace to illustrate how we get here:


#0  WebCore::RenderElement::styleWillChange (this=0x7f5f65e009d8, diff=WebCore::StyleDifference::NewStyle, newStyle=...) at ../../Source/WebCore/rendering/RenderElement.cpp:704
#1  0x00007f5ff09ddac8 in WebCore::RenderLayerModelObject::styleWillChange (this=0x7f5f65e009d8, diff=WebCore::StyleDifference::NewStyle, newStyle=...)
    at ../../Source/WebCore/rendering/RenderLayerModelObject.cpp:151
#2  0x00007f5ff08c2d55 in WebCore::RenderBox::styleWillChange (this=0x7f5f65e009d8, diff=WebCore::StyleDifference::NewStyle, newStyle=...) at ../../Source/WebCore/rendering/RenderBox.cpp:291
#3  0x00007f5ff086371f in WebCore::RenderBlock::styleWillChange (this=0x7f5f65e009d8, diff=WebCore::StyleDifference::NewStyle, newStyle=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:418
#4  0x00007f5ff087c19c in WebCore::RenderBlockFlow::styleWillChange (this=0x7f5f65e009d8, diff=WebCore::StyleDifference::NewStyle, newStyle=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:2053
#5  0x00007f5ff092bc75 in WebCore::RenderElement::initializeStyle (this=0x7f5f65e009d8) at ../../Source/WebCore/rendering/RenderElement.cpp:382
#6  0x00007f5ff086ffd4 in WebCore::RenderBlock::createAnonymousBlockWithStyleAndDisplay (document=..., style=..., display=WebCore::DisplayType::Block) at ../../Source/WebCore/rendering/RenderBlock.cpp:2889
#7  0x00007f5ff088cede in WebCore::RenderBlock::createAnonymousBlock (this=0x7f5f65e007a8, display=WebCore::DisplayType::Block) at ../../Source/WebCore/rendering/RenderBlock.h:542
#8  0x00007f5ff0c1d3c5 in WebCore::RenderTreeBuilder::makeChildrenNonInline (this=0x7fff124b5cd8, parent=..., insertionPoint=0x0) at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:597
#9  0x00007f5ff0c1f327 in WebCore::RenderTreeBuilder::Block::childBecameNonInline (this=0x7f5f660dc6b8, parent=...) at ../../Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp:250
#10 0x00007f5ff0c1d7a1 in WebCore::RenderTreeBuilder::childFlowStateChangesAndAffectsParentBlock (this=0x7fff124b5cd8, child=...) at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:653
#11 0x00007f5ff0c1d183 in WebCore::RenderTreeBuilder::normalizeTreeAfterStyleChange (this=0x7fff124b5cd8, renderer=..., oldStyle=...) at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:551
#12 0x00007f5ff0c36b3f in WebCore::RenderTreeUpdater::updateRendererStyle (this=0x7fff124b5cb0, renderer=..., newStyle=..., minimalStyleDifference=WebCore::StyleDifference::Equal)
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:305
#13 0x00007f5ff0c36efd in WebCore::RenderTreeUpdater::updateElementRenderer (this=0x7fff124b5cb0, element=..., update=...) at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:361
#14 0x00007f5ff0c36498 in WebCore::RenderTreeUpdater::updateRenderTree (this=0x7fff124b5cb0, root=...) at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:202
#15 0x00007f5ff0c35ed0 in WebCore::RenderTreeUpdater::commit (this=0x7fff124b5cb0, styleUpdate=std::unique_ptr<const WebCore::Style::Update> = {...})
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:134
#16 0x00007f5fefb85ce8 in WebCore::Document::resolveStyle (this=0x7f5f67801fc0, type=WebCore::Document::ResolveStyleType::Normal) at ../../Source/WebCore/dom/Document.cpp:1924
#17 0x00007f5fefb864c7 in WebCore::Document::updateStyleIfNeeded (this=0x7f5f67801fc0) at ../../Source/WebCore/dom/Document.cpp:2031


Why we are processing this logic now ? Was the out-of-flow box an inline element before ?

[Javier Fernandez]

Created attachment 354504 [details]
Patch

[alan]

> In my opinion the root cause of the issue is that the out-of-flow box is not
> inserted in the render tree as a direct child of the grid container (this is
> Blink's current behavior, BTW)
While I agree that the out-of-flow box position is not optimal, its current placement is not incorrect either. I'd rather fix the missing cache invalidation path since there might be other cases when this type of mutation happens.

[Javier Fernandez]

(In reply to zalan from comment #20)
> > In my opinion the root cause of the issue is that the out-of-flow box is not
> > inserted in the render tree as a direct child of the grid container (this is
> > Blink's current behavior, BTW)
> While I agree that the out-of-flow box position is not optimal, its current
> placement is not incorrect either. I'd rather fix the missing cache
> invalidation path since there might be other cases when this type of
> mutation happens.

The current patch tries to fix the cache invalidation issue by detecting a render tree change affecting a grid container. This is done in the  moveAllChildrenToInternal function, which i think is the only place where we can figure out the kind of the new parent. Are your comments suggesting that there is a better place to do it ? 

In your previous comment #16 you mentioned  childFlowStateChangesAndAffectsParentBlock as a possible responsible of this cache invalidation issue. However, at that point we don't know yet the new parent, do we ? I tried to implement a patch to invalidate the cache there (see attachment #354504 [details]) but without success, so far.

[alan]

(In reply to Javier Fernandez from comment #21)
> (In reply to zalan from comment #20)
> > > In my opinion the root cause of the issue is that the out-of-flow box is not
> > > inserted in the render tree as a direct child of the grid container (this is
> > > Blink's current behavior, BTW)
> > While I agree that the out-of-flow box position is not optimal, its current
> > placement is not incorrect either. I'd rather fix the missing cache
> > invalidation path since there might be other cases when this type of
> > mutation happens.
> 
> The current patch tries to fix the cache invalidation issue by detecting a
> render tree change affecting a grid container. This is done in the 
> moveAllChildrenToInternal function, which i think is the only place where we
> can figure out the kind of the new parent. Are your comments suggesting that
> there is a better place to do it ? 
> 
> In your previous comment #16 you mentioned 
> childFlowStateChangesAndAffectsParentBlock as a possible responsible of this
> cache invalidation issue. However, at that point we don't know yet the new
> parent, do we ? I tried to implement a patch to invalidate the cache there
> (see attachment #354504 [details]) but without success, so far.
This should fix this issue.
diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp
index 1f9faa67f23..95a4933c3c1 100644
--- a/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp
+++ b/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp
@@ -653,6 +653,13 @@ void RenderTreeBuilder::childFlowStateChangesAndAffectsParentBlock(RenderElement
             blockBuilder().childBecameNonInline(downcast<RenderBlock>(*parent), child);
         else if (is<RenderInline>(*parent))
             inlineBuilder().childBecameNonInline(downcast<RenderInline>(*parent), child);
+
+        // childBecameNonInline might have re-parented us.
+        if (auto* parent = child.parent()) {
+            // Check if RenderGrid had gained a new grid item.
+            if (is<RenderGrid>(*parent))
+                downcast<RenderGrid>(*parent).dirtyGrid();
+        }
     } else {
         // An anonymous block must be made to wrap this inline.
         auto newBlock = downcast<RenderBlock>(*parent).createAnonymousBlock();

I prefer this to the posted patch because at childFlowStateChangesAndAffectsParentBlock() we actually have a very clear idea of what happened. A renderer went from out-of-flow to in-flow and now it "affects" its parent containing block. In moveAllChildrenToInternal() we don't have a context so it's harder to justify any kind of invalidation logic there.
(Note the "childBecameNonInline" function name is oddly misleading. This renderer has never been an inline box (out-of-flow elements get blockified).

[Javier Fernandez]

The approach described in comment #22 makes sense. thanks for suggesting it. I'll evaluate it and prepare a patch.

[Javier Fernandez]

Created attachment 356485 [details]
Patch

[Javier Fernandez]

Comment on attachment 356485 [details]
Patch

This patch doesn't solve the issue actually.

[alan]

(In reply to Javier Fernandez from comment #25)
> Comment on attachment 356485 [details]
> Patch
> 
> This patch doesn't solve the issue actually.
It did solve for me locally. (I did not hit the assertion in debug).

[alan]

(In reply to Javier Fernandez from comment #25)
> Comment on attachment 356485 [details]
> Patch
> 
> This patch doesn't solve the issue actually.
Oh, you applied the patch wrong. This fix should not go to the "else if (is<RenderInline>(*parent)) {" branch. The test reduction does not generate any RenderInlines.

[Javier Fernandez]

Created attachment 356493 [details]
Patch

[Javier Fernandez]

(In reply to zalan from comment #27)
> (In reply to Javier Fernandez from comment #25)
> > Comment on attachment 356485 [details]
> > Patch
> > 
> > This patch doesn't solve the issue actually.
> Oh, you applied the patch wrong. This fix should not go to the "else if
> (is<RenderInline>(*parent)) {" branch. The test reduction does not generate
> any RenderInlines.

Yes, I realized and submitted a new patch.

I added an additional condition, since we shouldn't have to dirty the grid if the item has not been re-parented.

[Javier Fernandez]

Created attachment 356496 [details]
Patch

Patch rebased

[alan]

Please fix the ChangeLog entry before landing.
"When an box becomes non-inline,"
There's no inline box there.

[Javier Fernandez]

Created attachment 356526 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356526 [details]
Patch

Clearing flags on attachment: 356526

Committed r238888: <https://trac.webkit.org/changeset/238888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46479631>