Skip to content

Commit 6ee8588

Browse files
author
[email protected]
committed
Clean up spanners before creating nested column context
https://bugs.webkit.org/show_bug.cgi?id=180107 <rdar://problem/35686655> Reviewed by Antti Koivisto. Source/WebCore: When an existing spanner placeholder is moved into a newly constructed (and nested) multicolumn context, we figure it's not valid anymore and end up destroying it (see RenderMultiColumnFlow::fragmentedFlowDescendantInserted). This is very unfortunate since as we climb back on the stack, we could hit this renderer as the newly inserted child. This patch proactively removes the invalid placeholders and moves the associated spanners back to their original position. Test: fast/multicol/crash-when-constructing-nested-columns.html * rendering/RenderMultiColumnFlow.h: * style/RenderTreeUpdaterMultiColumn.cpp: (WebCore::RenderTreeUpdater::MultiColumn::createFragmentedFlow): RenderTreeUpdater::MultiColumn::destroyFragmentedFlow still relies on the placeholder removal logic in RenderMultiColumnFlow::fragmentedFlowDescendantInserted. LayoutTests: * fast/multicol/crash-when-constructing-nested-columns-expected.txt: Added. * fast/multicol/crash-when-constructing-nested-columns.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@225255 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 6353138 commit 6ee8588

File tree

6 files changed

+79
-4
lines changed

6 files changed

+79
-4
lines changed

‎LayoutTests/ChangeLog

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2017-11-28 Zalan Bujtas <[email protected]>
2+
3+
Clean up spanners before creating nested column context
4+
https://bugs.webkit.org/show_bug.cgi?id=180107
5+
<rdar://problem/35686655>
6+
7+
Reviewed by Antti Koivisto.
8+
9+
* fast/multicol/crash-when-constructing-nested-columns-expected.txt: Added.
10+
* fast/multicol/crash-when-constructing-nested-columns.html: Added.
11+
112
2017-11-28 Wenson Hsieh <[email protected]>
213

314
Allow attachment elements with no appearance to defer rendering to child nodes

‎LayoutTests/fast/multicol/crash-when-constructing-nested-columns-expected.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
PASS if no crash.
2+

‎LayoutTests/fast/multicol/crash-when-constructing-nested-columns.html

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
PASS if no crash.
5+
<div style="columns: 2;"><div id=nestedColumn><div style="column-span:all;"></div></div></div>
6+
<script>
7+
if (window.testRunner)
8+
testRunner.dumpAsText();
9+
document.body.offsetTop;
10+
nestedColumn.style.columns = "2";
11+
</script>
12+
</html>

‎Source/WebCore/ChangeLog

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,28 @@
1+
2017-11-28 Zalan Bujtas <[email protected]>
2+
3+
Clean up spanners before creating nested column context
4+
https://bugs.webkit.org/show_bug.cgi?id=180107
5+
<rdar://problem/35686655>
6+
7+
Reviewed by Antti Koivisto.
8+
9+
When an existing spanner placeholder is moved into a newly constructed (and nested)
10+
multicolumn context, we figure it's not valid anymore and end up destroying it
11+
(see RenderMultiColumnFlow::fragmentedFlowDescendantInserted).
12+
This is very unfortunate since as we climb back on the stack, we could hit this renderer as
13+
the newly inserted child.
14+
15+
This patch proactively removes the invalid placeholders and moves the associated spanners back to their
16+
original position.
17+
18+
Test: fast/multicol/crash-when-constructing-nested-columns.html
19+
20+
* rendering/RenderMultiColumnFlow.h:
21+
* style/RenderTreeUpdaterMultiColumn.cpp:
22+
(WebCore::RenderTreeUpdater::MultiColumn::createFragmentedFlow):
23+
RenderTreeUpdater::MultiColumn::destroyFragmentedFlow still relies on the placeholder removal
24+
logic in RenderMultiColumnFlow::fragmentedFlowDescendantInserted.
25+
126
2017-11-28 Tim Horton <[email protected]>
227

328
REGRESSION (High Sierra): Layout Test fast/multicol/newmulticol/spanner2.html is a flaky image failure on WK1

‎Source/WebCore/rendering/RenderMultiColumnFlow.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ class RenderMultiColumnFlow final : public RenderFragmentedFlow {
9696
bool shouldCheckColumnBreaks() const override;
9797

9898
typedef HashMap<RenderBox*, WeakPtr<RenderMultiColumnSpannerPlaceholder>> SpannerMap;
99+
SpannerMap& spannerMap() { return *m_spannerMap; }
99100
std::unique_ptr<SpannerMap> takeSpannerMap() { return WTFMove(m_spannerMap); }
100101

101102
private:
@@ -119,8 +120,6 @@ class RenderMultiColumnFlow final : public RenderFragmentedFlow {
119120
void handleSpannerRemoval(RenderObject& spanner);
120121
RenderObject* processPossibleSpannerDescendant(RenderObject*& subtreeRoot, RenderObject& descendant);
121122

122-
SpannerMap& spannerMap() { return *m_spannerMap; }
123-
124123
private:
125124
std::unique_ptr<SpannerMap> m_spannerMap;
126125

‎Source/WebCore/style/RenderTreeUpdaterMultiColumn.cpp

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,36 @@ void RenderTreeUpdater::MultiColumn::update(RenderBlockFlow& flow)
4949

5050
void RenderTreeUpdater::MultiColumn::createFragmentedFlow(RenderBlockFlow& flow)
5151
{
52-
auto newFragmentedFlow = WebCore::createRenderer<RenderMultiColumnFlow>(flow.document(), RenderStyle::createAnonymousStyleWithDisplay(flow.style(), BLOCK));
53-
newFragmentedFlow->initializeStyle();
5452
flow.setChildrenInline(false); // Do this to avoid wrapping inline children that are just going to move into the flow thread.
5553
flow.deleteLines();
54+
// If this soon-to-be multicolumn flow is already part of a multicolumn context, we need to move back the descendant spanners
55+
// to their original position before moving subtrees around.
56+
auto* enclosingflow = flow.enclosingFragmentedFlow();
57+
if (is<RenderMultiColumnFlow>(enclosingflow)) {
58+
auto& spanners = downcast<RenderMultiColumnFlow>(enclosingflow)->spannerMap();
59+
Vector<RenderMultiColumnSpannerPlaceholder*> placeholdersToDelete;
60+
for (auto& spannerAndPlaceholder : spanners) {
61+
auto& placeholder = *spannerAndPlaceholder.value;
62+
if (!placeholder.isDescendantOf(&flow))
63+
continue;
64+
placeholdersToDelete.append(&placeholder);
65+
}
66+
for (auto* placeholder : placeholdersToDelete) {
67+
auto* spanner = placeholder->spanner();
68+
if (!spanner) {
69+
ASSERT_NOT_REACHED();
70+
continue;
71+
}
72+
// Move the spanner back to its original position.
73+
auto& spannerOriginalParent = *placeholder->parent();
74+
// Detaching the spanner takes care of removing the placeholder (and merges the RenderMultiColumnSets).
75+
auto spannerToReInsert = spanner->parent()->takeChild(*spanner);
76+
spannerOriginalParent.addChild(WTFMove(spannerToReInsert));
77+
}
78+
}
79+
80+
auto newFragmentedFlow = WebCore::createRenderer<RenderMultiColumnFlow>(flow.document(), RenderStyle::createAnonymousStyleWithDisplay(flow.style(), BLOCK));
81+
newFragmentedFlow->initializeStyle();
5682
auto& fragmentedFlow = *newFragmentedFlow;
5783
flow.RenderBlock::addChild(WTFMove(newFragmentedFlow));
5884

0 commit comments

Comments
 (0)