Changeset 201667 in webkit


Ignore:
Timestamp:
Jun 3, 2016 4:01:49 PM (8 years ago)
Author:
rniwa@webkit.org
Message:

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.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r201666 r201667  
     12016-06-03  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash under VisibleSelection::firstRange()
     4        https://bugs.webkit.org/show_bug.cgi?id=158241
     5
     6        Reviewed by Enrica Casucci.
     7
     8        Added a regression test.
     9
     10        * fast/shadow-dom/selection-at-shadow-root-crash-expected.txt: Added.
     11        * fast/shadow-dom/selection-at-shadow-root-crash.html: Added.
     12
    1132016-06-03  Zalan Bujtas  <zalan@apple.com>
    214
  • trunk/Source/WebCore/ChangeLog

    r201666 r201667  
     12016-06-03  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash under VisibleSelection::firstRange()
     4        https://bugs.webkit.org/show_bug.cgi?id=158241
     5
     6        Reviewed by Enrica Casucci.
     7
     8        The crash was commonly caused by parentAnchoredEquivalent returning null when the anchored node was a shadow root.
     9        Fixed it by returning a shadow root in parentAnchoredEquivalent.
     10
     11        Also guard against other kinds of crashes by adding a null check in VisibleSelection::firstRange() since we've seen
     12        a crash in the same code path outside of a shadow tree.
     13
     14        This patch also fixes other Position methods to stop using nonShadowBoundaryParentNode in place of parentNode as
     15        that would cause a similar crash and/or a bug elsewhere.
     16
     17        Test: fast/shadow-dom/selection-at-shadow-root-crash.html
     18
     19        * accessibility/AXObjectCache.cpp:
     20        (AXObjectCache::startCharacterOffsetOfParagraph): Fixed a bug uncovered by the assertion fix in Position::Position.
     21        This code was sometimes creating a position inside a BR, which is wrong.
     22        (AXObjectCache::endCharacterOffsetOfParagraph): Ditto.
     23        * dom/Position.cpp:
     24        (WebCore::Position::Position): Fixed an assertion which was checking that this constructor wasn't being called
     25        with m_anchorNode set to an element editing ignores content of. ||ing it with isShadowRoot() made this assertion
     26        useless because it's true whenever m_anchorNode is not a shadow root.
     27        (WebCore::Position::containerNode): Use parentNode() instead of findParent() which calls nonShadowBoundaryParentNode
     28        since Position should
     29        (WebCore::Position::parentAnchoredEquivalent): Fixed the bug by letting this function return a shadow root.
     30        (WebCore::Position::previous): Use parentNode() instead of findParent().
     31        (WebCore::Position::next): Ditto.
     32        (WebCore::Position::atStartOfTree): Ditto.
     33        (WebCore::Position::atEndOfTree): Ditto.
     34        (WebCore::Position::findParent): Deleted.
     35        * dom/Position.h:
     36        * editing/VisibleSelection.cpp:
     37        (VisibleSelection::firstRange): Added a null check.
     38
    1392016-06-03  Zalan Bujtas  <zalan@apple.com>
    240
  • trunk/Source/WebCore/accessibility/AXObjectCache.cpp

    r201443 r201667  
    23302330    auto* startBlock = enclosingBlock(startNode);
    23312331    int offset = characterOffset.startIndex + characterOffset.offset;
    2332     Position p(startNode, offset, Position::PositionIsOffsetInAnchor);
    2333     auto* highestRoot = highestEditableRoot(p);
     2332    auto* highestRoot = highestEditableRoot(firstPositionInOrBeforeNode(startNode));
    23342333    Position::AnchorType type = Position::PositionIsOffsetInAnchor;
    23352334   
     
    23532352    Node* stayInsideBlock = enclosingBlock(startNode);
    23542353    int offset = characterOffset.startIndex + characterOffset.offset;
    2355     Position p(startNode, offset, Position::PositionIsOffsetInAnchor);
    2356     Node* highestRoot = highestEditableRoot(p);
     2354    Node* highestRoot = highestEditableRoot(firstPositionInOrBeforeNode(startNode));
    23572355    Position::AnchorType type = Position::PositionIsOffsetInAnchor;
    23582356   
  • trunk/Source/WebCore/dom/Position.cpp

    r201205 r201667  
    128128    , m_isLegacyEditingPosition(false)
    129129{
    130     ASSERT(!m_anchorNode || !editingIgnoresContent(*m_anchorNode) || !m_anchorNode->isShadowRoot());
     130    ASSERT(!m_anchorNode || !editingIgnoresContent(*m_anchorNode));
    131131    ASSERT(!m_anchorNode || !m_anchorNode->isPseudoElement());
    132132    ASSERT(anchorType == PositionIsOffsetInAnchor);
     
    171171    case PositionIsBeforeAnchor:
    172172    case PositionIsAfterAnchor:
    173         return findParent(*m_anchorNode);
     173        return m_anchorNode->parentNode();
    174174    }
    175175    ASSERT_NOT_REACHED();
     
    232232    // FIXME: This should only be necessary for legacy positions, but is also needed for positions before and after Tables
    233233    if (m_offset <= 0 && (m_anchorType != PositionIsAfterAnchor && m_anchorType != PositionIsAfterChildren)) {
    234         if (findParent(*m_anchorNode) && (editingIgnoresContent(*m_anchorNode) || isRenderedTable(m_anchorNode.get())))
     234        if (m_anchorNode->parentNode() && (editingIgnoresContent(*m_anchorNode) || isRenderedTable(m_anchorNode.get())))
    235235            return positionInParentBeforeNode(m_anchorNode.get());
    236236        return Position(m_anchorNode.get(), 0, PositionIsOffsetInAnchor);
     
    345345    }
    346346
    347     ContainerNode* parent = findParent(*node);
     347    ContainerNode* parent = node->parentNode();
    348348    if (!parent)
    349349        return *this;
     
    392392    }
    393393
    394     ContainerNode* parent = findParent(*node);
     394    ContainerNode* parent = node->parentNode();
    395395    if (!parent)
    396396        return *this;
     
    494494
    495495    Node* container = containerNode();
    496     if (container && findParent(*container))
     496    if (container && container->parentNode())
    497497        return false;
    498498
     
    519519
    520520    Node* container = containerNode();
    521     if (container && findParent(*container))
     521    if (container && container->parentNode())
    522522        return false;
    523523
     
    954954}
    955955
    956 ContainerNode* Position::findParent(const Node& node)
    957 {
    958     return node.nonShadowBoundaryParentNode();
    959 }
    960 
    961956#if ENABLE(USERSELECT_ALL)
    962957bool Position::nodeIsUserSelectAll(const Node* node)
  • trunk/Source/WebCore/dom/Position.h

    r195237 r201667  
    200200    static Node* rootUserSelectAllForNode(Node*) { return 0; }
    201201#endif
    202     static ContainerNode* findParent(const Node&);
    203    
     202
    204203    void debugPosition(const char* msg = "") const;
    205204
  • trunk/Source/WebCore/editing/VisiblePosition.cpp

    r201205 r201667  
    588588    // If the html element is editable, descending into its body will look like a descent
    589589    // from non-editable to editable content since rootEditableElement() always stops at the body.
    590     if ((editingRoot && editingRoot->hasTagName(htmlTag)) || position.deprecatedNode()->isDocumentNode())
     590    if ((editingRoot && editingRoot->hasTagName(htmlTag)) || (node && (node->isDocumentNode() || node->isShadowRoot())))
    591591        return next.isNotNull() ? next : prev;
    592592       
  • trunk/Source/WebCore/editing/VisibleSelection.cpp

    r200931 r201667  
    130130    Position start = m_start.parentAnchoredEquivalent();
    131131    Position end = m_end.parentAnchoredEquivalent();
     132    if (start.isNull() || end.isNull())
     133        return nullptr;
    132134    return Range::create(start.anchorNode()->document(), start, end);
    133135}
Note: See TracChangeset for help on using the changeset viewer.