Changeset 169105 in webkit

Timestamp:

May 20, 2014 3:57:58 AM (10 years ago)

Author:

mihnea@adobe.com

Message:

[CSS Regions] Crash while painting block selection gaps in regions
​https://bugs.webkit.org/show_bug.cgi?id=132720

Reviewed by David Hyatt.

Source/WebCore:
The fix for WebKit bug ​https://bugs.webkit.org/show_bug.cgi?id=131511
allowed selection highlight to match the DOM selection when the start
and end point of the selection were in different flow threads. In order
to enable that, the selection was performed separately on view and
render flow threads, considered selection subtrees.

However, the start and end points for each selection subtree were computed
by means of Range class but it is not always possible to construct a valid
Range from two pairs of RenderObjects and offsets.

This patch keeps the substrees approach but instead of storing the endpoints
for each subtree in a Range and continuously extending the Range, it stores them
using the already available SelectionSubtreeRoot class. After the end points are
computed for each subtree and before processing the subtree selection, the end points
are adjusted in a similar fashion as the one used in FrameSelection::updateAppearance(),
to make sure we are passing the same expected information to the method implementing
the visible selection processing.

Test: fast/regions/selection-gaps-paint-crash.html

• rendering/RenderView.cpp:

(WebCore::RenderView::splitSelectionBetweenSubtrees):

• rendering/SelectionSubtreeRoot.cpp:

(WebCore::SelectionSubtreeRoot::adjustForVisibleSelection):

• rendering/SelectionSubtreeRoot.h:

(WebCore::SelectionSubtreeRoot::selectionClear):

LayoutTests:

• TestExpectations: Unskip a test that was crashing
• fast/regions/selection-gaps-paint-crash-expected.txt: Added.
• fast/regions/selection-gaps-paint-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/fast/regions/selection-gaps-paint-crash-expected.txt (added)
• LayoutTests/fast/regions/selection-gaps-paint-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderView.cpp (modified) (2 diffs)
• Source/WebCore/rendering/SelectionSubtreeRoot.cpp (modified) (2 diffs)
• Source/WebCore/rendering/SelectionSubtreeRoot.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-05-20  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        [CSS Regions] Crash while painting block selection gaps in regions
+        https://bugs.webkit.org/show_bug.cgi?id=132720
+
+        Reviewed by David Hyatt.
+
+        * TestExpectations: Unskip a test that was crashing
+        * fast/regions/selection-gaps-paint-crash-expected.txt: Added.
+        * fast/regions/selection-gaps-paint-crash.html: Added.
+
 2014-05-20  Lorenzo Tilve  <ltilve@igalia.com>
 

trunk/LayoutTests/TestExpectations

@@ -116 +116 @@
 css2.1/20110323/replaced-intrinsic-002.htm [ Failure ]
 
-# Regression from r167652:
-webkit.org/b/131982 [ Debug ] fast/regions/cssom/region-range-for-box-crash.html [ Crash ]
 
 webkit.org/b/132791 svg/as-object/sizing/svg-in-object-placeholder-height-fixed.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-05-20  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        [CSS Regions] Crash while painting block selection gaps in regions
+        https://bugs.webkit.org/show_bug.cgi?id=132720
+
+        Reviewed by David Hyatt.
+
+        The fix for WebKit bug https://bugs.webkit.org/show_bug.cgi?id=131511
+        allowed selection highlight to match the DOM selection when the start
+        and end point of the selection were in different flow threads. In order
+        to enable that, the selection was performed separately on view and
+        render flow threads, considered selection subtrees.
+
+        However, the start and end points for each selection subtree were computed
+        by means of Range class but it is not always possible to construct a valid
+        Range from two pairs of RenderObjects and offsets.
+
+        This patch keeps the substrees approach but instead of storing the endpoints
+        for each subtree in a Range and continuously extending the Range, it stores them
+        using the already available SelectionSubtreeRoot class. After the end points are
+        computed for each subtree and before processing the subtree selection, the end points
+        are adjusted in a similar fashion as the one used in FrameSelection::updateAppearance(),
+        to make sure we are passing the same expected information to the method implementing
+        the visible selection processing.
+
+        Test: fast/regions/selection-gaps-paint-crash.html
+
+        * rendering/RenderView.cpp:
+        (WebCore::RenderView::splitSelectionBetweenSubtrees):
+        * rendering/SelectionSubtreeRoot.cpp:
+        (WebCore::SelectionSubtreeRoot::adjustForVisibleSelection):
+        * rendering/SelectionSubtreeRoot.h:
+        (WebCore::SelectionSubtreeRoot::selectionClear):
+
 2014-05-19  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/rendering/RenderView.cpp

@@ -868 +868 @@
 void RenderView::splitSelectionBetweenSubtrees(RenderObject* start, int startPos, RenderObject* end, int endPos, SelectionRepaintMode blockRepaintMode)
 {
-    // Get ranges.
-    typedef HashMap<SelectionSubtreeRoot*, RefPtr<Range> > RenderSubtreesMap;
+    // Compute the visible selection end points for each of the subtrees.
+    typedef HashMap<SelectionSubtreeRoot*, SelectionSubtreeRoot> RenderSubtreesMap;
     RenderSubtreesMap renderSubtreesMap;
 
-    // Initialize map for RenderView and all RenderNamedFlowThreads.
-    renderSubtreesMap.set(this, nullptr);
+    SelectionSubtreeRoot initialSelection;
+    renderSubtreesMap.set(this, initialSelection);
     for (auto* namedFlowThread : *flowThreadController().renderNamedFlowThreadList())
-        renderSubtreesMap.set(namedFlowThread, nullptr);
+        renderSubtreesMap.set(namedFlowThread, initialSelection);
 
     if (start && end) {
-        RefPtr<Range> initialRange = Range::create(document(), start->node(), startPos, end->node(), endPos);
-
-        Node* startNode = initialRange->startContainer();
-        Node* endNode = initialRange->endContainer();
-        Node* stopNode = initialRange->pastLastNode();
+        Node* startNode = start->node();
+        Node* endNode = end->node();
+        Node* stopNode = NodeTraversal::nextSkippingChildren(endNode);
 
         for (Node* node = startNode; node != stopNode; node = NodeTraversal::next(node)) {
@@ -890 +888 @@
 
             SelectionSubtreeRoot& root = renderer->selectionRoot();
-            RefPtr<Range> range = renderSubtreesMap.get(&root);
-            if (!range) {
-                range = Range::create(document());
-
-                if (node == startNode)
-                    range->setStart(node, startPos);
-                else
-                    range->setStart(node, 0);
-
-                renderSubtreesMap.set(&root, range);
+            SelectionSubtreeRoot selectionData = renderSubtreesMap.get(&root);
+            if (selectionData.selectionClear()) {
+                selectionData.setSelectionStart(node->renderer());
+                selectionData.setSelectionStartPos(node == startNode ? startPos : 0);
             }
 
+            selectionData.setSelectionEnd(node->renderer());
             if (node == endNode)
-                range->setEnd(node, endPos);
+                selectionData.setSelectionEndPos(endPos);
             else
-                range->setEnd(node, node->offsetInCharacters() ? node->maxCharacterOffset() : node->childNodeCount());
+                selectionData.setSelectionEndPos(node->offsetInCharacters() ? node->maxCharacterOffset() : node->childNodeCount());
+
+            renderSubtreesMap.set(&root, selectionData);
         }
     }
 
     for (RenderSubtreesMap::iterator i = renderSubtreesMap.begin(); i != renderSubtreesMap.end(); ++i) {
-        RefPtr<Range> range = i->value;
-
-        RenderObject* newStart;
-        int newStartPos;
-        RenderObject* newEnd;
-        int newEndPos;
-
-        if (range) {
-            newStart = range->startContainer()->renderer();
-            newStartPos = range->startOffset();
-            newEnd = range->endContainer()->renderer();
-            newEndPos = range->endOffset();
-        } else {
-            newStart = 0;
-            newStartPos = -1;
-            newEnd = 0;
-            newEndPos = -1;
-        }
-
-        setSubtreeSelection(*i->key, newStart, newStartPos, newEnd, newEndPos, blockRepaintMode);
+        SelectionSubtreeRoot subtreeSelectionData = i->value;
+        subtreeSelectionData.adjustForVisibleSelection(document());
+        setSubtreeSelection(*i->key, subtreeSelectionData.selectionStart(), subtreeSelectionData.selectionStartPos(),
+            subtreeSelectionData.selectionEnd(), subtreeSelectionData.selectionEndPos(), blockRepaintMode);
     }
 }

trunk/Source/WebCore/rendering/SelectionSubtreeRoot.cpp

@@ -31 +31 @@
 #include "SelectionSubtreeRoot.h"
 
+#include "Document.h"
+#include "Position.h"
+#include "Range.h"
+#include "VisibleSelection.h"
+
 namespace WebCore {
 
@@ -47 +52 @@
 }
 
+void SelectionSubtreeRoot::adjustForVisibleSelection(Document& document)
+{
+    if (selectionClear())
+        return;
+
+    // Create a range based on the cached end points
+    Position startPosition = createLegacyEditingPosition(m_selectionStart->node(), m_selectionStartPos);
+    Position endPosition = createLegacyEditingPosition(m_selectionEnd->node(), m_selectionEndPos);
+
+    RefPtr<Range> range = Range::create(document, startPosition.parentAnchoredEquivalent(), endPosition.parentAnchoredEquivalent());
+    VisibleSelection selection(range.get());
+    Position startPos = selection.start();
+    Position candidate = startPos.downstream();
+    if (candidate.isCandidate())
+        startPos = candidate;
+
+    Position endPos = selection.end();
+    candidate = endPos.upstream();
+    if (candidate.isCandidate())
+        endPos = candidate;
+
+    m_selectionStart = nullptr;
+    m_selectionStartPos = -1;
+    m_selectionEnd = nullptr;
+    m_selectionEndPos = -1;
+
+    if (startPos.isNotNull() && endPos.isNotNull() && selection.visibleStart() != selection.visibleEnd()) {
+        m_selectionStart = startPos.deprecatedNode()->renderer();
+        m_selectionStartPos = startPos.deprecatedEditingOffset();
+        m_selectionEnd = endPos.deprecatedNode()->renderer();
+        m_selectionEndPos = endPos.deprecatedEditingOffset();
+    }
+}
+
 } // namespace WebCore

trunk/Source/WebCore/rendering/SelectionSubtreeRoot.h

@@ -35 +35 @@
 namespace WebCore {
 
+class Document;
+
 class SelectionSubtreeRoot {
 public:
@@ -44 +46 @@
     RenderObject* selectionEnd() const { return m_selectionEnd; }
     int selectionEndPos() const { return m_selectionEndPos; }
+    void selectionStartEndPositions(int& startPos, int& endPos) const;
+    bool selectionClear() const
+    {
+        return !m_selectionStart
+        && (m_selectionStartPos == -1)
+        && !m_selectionEnd
+        && (m_selectionEndPos == -1);
+    }
 
     void setSelectionStart(RenderObject* selectionStart) { m_selectionStart = selectionStart; }
@@ -50 +60 @@
     void setSelectionEndPos(int selectionEndPos) { m_selectionEndPos = selectionEndPos; }
 
-    void selectionStartEndPositions(int& startPos, int& endPos) const;
+    void adjustForVisibleSelection(Document&);
 
 private: