Bug 191473 - [css-grid] Crash on debug changing the style of a positioned element
Summary: [css-grid] Crash on debug changing the style of a positioned element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-09 09:49 PST by Javier Fernandez
Modified: 2018-12-05 00:18 PST (History)
11 users (show)

See Also:


Attachments
Test case to reproduce the issue (345 bytes, text/html)
2018-11-09 09:49 PST, Javier Fernandez
no flags Details
Patch (3.99 KB, patch)
2018-11-09 16:43 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2018-11-09 17:08 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.43 MB, application/zip)
2018-11-09 18:04 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (3.56 MB, application/zip)
2018-11-09 19:23 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (4.02 MB, application/zip)
2018-11-09 19:50 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (5.59 MB, application/zip)
2018-11-09 22:35 PST, Build Bot
no flags Details
Patch (4.00 KB, patch)
2018-11-10 15:07 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Test reduction (166 bytes, text/html)
2018-11-10 15:09 PST, zalan
no flags Details
Patch (4.08 KB, patch)
2018-11-11 08:07 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2018-12-04 05:04 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2018-12-04 07:16 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2018-12-04 07:27 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2018-12-04 13:46 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2018-11-09 09:49:30 PST
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
Comment 1 Javier Fernandez 2018-11-09 16:43:18 PST
Created attachment 354414 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-11-09 16:46:14 PST
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()
Comment 3 Javier Fernandez 2018-11-09 17:06:50 PST
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
Comment 4 Javier Fernandez 2018-11-09 17:08:33 PST
Created attachment 354419 [details]
Patch
Comment 5 Build Bot 2018-11-09 18:04:06 PST
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
Comment 6 Build Bot 2018-11-09 18:04:08 PST
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
Comment 7 Build Bot 2018-11-09 19:23:38 PST
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
Comment 8 Build Bot 2018-11-09 19:23:39 PST
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
Comment 9 Build Bot 2018-11-09 19:50:26 PST
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
Comment 10 Build Bot 2018-11-09 19:50:28 PST
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
Comment 11 Build Bot 2018-11-09 22:35:04 PST
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
Comment 12 Build Bot 2018-11-09 22:35:06 PST
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
Comment 13 Javier Fernandez 2018-11-10 15:07:32 PST
Created attachment 354476 [details]
Patch
Comment 14 zalan 2018-11-10 15:09:10 PST
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.
Comment 15 zalan 2018-11-10 15:09:40 PST
Created attachment 354477 [details]
Test reduction
Comment 16 zalan 2018-11-10 15:48:05 PST
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.
Comment 17 Javier Fernandez 2018-11-11 07:32:39 PST
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)
Comment 18 Javier Fernandez 2018-11-11 07:51:17 PST
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 ?
Comment 19 Javier Fernandez 2018-11-11 08:07:42 PST
Created attachment 354504 [details]
Patch
Comment 20 zalan 2018-11-12 09:52:29 PST
> 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.
Comment 21 Javier Fernandez 2018-11-13 03:06:22 PST
(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.
Comment 22 zalan 2018-12-01 15:35:16 PST
(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).
Comment 23 Javier Fernandez 2018-12-04 04:30:16 PST
The approach described in comment #22 makes sense. thanks for suggesting it. I'll evaluate it and prepare a patch.
Comment 24 Javier Fernandez 2018-12-04 05:04:57 PST
Created attachment 356485 [details]
Patch
Comment 25 Javier Fernandez 2018-12-04 06:14:35 PST
Comment on attachment 356485 [details]
Patch

This patch doesn't solve the issue actually.
Comment 26 zalan 2018-12-04 06:36:26 PST
(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).
Comment 27 zalan 2018-12-04 06:40:22 PST
(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.
Comment 28 Javier Fernandez 2018-12-04 07:16:11 PST
Created attachment 356493 [details]
Patch
Comment 29 Javier Fernandez 2018-12-04 07:17:41 PST
(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.
Comment 30 Javier Fernandez 2018-12-04 07:27:08 PST
Created attachment 356496 [details]
Patch

Patch rebased
Comment 31 zalan 2018-12-04 09:51:12 PST
Please fix the ChangeLog entry before landing.
"When an box becomes non-inline,"
There's no inline box there.
Comment 32 Javier Fernandez 2018-12-04 13:46:14 PST
Created attachment 356526 [details]
Patch
Comment 33 WebKit Commit Bot 2018-12-05 00:17:20 PST
Comment on attachment 356526 [details]
Patch

Clearing flags on attachment: 356526

Committed r238888: <https://trac.webkit.org/changeset/238888>
Comment 34 WebKit Commit Bot 2018-12-05 00:17:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2018-12-05 00:18:41 PST
<rdar://problem/46479631>