Crash under VisibleSelection::firstRange()
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Jun 2016 23:01:49 +0000 (23:01 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Jun 2016 23:01:49 +0000 (23:01 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/shadow-dom/selection-at-shadow-root-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp
Source/WebCore/dom/Position.cpp
Source/WebCore/dom/Position.h
Source/WebCore/editing/VisiblePosition.cpp
Source/WebCore/editing/VisibleSelection.cpp

index bf1e01e..2e60684 100644 (file)
@@ -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
new file mode 100644 (file)
index 0000000..d4d48b8
--- /dev/null
@@ -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
new file mode 100644 (file)
index 0000000..809870d
--- /dev/null
@@ -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>
index 321c9d6..8540ab6 100644 (file)
@@ -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
index d28c379..4f5bdf1 100644 (file)
@@ -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);
index b2cbd7f..b80a8e1 100644 (file)
@@ -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)
 {
index ab07051..b0e0ec9 100644 (file)
@@ -199,8 +199,7 @@ public:
     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)
index b45d5b8..d48568d 100644 (file)
@@ -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;
index 72b8fa9..174b2d5 100644 (file)
@@ -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);
 }