Changeset 272928 in webkit


Ignore:
Timestamp:
Feb 16, 2021 2:05:09 PM (3 years ago)
Author:
rniwa@webkit.org
Message:

EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an orphaned selection
​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:
Location:
trunk
Files:
2 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r272927 r272928  
     12021-02-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an orphaned selection
     4        https://bugs.webkit.org/show_bug.cgi?id=221942
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        Added a regression test for the bug 221786 / r272777.
     9
     10        Also updated a test imported from blink to expect rangeCount of 0 instead of 1
     11        since we no longer update the selection when the target node has been removed
     12        during selectstart.
     13
     14        * editing/selection/click-selection-with-selectstart-node-removal-expected.txt: Added.
     15        * editing/selection/click-selection-with-selectstart-node-removal.html: Added.
     16        * imported/blink/editing/selection/selectstart-event-crash.html:
     17
    1182021-02-16  Antoine Quint  <graouts@webkit.org>
    219

  • trunk/LayoutTests/imported/blink/editing/selection/selectstart-event-crash.html

    r194917 r272928  
    1515    eventSender.mouseDown();
    1616    var selection = getSelection();
    17     assert_equals(selection.rangeCount, 1);
     17    assert_equals(selection.rangeCount, 0);
    1818});
    1919</script>

  • trunk/Source/WebCore/ChangeLog

    r272927 r272928  
     12021-02-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        EventHandler::updateSelectionForMouseDownDispatchingSelectStart should not use an orphaned selection
     4        https://bugs.webkit.org/show_bug.cgi?id=221942
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        In r272777, we re-introduced a nullptr check in DOMSelection::getRangeAt as we were getting crash reports
     9        in this code but we weren't sure of the root cause. Since then we've identified that one of the root causes
     10        is that EventHandler::updateSelectionForMouseDownDispatchingSelectStart doesn't check if VisibleSelection
     11        is still in a good state after dispatching selectstart event. This results in FrameSelection's
     12        setSelectionWithoutUpdatingAppearance getting called with a selection with end points already being orphaned.
     13
     14        This patch fixes this bug in setSelectionWithoutUpdatingAppearance and also adds an additional check in
     15        FrameSelection::setSelectionWithoutUpdatingAppearance itself to avoid using an orphaned selection. It also
     16        introduces a number of release and debug assertions in a number of places to help catch similar bugs.
     17
     18        Test: editing/selection/click-selection-with-selectstart-node-removal.html
     19
     20        * dom/ContainerNode.cpp:
     21        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Added a debug-only assertion.
     22        (WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
     23        * dom/Document.cpp:
     24        (WebCore::Document::removedLastRef): Disallow script execution in this code entirely. Also release assert
     25        that the selection had already been cleared by this point.
     26        (WebCore::Document::willBeRemovedFromFrame): Disallow script execution once we've unloaded subframes.
     27        * editing/FrameSelection.cpp:
     28        (WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance): Return early if the new selection's
     29        end points had been orphaned, and disallow script execution between that and until we update the selection.
     30        * editing/VisibleSelection.cpp:
     31        (WebCore::VisibleSelection::isOrphan const): Added.
     32        * editing/VisibleSelection.h:
     33        * page/DOMSelection.cpp:
     34        (WebCore::DOMSelection::getRangeAt): Removed nullptr check added in r272777.
     35        * page/EventHandler.cpp:
     36        (WebCore::EventHandler::updateSelectionForMouseDownDispatchingSelectStart): Fixed what is now believed to be
     37        the root cause of the bug 221786.
     38
    1392021-02-16  Antoine Quint  <graouts@webkit.org>
    240

  • trunk/Source/WebCore/dom/ContainerNode.cpp

    r272703 r272928  
    127127    }
    128128
     129    ASSERT_WITH_SECURITY_IMPLICATION(!document().selection().selection().isOrphan());
     130
    129131    if (deferChildrenChanged == DeferChildrenChanged::No)
    130132        childrenChanged(ContainerNode::ChildChange { ChildChange::Type::AllChildrenRemoved, nullptr, nullptr, source });
    … …  
    192194    if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
    193195        willCreatePossiblyOrphanedTreeByRemoval(&childToRemove);
     196
     197    ASSERT_WITH_SECURITY_IMPLICATION(!document().selection().selection().isOrphan());
    194198
    195199    // FIXME: Move childrenChanged into ScriptDisallowedScope block.

  • trunk/Source/WebCore/dom/Document.cpp

    r272805 r272928  
    782782void Document::removedLastRef()
    783783{
     784    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
    784785    ASSERT(!m_deletionHasBegun);
    785786    if (m_referencingNodeCount) {
    … …  
    811812
    812813        detachParser();
     814
     815        RELEASE_ASSERT(m_selection->isNone());
    813816
    814817        // removeDetachedChildren() doesn't always unregister IDs,
    … …  
    25942597    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_frame || !m_frame->tree().childCount());
    25952598
     2599    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
     2600
    25962601    if (m_domWindow && m_frame)
    25972602        m_domWindow->willDetachDocumentFromFrame();

  • trunk/Source/WebCore/editing/FrameSelection.cpp

    r271814 r272928  
    6666#include "RenderWidget.h"
    6767#include "RenderedPosition.h"
     68#include "ScriptDisallowedScope.h"
    6869#include "Settings.h"
    6970#include "SimpleRange.h"
    … …  
    334335        newSelection.setIsDirectional(true);
    335336
    336     if (!m_document || !m_document->frame()) {
    337         m_selection = newSelection;
    338         updateAssociatedLiveRange();
    339         return false;
    340     }
    341 
    342337    // <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection
    343338    // if document->frame() == m_document->frame() we can get into an infinite loop
    344339    if (Document* newSelectionDocument = newSelection.base().document()) {
    345340        if (RefPtr<Frame> newSelectionFrame = newSelectionDocument->frame()) {
    346             if (newSelectionFrame != m_document->frame() && newSelectionDocument != m_document) {
     341            if (m_document && newSelectionFrame != m_document->frame() && newSelectionDocument != m_document) {
    347342                newSelectionDocument->selection().setSelection(newSelection, options, AXTextStateChangeIntent(), align, granularity);
    348343                // It's possible that during the above set selection, this FrameSelection has been modified by
    … …  
    356351    }
    357352
    358     m_granularity = granularity;
    359 
    360     if (closeTyping)
    361         TypingCommand::closeTyping(*m_document);
    362 
    363     if (shouldClearTypingStyle)
    364         clearTypingStyle();
    365 
    366353    VisibleSelection oldSelection = m_selection;
    367     bool didMutateSelection = oldSelection != newSelection;
    368     if (didMutateSelection)
     354    bool willMutateSelection = oldSelection != newSelection;
     355    if (willMutateSelection && m_document)
    369356        m_document->editor().selectionWillChange();
    370357
    371     m_selection = newSelection;
    372     updateAssociatedLiveRange();
     358    {
     359        ScriptDisallowedScope::InMainThread scriptDisallowedScope;
     360        if (newSelection.isOrphan()) {
     361            ASSERT_NOT_REACHED();
     362            clear();
     363            return false;
     364        }
     365
     366        if (!m_document || !m_document->frame()) {
     367            m_selection = newSelection;
     368            updateAssociatedLiveRange();
     369            return false;
     370        }
     371
     372        if (closeTyping)
     373            TypingCommand::closeTyping(*m_document);
     374
     375        if (shouldClearTypingStyle)
     376            clearTypingStyle();
     377
     378        m_granularity = granularity;
     379        m_selection = newSelection;
     380        updateAssociatedLiveRange();
     381    }
    373382
    374383    // Selection offsets should increase when LF is inserted before the caret in InsertLineBreakCommand. See <https://webkit.org/b/56061>.
    … …  
    376385        textControl->selectionChanged(options.contains(FireSelectEvent));
    377386
    378     if (!didMutateSelection)
     387    if (!willMutateSelection)
    379388        return false;
    380389

  • trunk/Source/WebCore/editing/VisibleSelection.cpp

    r269136 r272928  
    132132{
    133133    setExtent(visiblePosition.deepEquivalent());
     134}
     135
     136bool VisibleSelection::isOrphan() const
     137{
     138    if (m_base.isOrphan() || m_extent.isOrphan() || m_start.isOrphan() || m_end.isOrphan())
     139        return true;
     140    if (m_anchor.isOrphan() && m_anchor.document()->settings().liveRangeSelectionEnabled())
     141        return true;
     142    if (m_focus.isOrphan() && m_focus.document()->settings().liveRangeSelectionEnabled())
     143        return true;
     144    return false;
    134145}
    135146

  • trunk/Source/WebCore/editing/VisibleSelection.h

    r267362 r272928  
    8787    bool isNonOrphanedRange() const { return isRange() && !start().isOrphan() && !end().isOrphan(); }
    8888    bool isNoneOrOrphaned() const { return isNone() || start().isOrphan() || end().isOrphan(); }
     89    bool isOrphan() const;
    8990
    9091    bool isBaseFirst() const { return m_anchorIsFirst; }

  • trunk/Source/WebCore/page/DOMSelection.cpp

    r272777 r272928  
    368368    if (auto shadowAncestor = selectionShadowAncestor(frame))
    369369        return createLiveRange(makeSimpleRange(*makeBoundaryPointBeforeNode(*shadowAncestor)));
    370     auto simpleRange = frame->selection().selection().firstRange();
    371     ASSERT(simpleRange); // selection is orphaned but this shouldn't happen.
    372     if (!simpleRange)
    373         return Exception { IndexSizeError };
    374     return createLiveRange(*simpleRange);
     370    return createLiveRange(*frame->selection().selection().firstRange());
    375371}
    376372

  • trunk/Source/WebCore/page/EventHandler.cpp

    r272503 r272928  
    467467    }
    468468
     469    if (selection.isOrphan()) {
     470        m_mouseDownMayStartSelect = false;
     471        return false;
     472    }
     473
    469474    if (selection.isRange())
    470475        m_selectionInitiationState = ExtendedSelection;
Note: See TracChangeset for help on using the changeset viewer.