Bug 212116

Summary: Nullptr deref in WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation when parent and beforeChild are siblings
Product: WebKit Reporter: Jack <shihchieh_lee>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, esprehn+autocc, ews-feeder, ews-watchlist, ggaren, glenn, kondapallykalyan, pdr, product-security, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
zalan: review-
Test reduction
none
Patch
none
Patch none

Description Jack 2020-05-19 17:53:30 PDT
<rdar://62993844>

0   com.apple.WebCore             	0x0000000107af46f3 WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 51
1   com.apple.WebCore             	0x0000000107af3e15 WebCore::RenderTreeBuilder::Block::attach(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 421
2   com.apple.WebCore             	0x0000000107af3bfa WebCore::RenderTreeBuilder::BlockFlow::attach(WebCore::RenderBlockFlow&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 426
3   com.apple.WebCore             	0x0000000107af3836 WebCore::RenderTreeBuilder::attachInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 2278
4   com.apple.WebCore             	0x0000000107af3b89 WebCore::RenderTreeBuilder::BlockFlow::attach(WebCore::RenderBlockFlow&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 313
5   com.apple.WebCore             	0x0000000107af3836 WebCore::RenderTreeBuilder::attachInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 2278
6   com.apple.WebCore             	0x0000000107afe8bc WebCore::RenderTreeBuilder::MultiColumn::multiColumnDescendantInserted(WebCore::RenderMultiColumnFlow&, WebCore::RenderObject&) + 2476
7   com.apple.WebCore             	0x0000000107af6ae3 WebCore::RenderTreeBuilder::attachToRenderElementInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 355
8   com.apple.WebCore             	0x0000000107af4071 WebCore::RenderTreeBuilder::attachToRenderElement(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 353
9   com.apple.WebCore             	0x0000000107af48df WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 543
10  com.apple.WebCore             	0x0000000107af3e15 WebCore::RenderTreeBuilder::Block::attach(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 421
11  com.apple.WebCore             	0x0000000107af3bfa WebCore::RenderTreeBuilder::BlockFlow::attach(WebCore::RenderBlockFlow&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 426
12  com.apple.WebCore             	0x0000000107af3836 WebCore::RenderTreeBuilder::attachInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*) + 2278
13  com.apple.WebCore             	0x0000000107afba88 WebCore::RenderTreeBuilder::Inline::splitFlow(WebCore::RenderInline&, WebCore::RenderObject*, std::__1::unique_ptr<WebCore::RenderBlock, WebCore::RenderObjectDeleter>, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderBoxModelObject*) + 2200
14  com.apple.WebCore             	0x0000000107af742e WebCore::RenderTreeBuilder::normalizeTreeAfterStyleChange(WebCore::RenderElement&, WebCore::RenderStyle&) + 1166
15  com.apple.WebCore             	0x0000000107b05754 WebCore::RenderTreeUpdater::updateRendererStyle(WebCore::RenderElement&, WebCore::RenderStyle&&, WebCore::StyleDifference) + 164
16  com.apple.WebCore             	0x0000000107b0508d WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 1293
17  com.apple.WebCore             	0x0000000107b03634 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 2404
18  com.apple.WebCore             	0x00000001070fa635 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1381
19  com.apple.WebCore             	0x0000000105dd9154 WebCore::Document::updateStyleIfNeeded() + 468
20  com.apple.WebCore             	0x0000000105e4fc68 WebCore::Document::updateLayout() + 360
Comment 1 Jack 2020-05-19 17:55:14 PDT
Test case:
<style>
ol { position: fixed; }
ol, body { -webkit-columns: 2; }
</style>
<script>
function run() {
    q.appendChild(ol);
    window.requestIdleCallback(idleCallback);
}
function idleCallback() {
    range = document.createElement("main").ownerDocument.createRange();
    range.setEndBefore(li);
    range.extractContents();
}
</script>
<body onload=run()><ol id=ol><li id=li style="border-top-width: medium; column-span: all;"></ol><q id=q></q>
Comment 2 Jack 2020-05-19 18:29:36 PDT
Minimized test case:

<style>
ol { position: fixed; }
ol, body { -webkit-columns: 2; }
</style>
<script>
function run() {
    range = document.createElement("main").ownerDocument.createRange();
    range.setEndBefore(li);
    range.extractContents();
}

</script>
<body onload=run()><q><ol><li id=li style="column-span: all;"></ol></q>
Comment 3 Jack 2020-05-19 18:56:58 PDT
Cause of the crash:
1. After the document is loaded, updateStyleIfNeeded is called and render tree is being updated.
2. When walking through the nodes in updateRenderTree(), we find that OL is not inline and needs to be splitter and be attached to a RenderBlock.
3. In RenderTreeBuilder::BlockFlow::attach() we find that OL has multiColumnFlow, so we call RenderTreeBuilder::Block::attachIgnoringContinuation() and pass the RenderMultiColumnFlowThread in OL as parent and RenderMultiColumnSet as before child.
4. In Block::attachIgnoringContinuation(), we try to find the immediate child of parent by looking at ancestors of RenderMultiColumnSet. 
5. However, since RenderMultiColumnFlowThread and RenderMultiColumnSet are siblings, the logic exhausts the render tree and deref nullptr.

Step 2:
childBecameNonInline: parent=0x542fe00b0, child=0x542fe0400, beforeChild=0x542fe0c30

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLC -+  RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child]
B-----L- -+    HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child]
B------- -+      BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child]
B---YGL- -+        RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child]
I------- -+*         Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child]
I----G-- -+            <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child]
I---YG-- -+              RenderQuote renderer->(0x542fe0270) layout->[self][normal child]
I---YG-- -+                RenderText renderer->(0x542fe0360) layout->[self]
B------- -+            OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YGL- -+              RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+                RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self]
B------- -+              LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child]
I---YG-- -+                RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) layout->[self]
B---YG-- -+              RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self]
I----G-- -+            <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child]
I---YG-- -+              RenderQuote renderer->(0x542fe0d10) layout->[self][normal child]
I---YG-- -+                RenderText renderer->(0x542fe0e00) layout->[self]
I------- -+          #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self]
B---YG-- -+        RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self]

Step 3:
BlockFlow::attach: parent=0x542fe0400, child=0x542fe1a80, beforeChild=0x542fe0a90

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLC -+  RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child]
B-----L- -+    HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child]
B------- -+      BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child]
B---YGL- -+        RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1740) [Rs:0x0 Re:0x0] layout->[self][normal child]
I------- -+            Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child]
I----G-- -+              <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child]
I---YG-- -+                RenderQuote renderer->(0x542fe0270) layout->[self][normal child]
I---YG-- -+                  RenderText renderer->(0x542fe0360) layout->[self]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1610) continuation->(0x542fe19a0) [Rs:0x0 Re:0x0] layout->[self]
B------- -+            OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YGL- -+              RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+                RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self]
B------- -+              LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child]
I---YG-- -+                RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+*             RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1870) [Rs:0x0 Re:0x0] layout->[self][normal child]
I------- -+            Q RenderInline renderer->(0x542fe19a0) node->(0x542f5fcc0) layout->[self][normal child]
I----G-- -+              <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child]
I---YG-- -+                RenderQuote renderer->(0x542fe0d10) layout->[self][normal child]
I---YG-- -+                  RenderText renderer->(0x542fe0e00) layout->[self]
I------- -+            #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self]
B---YG-- -+        RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self]

Step 4:
beforeChildContainer = 0x0, parent = 0x542fe0790, beforeChild = 0x542fe0a90

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLC -+  RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child]
B-----L- -+    HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child]
B------- -+      BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child]
B---YGL- -+        RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1740) [Rs:0x0 Re:0x0] layout->[self][normal child]
I------- -+            Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child]
I----G-- -+              <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child]
I---YG-- -+                RenderQuote renderer->(0x542fe0270) layout->[self][normal child]
I---YG-- -+                  RenderText renderer->(0x542fe0360) layout->[self]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1610) continuation->(0x542fe19a0) [Rs:0x0 Re:0x0] layout->[self]
B------- -+            OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YGL- -+              RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+                RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self]
B------- -+              LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child]
I---YG-- -+                RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+*             RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+          RenderBlock at (0,0) size 0x0 renderer->(0x542fe1870) [Rs:0x0 Re:0x0] layout->[self][normal child]
I------- -+            Q RenderInline renderer->(0x542fe19a0) node->(0x542f5fcc0) layout->[self][normal child]
I----G-- -+              <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child]
I---YG-- -+                RenderQuote renderer->(0x542fe0d10) layout->[self][normal child]
I---YG-- -+                  RenderText renderer->(0x542fe0e00) layout->[self]
I------- -+            #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self]
B---YG-- -+        RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self]
Comment 4 Jack 2020-05-19 20:45:34 PDT
Created attachment 399802 [details]
Patch
Comment 5 Jack 2020-05-19 20:52:33 PDT
The patch is for discussion. In this patch, beforeChild is set to nullptr before calling m_builder.attach to avoid passing siblings as parent and beforeChild.

However, doing so triggers the assertion below:

ASSERTION FAILED: !flow.spannerMap().get(placeholder.spanner())
./rendering/updating/RenderTreeBuilderMultiColumn.cpp(270) : void WebCore::RenderTreeBuilder::MultiColumn::multiColumnDescendantInserted(WebCore::RenderMultiColumnFlow &, WebCore::RenderObject &)

If the code returns without calling m_builder.attach, there is no assertion or crash.

(In reply to Jack from comment #4)
> Created attachment 399802 [details]
> Patch
Comment 6 Geoffrey Garen 2020-05-19 21:13:30 PDT
Comment on attachment 399802 [details]
Patch

Looks like EWS tests are crashing.
Comment 7 zalan 2020-05-19 21:53:15 PDT
Comment on attachment 399802 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeBuilderBlockFlow.cpp:47
> +        if (beforeChild->parent() == &parent)
> +            beforeChild = nullptr;

This clearly needs some explanation. In many cases (probably the most common) the beforeChild's parent is the host for the insertion point.
Comment 8 Jack 2020-05-20 08:56:07 PDT
Thanks. Yeah, this patch is to demonstrate the test case. Please see the complete render tree dump in comment #3.

In this case we are passing parent.multiColumnFlow() and beforeChild to m_builder.attach.
    if (parent.multiColumnFlow() && (!parent.isFieldset() || !child->isLegend())) {
        ...

        return m_builder.attach(*parent.multiColumnFlow(), WTFMove(child), beforeChild);
    }

And the render tree show that parent.multiColumnFlow() and beforeChild are siblings.
BlockFlow::attach: parent=0x542fe0400, child=0x542fe1a80, beforeChild=0x542fe0a90
B------- -+            OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YGL- -+              RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+                RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self]
B------- -+              LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child]
I---YG-- -+                RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+*             RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self]

And when the code enters RenderTreeBuilder::Block::attachIgnoringContinuation(), the following while loop will exhaust the ancestors and crashes when beforeChildContainer becomes nullptr.
    if (beforeChild && beforeChild->parent() != &parent) {
        RenderElement* beforeChildContainer = beforeChild->parent();
        while (beforeChildContainer->parent() != &parent)
            beforeChildContainer = beforeChildContainer->parent();



(In reply to zalan from comment #7)
> Comment on attachment 399802 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399802&action=review
> 
> > Source/WebCore/rendering/updating/RenderTreeBuilderBlockFlow.cpp:47
> > +        if (beforeChild->parent() == &parent)
> > +            beforeChild = nullptr;
> 
> This clearly needs some explanation. In many cases (probably the most
> common) the beforeChild's parent is the host for the insertion point.
Comment 9 Jack 2020-05-20 09:17:44 PDT
Besides, I was wondering if RenderTreeBuilder::attachInternal should move beforeChild to the RenderMultiColumnSpannerPlaceholder since it is the child of RenderMultiColumnFlowThread?
B------- -+            OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YGL- -+              RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child]
B---YG-- -+                RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self]
B------- -+              LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child]
I---YG-- -+                RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self]
B---YG-- -+*             RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self]

However, in the following code, findColumnSpannerPlaceholder cannot find the spanner in the spannerMap so return nullptr, therefore beforeChild is not moved to spannerPlaceholder.

    } else if (is<RenderBox>(beforeChild)) {
        // Adjust the beforeChild if it happens to be a spanner and the its actual location is inside the fragmented flow.
        auto& beforeChildBox = downcast<RenderBox>(*beforeChild);
        if (auto* enclosingFragmentedFlow = parent.enclosingFragmentedFlow()) {
            auto columnSpannerPlaceholderForBeforeChild = [&]() -> RenderMultiColumnSpannerPlaceholder* {
                if (!is<RenderMultiColumnFlow>(enclosingFragmentedFlow))
                    return nullptr;
                auto& multiColumnFlow = downcast<RenderMultiColumnFlow>(*enclosingFragmentedFlow);
                return multiColumnFlow.findColumnSpannerPlaceholder(&beforeChildBox);
            };

            if (auto* spannerPlaceholder = columnSpannerPlaceholderForBeforeChild())
                beforeChild = spannerPlaceholder;
        }
    }
Comment 10 Jack 2020-05-20 09:38:42 PDT
It seems there is something wrong when we pass siblings as parent and beforeChild into RenderTreeBuilder::Block::attachIgnoringContinuation since it assumes that parent needs to be an ancestor of beforeChild, otherwise many places in that function would assert or crash.

Does this seem like a case that we need to support, or just a corner case that a patch is enough? The simplest patch is to bail out in attachIgnoringContinuation when the while loop exhaust ancestors.
Comment 11 Jack 2020-05-20 09:55:36 PDT
Created attachment 399848 [details]
Patch
Comment 12 zalan 2020-05-22 14:19:42 PDT
Comment on attachment 399848 [details]
Patch

This is papering over the real issue (not clearing the inner fragmented context when the column style gets unset).
Comment 13 zalan 2020-05-22 14:20:27 PDT
Created attachment 400075 [details]
Test reduction
Comment 14 zalan 2020-05-22 16:48:16 PDT
<rdar://problem/62993844>
Comment 15 zalan 2020-05-22 17:25:13 PDT
Created attachment 400095 [details]
Patch
Comment 16 Simon Fraser (smfr) 2020-05-22 18:42:02 PDT
Comment on attachment 400095 [details]
Patch

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

> LayoutTests/fast/multicol/nested-multicol-with-spanner-and-continuation.html:3
> +    -webkit-columns: 2;

Is the prefix required?
Comment 17 zalan 2020-05-22 19:07:05 PDT
(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 400095 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400095&action=review
> 
> > LayoutTests/fast/multicol/nested-multicol-with-spanner-and-continuation.html:3
> > +    -webkit-columns: 2;
> 
> Is the prefix required?
Yeah, certainly not Thanks.
Comment 18 zalan 2020-05-22 20:30:54 PDT
Created attachment 400105 [details]
Patch
Comment 19 EWS 2020-05-22 21:03:24 PDT
Committed r262093: <https://trac.webkit.org/changeset/262093>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400105 [details].