EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2021 22:05:09 +0000 (22:05 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2021 22:05:09 +0000 (22:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=221942

Reviewed by Wenson Hsieh.

Source/WebCore:

In r272777, we re-introduced a nullptr check in DOMSelection::getRangeAt as we were getting crash reports
in this code but we weren't sure of the root cause. Since then we've identified that one of the root causes
is that EventHandler::updateSelectionForMouseDownDispatchingSelectStart doesn't check if VisibleSelection
is still in a good state after dispatching selectstart event. This results in FrameSelection's
setSelectionWithoutUpdatingAppearance getting called with a selection with end points already being orphaned.

This patch fixes this bug in setSelectionWithoutUpdatingAppearance and also adds an additional check in
FrameSelection::setSelectionWithoutUpdatingAppearance itself to avoid using an orphaned selection. It also
introduces a number of release and debug assertions in a number of places to help catch similar bugs.

Test: editing/selection/click-selection-with-selectstart-node-removal.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Added a debug-only assertion.
(WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
* dom/Document.cpp:
(WebCore::Document::removedLastRef): Disallow script execution in this code entirely. Also release assert
that the selection had already been cleared by this point.
(WebCore::Document::willBeRemovedFromFrame): Disallow script execution once we've unloaded subframes.
* editing/FrameSelection.cpp:
(WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance): Return early if the new selection's
end points had been orphaned, and disallow script execution between that and until we update the selection.
* editing/VisibleSelection.cpp:
(WebCore::VisibleSelection::isOrphan const): Added.
* editing/VisibleSelection.h:
* page/DOMSelection.cpp:
(WebCore::DOMSelection::getRangeAt): Removed nullptr check added in r272777.
* page/EventHandler.cpp:
(WebCore::EventHandler::updateSelectionForMouseDownDispatchingSelectStart): Fixed what is now believed to be
the root cause of the bug 221786.

LayoutTests:

Added a regression test for the bug 221786 / r272777.

Also updated a test imported from blink to expect rangeCount of 0 instead of 1
since we no longer update the selection when the target node has been removed
during selectstart.

* editing/selection/click-selection-with-selectstart-node-removal-expected.txt: Added.
* editing/selection/click-selection-with-selectstart-node-removal.html: Added.
* imported/blink/editing/selection/selectstart-event-crash.html:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/selection/click-selection-with-selectstart-node-removal-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/click-selection-with-selectstart-node-removal.html [new file with mode: 0644]
LayoutTests/imported/blink/editing/selection/selectstart-event-crash.html
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/Document.cpp
Source/WebCore/editing/FrameSelection.cpp
Source/WebCore/editing/VisibleSelection.cpp
Source/WebCore/editing/VisibleSelection.h
Source/WebCore/page/DOMSelection.cpp
Source/WebCore/page/EventHandler.cpp

index befdb6d424c1f0413cf7923390b4d3e28b004fa9..b620a96e6eb723cfc59469de3a927a33ddb9f87b 100644 (file)
@@ -1,3 +1,20 @@
+2021-02-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an orphaned selection
+        https://bugs.webkit.org/show_bug.cgi?id=221942
+
+        Reviewed by Wenson Hsieh.
+
+        Added a regression test for the bug 221786 / r272777.
+
+        Also updated a test imported from blink to expect rangeCount of 0 instead of 1
+        since we no longer update the selection when the target node has been removed
+        during selectstart.
+
+        * editing/selection/click-selection-with-selectstart-node-removal-expected.txt: Added.
+        * editing/selection/click-selection-with-selectstart-node-removal.html: Added.
+        * imported/blink/editing/selection/selectstart-event-crash.html:
+
 2021-02-16  Antoine Quint  <graouts@webkit.org>
 
         REGRESSION(r271515): ::marker fired at wrong time
diff --git a/LayoutTests/editing/selection/click-selection-with-selectstart-node-removal-expected.txt b/LayoutTests/editing/selection/click-selection-with-selectstart-node-removal-expected.txt
new file mode 100644 (file)
index 0000000..b3c8ee2
--- /dev/null
@@ -0,0 +1,10 @@
+This tests removing the target of mouse down during selectstart. WebKit should not set the selection's endpoints to newly disconnected nodes even in this case.'
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getSelection().rangeCount is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+Click the box below.
diff --git a/LayoutTests/editing/selection/click-selection-with-selectstart-node-removal.html b/LayoutTests/editing/selection/click-selection-with-selectstart-node-removal.html
new file mode 100644 (file)
index 0000000..bf98279
--- /dev/null
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p id="container">Click the box below.</p>
+<div id="editor" style="border: solid 2px red; padding: 2px; width: 100px; height: 100px" contenteditable>a</div>
+<script src="../../resources/ui-helper.js"></script>
+<script src="../../resources/js-test.js"></script>
+<script>
+
+jsTestIsAsync = true;
+
+description(`This tests removing the target of mouse down during selectstart.
+WebKit should not set the selection's endpoints to newly disconnected nodes even in this case.'`);
+
+editor.addEventListener('mousedown', () => {
+    getSelection().removeAllRanges();
+    editor.addEventListener('selectstart', () => {
+        editor.remove();
+    }, {once: true});
+    setTimeout(() => {
+        shouldBe('getSelection().rangeCount', '0');
+        if (getSelection().rangeCount)
+            console.log(getSelection().getRangeAt(0));
+        finishJSTest();
+    }, 5);
+});
+
+if (window.testRunner)
+    UIHelper.activateElement(editor);
+
+successfullyParsed = true;
+
+</script>
+</body>
+</html>
index 253f87f4e7fa3519beb257c4d63c861623511a75..c3d76de635e2ca3d9dd73e2e92da616cef88c2e9 100644 (file)
@@ -14,6 +14,6 @@ test(function() {
     eventSender.mouseMoveTo(one.offsetLeft, one.offsetTop);
     eventSender.mouseDown();
     var selection = getSelection();
-    assert_equals(selection.rangeCount, 1);
+    assert_equals(selection.rangeCount, 0);
 });
 </script>
index bc00ccba39c02858901ea82043dab635ef31bed4..e442f8bdad780cbafbcbfb192ca4ad3ac4435c81 100644 (file)
@@ -1,3 +1,41 @@
+2021-02-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an orphaned selection
+        https://bugs.webkit.org/show_bug.cgi?id=221942
+
+        Reviewed by Wenson Hsieh.
+
+        In r272777, we re-introduced a nullptr check in DOMSelection::getRangeAt as we were getting crash reports
+        in this code but we weren't sure of the root cause. Since then we've identified that one of the root causes
+        is that EventHandler::updateSelectionForMouseDownDispatchingSelectStart doesn't check if VisibleSelection
+        is still in a good state after dispatching selectstart event. This results in FrameSelection's
+        setSelectionWithoutUpdatingAppearance getting called with a selection with end points already being orphaned.
+
+        This patch fixes this bug in setSelectionWithoutUpdatingAppearance and also adds an additional check in
+        FrameSelection::setSelectionWithoutUpdatingAppearance itself to avoid using an orphaned selection. It also
+        introduces a number of release and debug assertions in a number of places to help catch similar bugs.
+
+        Test: editing/selection/click-selection-with-selectstart-node-removal.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Added a debug-only assertion.
+        (WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef): Disallow script execution in this code entirely. Also release assert
+        that the selection had already been cleared by this point.
+        (WebCore::Document::willBeRemovedFromFrame): Disallow script execution once we've unloaded subframes.
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance): Return early if the new selection's
+        end points had been orphaned, and disallow script execution between that and until we update the selection. 
+        * editing/VisibleSelection.cpp:
+        (WebCore::VisibleSelection::isOrphan const): Added.
+        * editing/VisibleSelection.h:
+        * page/DOMSelection.cpp:
+        (WebCore::DOMSelection::getRangeAt): Removed nullptr check added in r272777.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::updateSelectionForMouseDownDispatchingSelectStart): Fixed what is now believed to be
+        the root cause of the bug 221786.
+
 2021-02-16  Antoine Quint  <graouts@webkit.org>
 
         REGRESSION(r271515): ::marker fired at wrong time
index 0e9abc87b9d92d14348ba5a26f52c92003e7f222..f89bc6cadcc15a82174721758cc4d0d87d8dfce5 100644 (file)
@@ -126,6 +126,8 @@ ALWAYS_INLINE NodeVector ContainerNode::removeAllChildrenWithScriptAssertion(Chi
             willCreatePossiblyOrphanedTreeByRemoval(child.get());
     }
 
+    ASSERT_WITH_SECURITY_IMPLICATION(!document().selection().selection().isOrphan());
+
     if (deferChildrenChanged == DeferChildrenChanged::No)
         childrenChanged(ContainerNode::ChildChange { ChildChange::Type::AllChildrenRemoved, nullptr, nullptr, source });
 
@@ -192,6 +194,8 @@ ALWAYS_INLINE bool ContainerNode::removeNodeWithScriptAssertion(Node& childToRem
     if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
         willCreatePossiblyOrphanedTreeByRemoval(&childToRemove);
 
+    ASSERT_WITH_SECURITY_IMPLICATION(!document().selection().selection().isOrphan());
+
     // FIXME: Move childrenChanged into ScriptDisallowedScope block.
     childrenChanged(change);
 
index 38ec18b9f4b6b7229d4e2fedc4347bc68b6ca01d..1b9f72663921085c58d08b5b07996569fea662aa 100644 (file)
@@ -781,6 +781,7 @@ Document::~Document()
 
 void Document::removedLastRef()
 {
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
     ASSERT(!m_deletionHasBegun);
     if (m_referencingNodeCount) {
         // Node::removedLastRef doesn't set refCount() to zero because it's not observable.
@@ -811,6 +812,8 @@ void Document::removedLastRef()
 
         detachParser();
 
+        RELEASE_ASSERT(m_selection->isNone());
+
         // removeDetachedChildren() doesn't always unregister IDs,
         // so tear down scope information up front to avoid having
         // stale references in the map.
@@ -2593,6 +2596,8 @@ void Document::willBeRemovedFromFrame()
     }
     RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_frame || !m_frame->tree().childCount());
 
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
     if (m_domWindow && m_frame)
         m_domWindow->willDetachDocumentFromFrame();
 
index cafc144eac699da73dc98e2ff5ef5d62ae9152e8..d93b1164eccc8c4348294fe03b7bd241d38ea0a7 100644 (file)
@@ -65,6 +65,7 @@
 #include "RenderView.h"
 #include "RenderWidget.h"
 #include "RenderedPosition.h"
+#include "ScriptDisallowedScope.h"
 #include "Settings.h"
 #include "SimpleRange.h"
 #include "SpatialNavigation.h"
@@ -333,17 +334,11 @@ bool FrameSelection::setSelectionWithoutUpdatingAppearance(const VisibleSelectio
     if (shouldAlwaysUseDirectionalSelection(m_document.get()))
         newSelection.setIsDirectional(true);
 
-    if (!m_document || !m_document->frame()) {
-        m_selection = newSelection;
-        updateAssociatedLiveRange();
-        return false;
-    }
-
     // <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection
     // if document->frame() == m_document->frame() we can get into an infinite loop
     if (Document* newSelectionDocument = newSelection.base().document()) {
         if (RefPtr<Frame> newSelectionFrame = newSelectionDocument->frame()) {
-            if (newSelectionFrame != m_document->frame() && newSelectionDocument != m_document) {
+            if (m_document && newSelectionFrame != m_document->frame() && newSelectionDocument != m_document) {
                 newSelectionDocument->selection().setSelection(newSelection, options, AXTextStateChangeIntent(), align, granularity);
                 // It's possible that during the above set selection, this FrameSelection has been modified by
                 // selectFrameElementInParentIfFullySelected, but that the selection is no longer valid since
@@ -355,27 +350,41 @@ bool FrameSelection::setSelectionWithoutUpdatingAppearance(const VisibleSelectio
         }
     }
 
-    m_granularity = granularity;
+    VisibleSelection oldSelection = m_selection;
+    bool willMutateSelection = oldSelection != newSelection;
+    if (willMutateSelection && m_document)
+        m_document->editor().selectionWillChange();
 
-    if (closeTyping)
-        TypingCommand::closeTyping(*m_document);
+    {
+        ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+        if (newSelection.isOrphan()) {
+            ASSERT_NOT_REACHED();
+            clear();
+            return false;
+        }
 
-    if (shouldClearTypingStyle)
-        clearTypingStyle();
+        if (!m_document || !m_document->frame()) {
+            m_selection = newSelection;
+            updateAssociatedLiveRange();
+            return false;
+        }
 
-    VisibleSelection oldSelection = m_selection;
-    bool didMutateSelection = oldSelection != newSelection;
-    if (didMutateSelection)
-        m_document->editor().selectionWillChange();
+        if (closeTyping)
+            TypingCommand::closeTyping(*m_document);
 
-    m_selection = newSelection;
-    updateAssociatedLiveRange();
+        if (shouldClearTypingStyle)
+            clearTypingStyle();
+
+        m_granularity = granularity;
+        m_selection = newSelection;
+        updateAssociatedLiveRange();
+    }
 
     // Selection offsets should increase when LF is inserted before the caret in InsertLineBreakCommand. See <https://webkit.org/b/56061>.
     if (HTMLTextFormControlElement* textControl = enclosingTextFormControl(newSelection.start()))
         textControl->selectionChanged(options.contains(FireSelectEvent));
 
-    if (!didMutateSelection)
+    if (!willMutateSelection)
         return false;
 
     setCaretRectNeedsUpdate();
index bad53573ba9d79ded9d1b52f06b1eb83d5c08e89..00d5f87a45526bcaef8cbec99f3fd79c50a720a6 100644 (file)
@@ -133,6 +133,17 @@ void VisibleSelection::setExtent(const VisiblePosition& visiblePosition)
     setExtent(visiblePosition.deepEquivalent());
 }
 
+bool VisibleSelection::isOrphan() const
+{
+    if (m_base.isOrphan() || m_extent.isOrphan() || m_start.isOrphan() || m_end.isOrphan())
+        return true;
+    if (m_anchor.isOrphan() && m_anchor.document()->settings().liveRangeSelectionEnabled())
+        return true;
+    if (m_focus.isOrphan() && m_focus.document()->settings().liveRangeSelectionEnabled())
+        return true;
+    return false;
+}
+
 Optional<SimpleRange> VisibleSelection::firstRange() const
 {
     if (isNoneOrOrphaned())
index e6e8f514a6e6b37684729be4a26289b28c04fe1c..7abeafd575127a14bbd541c4813a585387260296 100644 (file)
@@ -86,6 +86,7 @@ public:
     bool isCaretOrRange() const { return !isNone(); }
     bool isNonOrphanedRange() const { return isRange() && !start().isOrphan() && !end().isOrphan(); }
     bool isNoneOrOrphaned() const { return isNone() || start().isOrphan() || end().isOrphan(); }
+    bool isOrphan() const;
 
     bool isBaseFirst() const { return m_anchorIsFirst; }
     bool isDirectional() const { return m_isDirectional; }
index 41161c9f2cc06472d647b32889098a79aefe1e45..0df7d9b610777c64dda75aa0262f7492ba00a921 100644 (file)
@@ -367,11 +367,7 @@ ExceptionOr<Ref<Range>> DOMSelection::getRangeAt(unsigned index)
         return frame->selection().associatedLiveRange().releaseNonNull();
     if (auto shadowAncestor = selectionShadowAncestor(frame))
         return createLiveRange(makeSimpleRange(*makeBoundaryPointBeforeNode(*shadowAncestor)));
-    auto simpleRange = frame->selection().selection().firstRange();
-    ASSERT(simpleRange); // selection is orphaned but this shouldn't happen.
-    if (!simpleRange)
-        return Exception { IndexSizeError };
-    return createLiveRange(*simpleRange);
+    return createLiveRange(*frame->selection().selection().firstRange());
 }
 
 void DOMSelection::removeAllRanges()
index ab2460848338fe5b8a60ef69574414f1f5f2c06e..cb6ae3ea69df0ffb02a22694561d612ec761bf5d 100644 (file)
@@ -466,6 +466,11 @@ bool EventHandler::updateSelectionForMouseDownDispatchingSelectStart(Node* targe
         return false;
     }
 
+    if (selection.isOrphan()) {
+        m_mouseDownMayStartSelect = false;
+        return false;
+    }
+
     if (selection.isRange())
         m_selectionInitiationState = ExtendedSelection;
     else {