Changeset 192170 in webkit


Ignore:
Timestamp:
Nov 9, 2015 12:11:30 PM (9 years ago)
Author:
jiewen_tan@apple.com
Message:

Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html
​https://bugs.webkit.org/show_bug.cgi?id=149298
<rdar://problem/22746918>

Reviewed by Ryosuke Niwa.

Source/WebCore:

The test crashes in the method WebCore::CompositeEditCommand::moveParagraphs() because
the other method WebCore::CompositeEditCommand::cleanupAfterDeletion() accidentally
deletes the destination node. In WebCore::CompositeEditCommand::cleanupAfterDeletion(),
it fails to determine that caretAfterDelete equals to destination as Position::operator==,
which is called in VisiblePosition::operator==, only checks the equality of tuple
<Anchor Node, Anchor Type, Offset>. It is insufficient as a single position can be
represented by multiple tuples. Therefore, this change adds Position::equals() to fortify
the equal checking of two positions by considering combinations of different tuple
representations.

Furthermore, it adds VisiblePosition::equals() which considers affinity and call
Position::equals() while comparing two visible positions.

Test: editing/inserting/insert-html-crash-01.html

  • dom/Position.cpp:

(WebCore::Position::equals):

  • dom/Position.h:
  • editing/CompositeEditCommand.cpp:

(WebCore::CompositeEditCommand::cleanupAfterDeletion):
Replace operator== with VisiblePosition::equals() to tackle the test case.

  • editing/VisiblePosition.cpp:

(WebCore::VisiblePosition::equals):

  • editing/VisiblePosition.h:

LayoutTests:

This test case is from Blink r153982:
​https://codereview.chromium.org/16053005

  • editing/inserting/insert-html-crash-01-expected.txt: Added.
  • editing/inserting/insert-html-crash-01.html: Added.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r192169 r192170  
     12015-11-09  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149298
     5        <rdar://problem/22746918>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        This test case is from Blink r153982:
     10        https://codereview.chromium.org/16053005
     11
     12        * editing/inserting/insert-html-crash-01-expected.txt: Added.
     13        * editing/inserting/insert-html-crash-01.html: Added.
     14
    1152015-11-09  Myles C. Maxfield  <mmaxfield@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r192169 r192170  
     12015-11-09  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html
     4        https://bugs.webkit.org/show_bug.cgi?id=149298
     5        <rdar://problem/22746918>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        The test crashes in the method WebCore::CompositeEditCommand::moveParagraphs() because
     10        the other method WebCore::CompositeEditCommand::cleanupAfterDeletion() accidentally
     11        deletes the destination node. In WebCore::CompositeEditCommand::cleanupAfterDeletion(),
     12        it fails to determine that caretAfterDelete equals to destination as Position::operator==,
     13        which is called in VisiblePosition::operator==, only checks the equality of tuple
     14        <Anchor Node, Anchor Type, Offset>. It is insufficient as a single position can be
     15        represented by multiple tuples. Therefore, this change adds Position::equals() to fortify
     16        the equal checking of two positions by considering combinations of different tuple
     17        representations.
     18
     19        Furthermore, it adds VisiblePosition::equals() which considers affinity and call
     20        Position::equals() while comparing two visible positions.
     21
     22        Test: editing/inserting/insert-html-crash-01.html
     23
     24        * dom/Position.cpp:
     25        (WebCore::Position::equals):
     26        * dom/Position.h:
     27        * editing/CompositeEditCommand.cpp:
     28        (WebCore::CompositeEditCommand::cleanupAfterDeletion):
     29        Replace operator== with VisiblePosition::equals() to tackle the test case.
     30        * editing/VisiblePosition.cpp:
     31        (WebCore::VisiblePosition::equals):
     32        * editing/VisiblePosition.h:
     33
    1342015-11-09  Myles C. Maxfield  <mmaxfield@apple.com>
    235

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

    r191738 r192170  
    14541454#endif
    14551455
     1456bool Position::equals(const Position& other) const
     1457{
     1458    if (!m_anchorNode)
     1459        return !m_anchorNode == !other.m_anchorNode;
     1460    if (!other.m_anchorNode)
     1461        return false;
     1462
     1463    switch (anchorType()) {
     1464    case PositionIsBeforeChildren:
     1465        ASSERT(!m_anchorNode->isTextNode());
     1466        switch (other.anchorType()) {
     1467        case PositionIsBeforeChildren:
     1468            ASSERT(!other.m_anchorNode->isTextNode());
     1469            return m_anchorNode == other.m_anchorNode;
     1470        case PositionIsAfterChildren:
     1471            ASSERT(!other.m_anchorNode->isTextNode());
     1472            return m_anchorNode == other.m_anchorNode && !m_anchorNode->hasChildNodes();
     1473        case PositionIsOffsetInAnchor:
     1474            return m_anchorNode == other.m_anchorNode && !other.m_offset;
     1475        case PositionIsBeforeAnchor:
     1476            return m_anchorNode->firstChild() == other.m_anchorNode;
     1477        case PositionIsAfterAnchor:
     1478            return false;
     1479        }
     1480        break;
     1481    case PositionIsAfterChildren:
     1482        ASSERT(!m_anchorNode->isTextNode());
     1483        switch (other.anchorType()) {
     1484        case PositionIsBeforeChildren:
     1485            ASSERT(!other.m_anchorNode->isTextNode());
     1486            return m_anchorNode == other.m_anchorNode && !m_anchorNode->hasChildNodes();
     1487        case PositionIsAfterChildren:
     1488            ASSERT(!other.m_anchorNode->isTextNode());
     1489            return m_anchorNode == other.m_anchorNode;
     1490        case PositionIsOffsetInAnchor:
     1491            return m_anchorNode == other.m_anchorNode && m_anchorNode->countChildNodes() == static_cast<unsigned>(m_offset);
     1492        case PositionIsBeforeAnchor:
     1493            return false;
     1494        case PositionIsAfterAnchor:
     1495            return m_anchorNode->lastChild() == other.m_anchorNode;
     1496        }
     1497        break;
     1498    case PositionIsOffsetInAnchor:
     1499        switch (other.anchorType()) {
     1500        case PositionIsBeforeChildren:
     1501            ASSERT(!other.m_anchorNode->isTextNode());
     1502            return m_anchorNode == other.m_anchorNode && !m_offset;
     1503        case PositionIsAfterChildren:
     1504            ASSERT(!other.m_anchorNode->isTextNode());
     1505            return m_anchorNode == other.m_anchorNode && m_offset == static_cast<int>(other.m_anchorNode->countChildNodes());
     1506        case PositionIsOffsetInAnchor:
     1507            return m_anchorNode == other.m_anchorNode && m_offset == other.m_offset;
     1508        case PositionIsBeforeAnchor:
     1509            return m_anchorNode->traverseToChildAt(m_offset) == other.m_anchorNode;
     1510        case PositionIsAfterAnchor:
     1511            return m_offset && m_anchorNode->traverseToChildAt(m_offset - 1) == other.m_anchorNode;
     1512        }
     1513        break;
     1514    case PositionIsBeforeAnchor:
     1515        switch (other.anchorType()) {
     1516        case PositionIsBeforeChildren:
     1517            ASSERT(!other.m_anchorNode->isTextNode());
     1518            return m_anchorNode == other.m_anchorNode->firstChild();
     1519        case PositionIsAfterChildren:
     1520            ASSERT(!other.m_anchorNode->isTextNode());
     1521            return false;
     1522        case PositionIsOffsetInAnchor:
     1523            return m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset);
     1524        case PositionIsBeforeAnchor:
     1525            return m_anchorNode == other.m_anchorNode;
     1526        case PositionIsAfterAnchor:
     1527            return m_anchorNode->previousSibling() == other.m_anchorNode;
     1528        }
     1529        break;
     1530    case PositionIsAfterAnchor:
     1531        switch (other.anchorType()) {
     1532        case PositionIsBeforeChildren:
     1533            ASSERT(!other.m_anchorNode->isTextNode());
     1534            return false;
     1535        case PositionIsAfterChildren:
     1536            ASSERT(!other.m_anchorNode->isTextNode());
     1537            return m_anchorNode == other.m_anchorNode->lastChild();
     1538        case PositionIsOffsetInAnchor:
     1539            return other.m_offset && m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset - 1);
     1540        case PositionIsBeforeAnchor:
     1541            return m_anchorNode->nextSibling() == other.m_anchorNode;
     1542        case PositionIsAfterAnchor:
     1543            return m_anchorNode == other.m_anchorNode;
     1544        }
     1545        break;
     1546    }
     1547
     1548    ASSERT_NOT_REACHED();
     1549    return false;
     1550}
     1551
    14561552} // namespace WebCore
    14571553

  • trunk/Source/WebCore/dom/Position.h

    r191955 r192170  
    208208    void showTreeForThis() const;
    209209#endif
    210    
     210
     211    // This is a tentative enhancement of operator== to account for different position types.
     212    // FIXME: Combine this function with operator==
     213    bool equals(const Position&) const;
    211214private:
    212215    WEBCORE_EXPORT int offsetForPositionAfterAnchor() const;

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

    r191671 r192170  
    11211121{
    11221122    VisiblePosition caretAfterDelete = endingSelection().visibleStart();
    1123     if (caretAfterDelete != destination && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
     1123    if (!caretAfterDelete.equals(destination) && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
    11241124        // Note: We want the rightmost candidate.
    11251125        Position position = caretAfterDelete.deepEquivalent().downstream();

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

    r191671 r192170  
    744744}
    745745
     746bool VisiblePosition::equals(const VisiblePosition& other) const
     747{
     748    return m_affinity == other.m_affinity && m_deepPosition.equals(other.m_deepPosition);
     749}
     750
    746751}  // namespace WebCore
    747752

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

    r182207 r192170  
    101101    WEBCORE_EXPORT int lineDirectionPointForBlockDirectionNavigation() const;
    102102
     103    // This is a tentative enhancement of operator== to account for affinity.
     104    // FIXME: Combine this function with operator==
     105    bool equals(const VisiblePosition&) const;
     106
    103107#if ENABLE(TREE_DEBUGGING)
    104108    void debugPosition(const char* msg = "") const;
Note: See TracChangeset for help on using the changeset viewer.