RenderMultiColumnFlow::fragmentedFlowDescendantInserted should not destroy incoming...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Dec 2017 00:51:01 +0000 (00:51 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Dec 2017 00:51:01 +0000 (00:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180181

Reviewed by Antti Koivisto.

This is in preparation for having all multicolumn related tree mutation in RenderTreeUpdaterMultiColumn.

Covered by fast/multicol/column-span-range-crash.html

* rendering/RenderMultiColumnFlow.cpp:
(WebCore::RenderMultiColumnFlow::fragmentedFlowDescendantInserted):
* rendering/RenderMultiColumnFlow.h:
* style/RenderTreeUpdaterMultiColumn.cpp:
(WebCore::RenderTreeUpdater::MultiColumn::destroyFragmentedFlow):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225506 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderMultiColumnFlow.cpp
Source/WebCore/rendering/RenderMultiColumnFlow.h
Source/WebCore/style/RenderTreeUpdaterMultiColumn.cpp

index f879432..24966b0 100644 (file)
@@ -1,3 +1,20 @@
+2017-12-04  Zalan Bujtas  <zalan@apple.com>
+
+        RenderMultiColumnFlow::fragmentedFlowDescendantInserted should not destroy incoming newDescendant
+        https://bugs.webkit.org/show_bug.cgi?id=180181
+
+        Reviewed by Antti Koivisto.
+
+        This is in preparation for having all multicolumn related tree mutation in RenderTreeUpdaterMultiColumn.
+
+        Covered by fast/multicol/column-span-range-crash.html
+
+        * rendering/RenderMultiColumnFlow.cpp:
+        (WebCore::RenderMultiColumnFlow::fragmentedFlowDescendantInserted):
+        * rendering/RenderMultiColumnFlow.h:
+        * style/RenderTreeUpdaterMultiColumn.cpp:
+        (WebCore::RenderTreeUpdater::MultiColumn::destroyFragmentedFlow):
+
 2017-12-04  JF Bastien  <jfbastien@apple.com>
 
         Update std::expected to match libc++ coding style
index cd6cd8b..a374224 100644 (file)
@@ -350,46 +350,26 @@ void RenderMultiColumnFlow::fragmentedFlowDescendantInserted(RenderObject& newDe
     if (gShiftingSpanner || newDescendant.isInFlowRenderFragmentedFlow())
         return;
 
-    Vector<RenderPtr<RenderObject>> spannersToDelete;
-
-    RenderObject* subtreeRoot = &newDescendant;
-    for (auto* descendant = &newDescendant; descendant; descendant = (descendant ? descendant->nextInPreOrder(subtreeRoot) : nullptr)) {
+    auto* subtreeRoot = &newDescendant;
+    auto* descendant = subtreeRoot;
+    while (descendant) {
+        // Skip nested multicolumn flows.
+        if (is<RenderMultiColumnFlow>(*descendant)) {
+            descendant = descendant->nextSibling();
+            continue;
+        }
         if (is<RenderMultiColumnSpannerPlaceholder>(*descendant)) {
             // A spanner's placeholder has been inserted. The actual spanner renderer is moved from
             // where it would otherwise occur (if it weren't a spanner) to becoming a sibling of the
             // column sets.
             RenderMultiColumnSpannerPlaceholder& placeholder = downcast<RenderMultiColumnSpannerPlaceholder>(*descendant);
-            if (placeholder.fragmentedFlow() != this) {
-                // This isn't our spanner! It shifted here from an ancestor multicolumn block. It's going to end up
-                // becoming our spanner instead, but for it to do that we first have to nuke the original spanner,
-                // and get the spanner content back into this flow thread.
-                RenderBox* spanner = placeholder.spanner();
-                
-                // Insert after the placeholder, but don't let a notification happen.
-                gShiftingSpanner = true;
-                RenderBlockFlow& ancestorBlock = downcast<RenderBlockFlow>(*spanner->parent());
-                ancestorBlock.moveChildTo(placeholder.parentBox(), spanner, placeholder.nextSibling(), RenderBoxModelObject::NormalizeAfterInsertion::Yes);
-                gShiftingSpanner = false;
-                
-                // We have to nuke the placeholder, since the ancestor already lost the mapping to it when
-                // we shifted the placeholder down into this flow thread.
-                placeholder.fragmentedFlow()->spannerMap().remove(spanner);
-
-                spannersToDelete.append(placeholder.parent()->takeChild(placeholder));
-
-                if (subtreeRoot == descendant)
-                    subtreeRoot = spanner;
-                // Now we process the spanner.
-                descendant = processPossibleSpannerDescendant(subtreeRoot, *spanner);
-                continue;
-            }
-            
             ASSERT(!spannerMap().get(placeholder.spanner()));
-            spannerMap().add(placeholder.spanner(), makeWeakPtr(placeholder));
+            spannerMap().add(placeholder.spanner(), makeWeakPtr(downcast<RenderMultiColumnSpannerPlaceholder>(descendant)));
             ASSERT(!placeholder.firstChild()); // There should be no children here, but if there are, we ought to skip them.
-            continue;
-        }
-        descendant = processPossibleSpannerDescendant(subtreeRoot, *descendant);
+        } else
+            descendant = processPossibleSpannerDescendant(subtreeRoot, *descendant);
+        if (descendant)
+            descendant = descendant->nextInPreOrder(subtreeRoot);
     }
 }
 
index 23c9609..9e72c02 100644 (file)
@@ -97,7 +97,6 @@ public:
 
     typedef HashMap<RenderBox*, WeakPtr<RenderMultiColumnSpannerPlaceholder>> SpannerMap;
     SpannerMap& spannerMap() { return *m_spannerMap; }
-    std::unique_ptr<SpannerMap> takeSpannerMap() { return WTFMove(m_spannerMap); }
 
 private:
     bool isRenderMultiColumnFlow() const override { return true; }
index 1c1feb4..2e1f712 100644 (file)
@@ -97,26 +97,31 @@ void RenderTreeUpdater::MultiColumn::createFragmentedFlow(RenderBlockFlow& flow)
 
 void RenderTreeUpdater::MultiColumn::destroyFragmentedFlow(RenderBlockFlow& flow)
 {
-    auto& fragmentedFlow = *flow.multiColumnFlow();
-    flow.clearMultiColumnFlow();
-
-    fragmentedFlow.deleteLines();
-    fragmentedFlow.moveAllChildrenTo(&flow, RenderBoxModelObject::NormalizeAfterInsertion::Yes);
+    auto& multiColumnFlow = *flow.multiColumnFlow();
+    multiColumnFlow.deleteLines();
 
     // Move spanners back to their original DOM position in the tree, and destroy the placeholders.
-    auto spannerMap = fragmentedFlow.takeSpannerMap();
-    for (auto& spannerAndPlaceholder : *spannerMap) {
-        RenderBox& spanner = *spannerAndPlaceholder.key;
-        auto& placeholder = *spannerAndPlaceholder.value;
-        auto takenSpanner = flow.takeChild(spanner);
-        placeholder.parent()->addChild(WTFMove(takenSpanner), &placeholder);
-        placeholder.removeFromParentAndDestroy();
+    auto& spanners = multiColumnFlow.spannerMap();
+    Vector<RenderMultiColumnSpannerPlaceholder*> placeholdersToDelete;
+    for (auto& spannerAndPlaceholder : spanners)
+        placeholdersToDelete.append(spannerAndPlaceholder.value.get());
+    Vector<std::pair<RenderElement*, RenderPtr<RenderObject>>> parentAndSpannerList;
+    for (auto* placeholder : placeholdersToDelete) {
+        auto* spannerOriginalParent = placeholder->parent();
+        if (spannerOriginalParent == &multiColumnFlow)
+            spannerOriginalParent = &flow;
+        // Detaching the spanner takes care of removing the placeholder (and merges the RenderMultiColumnSets).
+        auto* spanner = placeholder->spanner();
+        parentAndSpannerList.append(std::make_pair(spannerOriginalParent, spanner->parent()->takeChild(*spanner)));
     }
-
-    while (auto* columnSet = fragmentedFlow.firstMultiColumnSet())
+    while (auto* columnSet = multiColumnFlow.firstMultiColumnSet())
         columnSet->removeFromParentAndDestroy();
 
-    fragmentedFlow.removeFromParentAndDestroy();
+    flow.clearMultiColumnFlow();
+    multiColumnFlow.moveAllChildrenTo(&flow, RenderBoxModelObject::NormalizeAfterInsertion::Yes);
+    multiColumnFlow.removeFromParentAndDestroy();
+    for (auto& parentAndSpanner : parentAndSpannerList)
+        parentAndSpanner.first->addChild(WTFMove(parentAndSpanner.second));
 }
 
 }