Crash under VisibleSelection::firstRange()

https://bugs.webkit.org/show_bug.cgi?id=158241 Reviewed by Enrica Casucci. Source/WebCore: The crash was commonly caused by parentAnchoredEquivalent returning null when the anchored node was a shadow root. Fixed it by returning a shadow root in parentAnchoredEquivalent. Also guard against other kinds of crashes by adding a null check in VisibleSelection::firstRange() since we've seen a crash in the same code path outside of a shadow tree. This patch also fixes other Position methods to stop using nonShadowBoundaryParentNode in place of parentNode as that would cause a similar crash and/or a bug elsewhere. Test: fast/shadow-dom/selection-at-shadow-root-crash.html * accessibility/AXObjectCache.cpp: (AXObjectCache::startCharacterOffsetOfParagraph): Fixed a bug uncovered by the assertion fix in Position::Position. This code was sometimes creating a position inside a BR, which is wrong. (AXObjectCache::endCharacterOffsetOfParagraph): Ditto. * dom/Position.cpp: (WebCore::Position::Position): Fixed an assertion which was checking that this constructor wasn't being called with m_anchorNode set to an element editing ignores content of. ||ing it with isShadowRoot() made this assertion useless because it's true whenever m_anchorNode is not a shadow root. (WebCore::Position::containerNode): Use parentNode() instead of findParent() which calls nonShadowBoundaryParentNode since Position should (WebCore::Position::parentAnchoredEquivalent): Fixed the bug by letting this function return a shadow root. (WebCore::Position::previous): Use parentNode() instead of findParent(). (WebCore::Position::next): Ditto. (WebCore::Position::atStartOfTree): Ditto. (WebCore::Position::atEndOfTree): Ditto. (WebCore::Position::findParent): Deleted. * dom/Position.h: * editing/VisibleSelection.cpp: (VisibleSelection::firstRange): Added a null check. LayoutTests: Added a regression test. * fast/shadow-dom/selection-at-shadow-root-crash-expected.txt: Added. * fast/shadow-dom/selection-at-shadow-root-crash.html: Added. Canonical link: https://commits.webkit.org/176453@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@201667 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 3, 2016 · d3f9bce5b2011bd674b503d5e319667eff3191ee · d3f9bce
1 parent 2b63786
commit d3f9bce5b2011bd674b503d5e319667eff3191ee
Showing 9 changed files with 97 additions and 19 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2016-06-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash under VisibleSelection::firstRange()
+        https://bugs.webkit.org/show_bug.cgi?id=158241
+
+        Reviewed by Enrica Casucci.
+
+        Added a regression test.
+
+        * fast/shadow-dom/selection-at-shadow-root-crash-expected.txt: Added.
+        * fast/shadow-dom/selection-at-shadow-root-crash.html: Added.
+
 2016-06-03  Zalan Bujtas  <zalan@apple.com>
 
         Incorrect rendering on boostmobile FAQ page

diff --git a/LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash-expected.txt b/LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash-expected.txt
@@ -0,0 +1,4 @@
+This tests copying an image which is a direct child of a shadow root. To manually test, copy the image by pressing command / control + c. WebKit should not crash or hit an assertion.
+
+PASS - WebKit did not crash
+
diff --git a/LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash.html b/LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash.html
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<body>
+<p>This tests copying an image which is a direct child of a shadow root.
+To manually test, copy the image by pressing command / control + c. WebKit should not crash or hit an assertion.
+</p>
+<pre id="result"></pre>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+var host = document.createElement('div');
+var root = host.attachShadow({mode: 'closed'});
+root.innerHTML = '<img src="../../editing/resources/abe.png" onload="runTest()">';
+
+document.body.appendChild(host);
+
+function runTest() {
+    window.getSelection().selectAllChildren(root);
+    document.execCommand('copy', null, false);
+    document.getElementById('result').textContent = 'PASS - WebKit did not crash';
+
+    if (testRunner)
+        testRunner.notifyDone();
+}
+
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,41 @@
+2016-06-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash under VisibleSelection::firstRange()
+        https://bugs.webkit.org/show_bug.cgi?id=158241
+
+        Reviewed by Enrica Casucci.
+
+        The crash was commonly caused by parentAnchoredEquivalent returning null when the anchored node was a shadow root.
+        Fixed it by returning a shadow root in parentAnchoredEquivalent.
+
+        Also guard against other kinds of crashes by adding a null check in VisibleSelection::firstRange() since we've seen
+        a crash in the same code path outside of a shadow tree.
+
+        This patch also fixes other Position methods to stop using nonShadowBoundaryParentNode in place of parentNode as
+        that would cause a similar crash and/or a bug elsewhere.
+
+        Test: fast/shadow-dom/selection-at-shadow-root-crash.html
+
+        * accessibility/AXObjectCache.cpp:
+        (AXObjectCache::startCharacterOffsetOfParagraph): Fixed a bug uncovered by the assertion fix in Position::Position.
+        This code was sometimes creating a position inside a BR, which is wrong.
+        (AXObjectCache::endCharacterOffsetOfParagraph): Ditto.
+        * dom/Position.cpp:
+        (WebCore::Position::Position): Fixed an assertion which was checking that this constructor wasn't being called
+        with m_anchorNode set to an element editing ignores content of. ||ing it with isShadowRoot() made this assertion
+        useless because it's true whenever m_anchorNode is not a shadow root.
+        (WebCore::Position::containerNode): Use parentNode() instead of findParent() which calls nonShadowBoundaryParentNode
+        since Position should
+        (WebCore::Position::parentAnchoredEquivalent): Fixed the bug by letting this function return a shadow root.
+        (WebCore::Position::previous): Use parentNode() instead of findParent().
+        (WebCore::Position::next): Ditto.
+        (WebCore::Position::atStartOfTree): Ditto.
+        (WebCore::Position::atEndOfTree): Ditto.
+        (WebCore::Position::findParent): Deleted.
+        * dom/Position.h:
+        * editing/VisibleSelection.cpp:
+        (VisibleSelection::firstRange): Added a null check. 
+
 2016-06-03  Zalan Bujtas  <zalan@apple.com>
 
         Incorrect rendering on boostmobile FAQ page

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -2329,8 +2329,7 @@ CharacterOffset AXObjectCache::startCharacterOffsetOfParagraph(const CharacterOf
 
     auto* startBlock = enclosingBlock(startNode);
     int offset = characterOffset.startIndex + characterOffset.offset;
-    Position p(startNode, offset, Position::PositionIsOffsetInAnchor);
-    auto* highestRoot = highestEditableRoot(p);
+    auto* highestRoot = highestEditableRoot(firstPositionInOrBeforeNode(startNode));
     Position::AnchorType type = Position::PositionIsOffsetInAnchor;
 
     auto* node = findStartOfParagraph(startNode, highestRoot, startBlock, offset, type, boundaryCrossingRule);
@@ -2352,8 +2351,7 @@ CharacterOffset AXObjectCache::endCharacterOffsetOfParagraph(const CharacterOffs
 
     Node* stayInsideBlock = enclosingBlock(startNode);
     int offset = characterOffset.startIndex + characterOffset.offset;
-    Position p(startNode, offset, Position::PositionIsOffsetInAnchor);
-    Node* highestRoot = highestEditableRoot(p);
+    Node* highestRoot = highestEditableRoot(firstPositionInOrBeforeNode(startNode));
     Position::AnchorType type = Position::PositionIsOffsetInAnchor;
 
     Node* node = findEndOfParagraph(startNode, highestRoot, stayInsideBlock, offset, type, boundaryCrossingRule);

diff --git a/Source/WebCore/dom/Position.cpp b/Source/WebCore/dom/Position.cpp
@@ -127,7 +127,7 @@ Position::Position(PassRefPtr<Node> anchorNode, int offset, AnchorType anchorTyp
     , m_anchorType(anchorType)
     , m_isLegacyEditingPosition(false)
 {
-    ASSERT(!m_anchorNode || !editingIgnoresContent(*m_anchorNode) || !m_anchorNode->isShadowRoot());
+    ASSERT(!m_anchorNode || !editingIgnoresContent(*m_anchorNode));
     ASSERT(!m_anchorNode || !m_anchorNode->isPseudoElement());
     ASSERT(anchorType == PositionIsOffsetInAnchor);
 }
@@ -170,7 +170,7 @@ Node* Position::containerNode() const
         return m_anchorNode.get();
     case PositionIsBeforeAnchor:
     case PositionIsAfterAnchor:
-        return findParent(*m_anchorNode);
+        return m_anchorNode->parentNode();
     }
     ASSERT_NOT_REACHED();
     return nullptr;
@@ -231,7 +231,7 @@ Position Position::parentAnchoredEquivalent() const
 
     // FIXME: This should only be necessary for legacy positions, but is also needed for positions before and after Tables
     if (m_offset <= 0 && (m_anchorType != PositionIsAfterAnchor && m_anchorType != PositionIsAfterChildren)) {
-        if (findParent(*m_anchorNode) && (editingIgnoresContent(*m_anchorNode) || isRenderedTable(m_anchorNode.get())))
+        if (m_anchorNode->parentNode() && (editingIgnoresContent(*m_anchorNode) || isRenderedTable(m_anchorNode.get())))
             return positionInParentBeforeNode(m_anchorNode.get());
         return Position(m_anchorNode.get(), 0, PositionIsOffsetInAnchor);
     }
@@ -344,7 +344,7 @@ Position Position::previous(PositionMoveType moveType) const
         }
     }
 
-    ContainerNode* parent = findParent(*node);
+    ContainerNode* parent = node->parentNode();
     if (!parent)
         return *this;
 
@@ -391,7 +391,7 @@ Position Position::next(PositionMoveType moveType) const
         return createLegacyEditingPosition(node, (moveType == Character) ? uncheckedNextOffset(node, offset) : offset + 1);
     }
 
-    ContainerNode* parent = findParent(*node);
+    ContainerNode* parent = node->parentNode();
     if (!parent)
         return *this;
 
@@ -493,7 +493,7 @@ bool Position::atStartOfTree() const
         return true;
 
     Node* container = containerNode();
-    if (container && findParent(*container))
+    if (container && container->parentNode())
         return false;
 
     switch (m_anchorType) {
@@ -518,7 +518,7 @@ bool Position::atEndOfTree() const
         return true;
 
     Node* container = containerNode();
-    if (container && findParent(*container))
+    if (container && container->parentNode())
         return false;
 
     switch (m_anchorType) {
@@ -953,11 +953,6 @@ bool Position::nodeIsUserSelectNone(Node* node)
     return node && node->renderer() && node->renderer()->style().userSelect() == SELECT_NONE;
 }
 
-ContainerNode* Position::findParent(const Node& node)
-{
-    return node.nonShadowBoundaryParentNode();
-}
-
 #if ENABLE(USERSELECT_ALL)
 bool Position::nodeIsUserSelectAll(const Node* node)
 {

diff --git a/Source/WebCore/dom/Position.h b/Source/WebCore/dom/Position.h
@@ -199,8 +199,7 @@ class Position {
     static bool nodeIsUserSelectAll(const Node*) { return false; }
     static Node* rootUserSelectAllForNode(Node*) { return 0; }
 #endif
-    static ContainerNode* findParent(const Node&);
-
+
     void debugPosition(const char* msg = "") const;
 
 #if ENABLE(TREE_DEBUGGING)

diff --git a/Source/WebCore/editing/VisiblePosition.cpp b/Source/WebCore/editing/VisiblePosition.cpp
@@ -587,7 +587,7 @@ Position VisiblePosition::canonicalPosition(const Position& passedPosition)
 
     // If the html element is editable, descending into its body will look like a descent 
     // from non-editable to editable content since rootEditableElement() always stops at the body.
-    if ((editingRoot && editingRoot->hasTagName(htmlTag)) || position.deprecatedNode()->isDocumentNode())
+    if ((editingRoot && editingRoot->hasTagName(htmlTag)) || (node && (node->isDocumentNode() || node->isShadowRoot())))
         return next.isNotNull() ? next : prev;
 
     bool prevIsInSameEditableElement = prevNode && editableRootForPosition(prev) == editingRoot;

diff --git a/Source/WebCore/editing/VisibleSelection.cpp b/Source/WebCore/editing/VisibleSelection.cpp
@@ -129,6 +129,8 @@ RefPtr<Range> VisibleSelection::firstRange() const
         return nullptr;
     Position start = m_start.parentAnchoredEquivalent();
     Position end = m_end.parentAnchoredEquivalent();
+    if (start.isNull() || end.isNull())
+        return nullptr;
     return Range::create(start.anchorNode()->document(), start, end);
 }