Merge r186984 - REGRESSION (r169105): Do not assign a renderer to mul…

…tiple selection subtrees. https://bugs.webkit.org/show_bug.cgi?id=147038 rdar://problem/21819351 Reviewed by David Kilzer. A renderer should never be assigned to multiple selection subtrees. (Currently RenderObject maintains the last selection state.) RenderView::applySubtreeSelection() loops from the start to the end of the selection to find renderers that are inside the selection. However, in case of regions (when multiple selection roots are present) traversing the renderer tree by calling RenderObject::nextInPreOrder() could end up going across selection roots. This patch ensures that we assign renderers to a specific selection only when the current selection root and the renderer's selection root match. Source/WebCore: Test: fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html * rendering/RenderView.cpp: (WebCore::SelectionIterator::SelectionIterator): (WebCore::SelectionIterator::current): (WebCore::SelectionIterator::checkForSpanner): (WebCore::RenderView::applySubtreeSelection): LayoutTests: * fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2-expected.txt: Added. * fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html: Added.
WebKit · Aug 4, 2015 · 7bba30d · 7bba30d
1 parent 9b6dbb1
commit 7bba30d
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 37 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,20 @@
+2015-07-17  Zalan Bujtas  <zalan@apple.com>
+
+        REGRESSION (r169105): Do not assign a renderer to multiple selection subtrees.
+        https://bugs.webkit.org/show_bug.cgi?id=147038
+        rdar://problem/21819351
+
+        Reviewed by David Kilzer.
+
+        A renderer should never be assigned to multiple selection subtrees. (Currently RenderObject maintains the last selection state.)
+        RenderView::applySubtreeSelection() loops from the start to the end of the selection to find renderers that are inside the selection.
+        However, in case of regions (when multiple selection roots are present) traversing the renderer tree by calling RenderObject::nextInPreOrder() could
+        end up going across selection roots.
+        This patch ensures that we assign renderers to a specific selection only when the current selection root and the renderer's selection root match.
+
+        * fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2-expected.txt: Added.
+        * fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html: Added.
+
 2015-07-17  Zalan Bujtas  <zalan@apple.com>
 
         (display: block)input range's thumb disappears when moved.

diff --git a/LayoutTests/fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2-expected.txt b/LayoutTests/fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2-expected.txt
@@ -0,0 +1,3 @@
+PASS if no assert or crash.
+
+foobar
diff --git a/LayoutTests/fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html b/LayoutTests/fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html
@@ -0,0 +1,23 @@
+<script>
+  if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+  }
+</script>
+<style>
+  div { -webkit-flow-into: content-flow; }
+  * { -webkit-flow-into: thread; }
+</style>
+<body onload="shuffle()"><p>PASS if no assert or crash.</p><div>content</div>foobar</body>
+<script>
+  document.execCommand("SelectAll");
+</script>
+<script>
+function shuffle() {
+  var element = document.getElementsByTagName("div")[0];
+  element.parentNode.removeChild(element);
+  document.body.offsetTop;
+  if (window.testRunner)
+    testRunner.notifyDone();
+}
+</script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2015-07-17  Zalan Bujtas  <zalan@apple.com>
+
+        REGRESSION (r169105): Do not assign a renderer to multiple selection subtrees.
+        https://bugs.webkit.org/show_bug.cgi?id=147038
+        rdar://problem/21819351
+
+        Reviewed by David Kilzer.
+
+        A renderer should never be assigned to multiple selection subtrees. (Currently RenderObject maintains the last selection state.)
+        RenderView::applySubtreeSelection() loops from the start to the end of the selection to find renderers that are inside the selection.
+        However, in case of regions (when multiple selection roots are present) traversing the renderer tree by calling RenderObject::nextInPreOrder() could
+        end up going across selection roots.
+        This patch ensures that we assign renderers to a specific selection only when the current selection root and the renderer's selection root match.
+
+        Test: fast/regions/crash-when-renderer-is-in-multiple-selection-subtrees2.html
+
+        * rendering/RenderView.cpp:
+        (WebCore::SelectionIterator::SelectionIterator):
+        (WebCore::SelectionIterator::current):
+        (WebCore::SelectionIterator::checkForSpanner):
+        (WebCore::RenderView::applySubtreeSelection):
+
 2015-07-17  Zalan Bujtas  <zalan@apple.com>
 
         (display: block)input range's thumb disappears when moved.

diff --git a/Source/WebCore/rendering/RenderView.cpp b/Source/WebCore/rendering/RenderView.cpp
@@ -75,25 +75,13 @@ struct FrameFlatteningLayoutDisallower {
 };
 
 struct SelectionIterator {
-    RenderObject* m_current;
-    Vector<RenderMultiColumnSpannerPlaceholder*> m_spannerStack;
-
-    SelectionIterator(RenderObject* o)
+    SelectionIterator(RenderObject* start)
+        : m_current(start)
     {
-        m_current = o;
         checkForSpanner();
     }
 
-    void checkForSpanner()
-    {
-        if (!is<RenderMultiColumnSpannerPlaceholder>(m_current))
-            return;
-        auto& placeholder = downcast<RenderMultiColumnSpannerPlaceholder>(*m_current);
-        m_spannerStack.append(&placeholder);
-        m_current = placeholder.spanner();
-    }
-
-    RenderObject* current()
+    RenderObject* current() const
     {
         return m_current;
     }
@@ -111,6 +99,19 @@ struct SelectionIterator {
         }
         return m_current;
     }
+
+private:
+    void checkForSpanner()
+    {
+        if (!is<RenderMultiColumnSpannerPlaceholder>(m_current))
+            return;
+        auto& placeholder = downcast<RenderMultiColumnSpannerPlaceholder>(*m_current);
+        m_spannerStack.append(&placeholder);
+        m_current = placeholder.spanner();
+    }
+
+    RenderObject* m_current { nullptr };
+    Vector<RenderMultiColumnSpannerPlaceholder*> m_spannerStack;
 };
 
 RenderView::RenderView(Document& document, Ref<RenderStyle>&& style)
@@ -1019,14 +1020,18 @@ void RenderView::applySubtreeSelection(const SelectionSubtreeRoot& root, Selecti
             root.selectionData().selectionEnd()->setSelectionStateIfNeeded(SelectionEnd);
     }
 
-    RenderObject* o = root.selectionData().selectionStart();
-    RenderObject* stop = rendererAfterPosition(root.selectionData().selectionEnd(), root.selectionData().selectionEndPos());
-    SelectionIterator selectionIterator(o);
-
-    while (o && o != stop) {
-        if (o != root.selectionData().selectionStart() && o != root.selectionData().selectionEnd() && o->canBeSelectionLeaf())
-            o->setSelectionStateIfNeeded(SelectionInside);
-        o = selectionIterator.next();
+    RenderObject* selectionStart = root.selectionData().selectionStart();
+    RenderObject* selectionEnd = rendererAfterPosition(root.selectionData().selectionEnd(), root.selectionData().selectionEndPos());
+    SelectionIterator selectionIterator(selectionStart);
+    for (RenderObject* currentRenderer = selectionStart; currentRenderer && currentRenderer != selectionEnd; currentRenderer = selectionIterator.next()) {
+        if (currentRenderer == root.selectionData().selectionStart() || currentRenderer == root.selectionData().selectionEnd())
+            continue;
+        if (!currentRenderer->canBeSelectionLeaf())
+            continue;
+        // FIXME: Move this logic to SelectionIterator::next()
+        if (&currentRenderer->selectionRoot() != &root)
+            continue;
+        currentRenderer->setSelectionStateIfNeeded(SelectionInside);
     }
 
     if (blockRepaintMode != RepaintNothing)
@@ -1036,36 +1041,33 @@ void RenderView::applySubtreeSelection(const SelectionSubtreeRoot& root, Selecti
     // put them in the new objects list.
     SelectedObjectMap newSelectedObjects;
     SelectedBlockMap newSelectedBlocks;
-    o = root.selectionData().selectionStart();
-    selectionIterator = SelectionIterator(o);
-    while (o && o != stop) {
-        if (isValidObjectForNewSelection(root, *o)) {
-            std::unique_ptr<RenderSelectionInfo> selectionInfo = std::make_unique<RenderSelectionInfo>(*o, true);
+    selectionIterator = SelectionIterator(selectionStart);
+    for (RenderObject* currentRenderer = selectionStart; currentRenderer && currentRenderer != selectionEnd; currentRenderer = selectionIterator.next()) {
+        if (isValidObjectForNewSelection(root, *currentRenderer)) {
+            std::unique_ptr<RenderSelectionInfo> selectionInfo = std::make_unique<RenderSelectionInfo>(*currentRenderer, true);
 
 #if ENABLE(SERVICE_CONTROLS)
             for (auto& rect : selectionInfo->collectedSelectionRects())
                 m_selectionRectGatherer.addRect(selectionInfo->repaintContainer(), rect);
-            if (!o->isTextOrLineBreak())
+            if (!currentRenderer->isTextOrLineBreak())
                 m_selectionRectGatherer.setTextOnly(false);
 #endif
 
-            newSelectedObjects.set(o, WTF::move(selectionInfo));
+            newSelectedObjects.set(currentRenderer, WTF::move(selectionInfo));
 
-            RenderBlock* cb = o->containingBlock();
-            while (cb && !cb->isRenderView()) {
-                std::unique_ptr<RenderBlockSelectionInfo>& blockInfo = newSelectedBlocks.add(cb, nullptr).iterator->value;
+            RenderBlock* containingBlock = currentRenderer->containingBlock();
+            while (containingBlock && !containingBlock->isRenderView()) {
+                std::unique_ptr<RenderBlockSelectionInfo>& blockInfo = newSelectedBlocks.add(containingBlock, nullptr).iterator->value;
                 if (blockInfo)
                     break;
-                blockInfo = std::make_unique<RenderBlockSelectionInfo>(*cb);
-                cb = cb->containingBlock();
+                blockInfo = std::make_unique<RenderBlockSelectionInfo>(*containingBlock);
+                containingBlock = containingBlock->containingBlock();
 
 #if ENABLE(SERVICE_CONTROLS)
                 m_selectionRectGatherer.addGapRects(blockInfo->repaintContainer(), blockInfo->rects());
 #endif
             }
         }
-
-        o = selectionIterator.next();
     }
 
     if (blockRepaintMode == RepaintNothing)