Changeset 248909 in webkit


Ignore:
Timestamp:
Aug 20, 2019 11:46:36 AM (5 years ago)
Author:
Wenson Hsieh
Message:

Clicking the search icon on ae.com hangs the web content process
​https://bugs.webkit.org/show_bug.cgi?id=200889
<rdar://problem/54359330>

Reviewed by Ryosuke Niwa.

Source/WebCore:

The hang occurs under FrameSelection::selectionAtSentenceStart, while computing an EditorState to send to the UI
process. This act of determining whether the given positon is at the start of sentence entails moving backwards
from the start of the current visible selection until the start of a paragraph or sentence is found, using
VisiblePosition::previous to iterate backwards through VisiblePositions.

However, on this website, VisiblePosition::previous ends up just returning the current position, and we loop
infinitely as a result because we never actually move backwards. This happens because VisiblePosition::previous
first uses previousVisuallyDistinctCandidate to find a candidate Position before the current position, but when
the position is canonicalized to create a VisiblePosition, it is moved back to its original Position as the deep
equivalent.

In the attached test case (which is representative of the relevant part of the DOM on ae.com), we try to find
the previous VisiblePosition from (#c, 0). The previous visually distinct candidate we initially find is
(#b, 0), since:

  1. The enclosing renderer is a RenderBlock with a non-zero height.
  2. The enclosing renderer has no rendered children.
  3. The position is at the first editing position in the node (i.e. the span element).

However, when canonicalizing the position, we find that neither the upstream nor the downstream position is a
candidate because both the upstream and downstream nodes end up well outside of the span (the upstream node ends
up being at the start of the body element, and the downstream position ends up right before the start of #c's
container). The downstream position is at the end of a text node with a leading newline, it's not a candidate
because its last caret offset is less than the length of the text node.

As a result, even though the given position (#b, 0) is a candidate itself, its downstream and upstream positions
are not. Thus, VisiblePosition::canonicalPosition expands the scope of its candidate positions to the next
closest candidate positions; the next candidate position is (#c, 0). Both of these candidates are outside of the
containing block, so we (somewhat arbitrarily) break the tie by choosing the next visible position, bringing us
back to (#c, 0).

There are several ways to fix this, one of which involves fixing the downstream/upstream positions of (#b, 0) so
that they no longer jump out of the containing block of #b and cause (#b, 0) to be an invalid visible position
despite being a candidate position. This can be achieved by adjusting the heuristic in
endsOfNodeAreVisuallyDistinctPositions (used when moving upstream or downstream). Currently, this helper
function returns false for #b because they contain a single (non-rendered) whitespace character. Removing this
extraneous whitespace character actually causes the problem to stop reproducing, since #b and #c no longer
contain any child nodes. This is important because the heuristic in Position::downstream attempts to keep the
downstream position within the confines of the enclosing visual boundary, which (currently) ends up being the
entire body element because endsOfNodeAreVisuallyDistinctPositions returns false for #b.

To avoid this scenario, we teach endsOfNodeAreVisuallyDistinctPositions to treat inline-block containers that
are empty (that is, contain no rendered content) but may have children for editing in the same way as inline-
block containers that don't have any children; in both scenarios, they may contain a candidate position, so we
should treat the ends of the container node as being visually distinct.

Doing so causes the downstream position of (#b, 0) to be kept within the immediate containing span element,
which then allows (#b, 0) to be a canonical VisiblePosition.

Tests: fast/events/focus-anchor-with-tabindex-hang.html

editing/selection/modify-backward-inline-block-containers.html

  • editing/VisiblePosition.cpp:

(WebCore::VisiblePosition::previous const):

LayoutTests:

  • editing/selection/modify-backward-inline-block-containers-expected.txt: Added.
  • editing/selection/modify-backward-inline-block-containers.html: Added.

Add a layout test to ensure that the selection may be moved through empty inline-block containers that span the
width of the page.

  • fast/events/focus-anchor-with-tabindex-hang-expected.txt: Added.
  • fast/events/focus-anchor-with-tabindex-hang.html: Added.

Add a layout test to ensure that clicking an empty span under a focusable anchor element moves focus to the
anchor element instead of hanging the web content process or hitting a debug assertion.

Location:
trunk
Files:
4 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r248908 r248909  
     12019-08-20  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Clicking the search icon on ae.com hangs the web content process
     4        https://bugs.webkit.org/show_bug.cgi?id=200889
     5        <rdar://problem/54359330>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        * editing/selection/modify-backward-inline-block-containers-expected.txt: Added.
     10        * editing/selection/modify-backward-inline-block-containers.html: Added.
     11
     12        Add a layout test to ensure that the selection may be moved through empty inline-block containers that span the
     13        width of the page.
     14
     15        * fast/events/focus-anchor-with-tabindex-hang-expected.txt: Added.
     16        * fast/events/focus-anchor-with-tabindex-hang.html: Added.
     17
     18        Add a layout test to ensure that clicking an empty span under a focusable anchor element moves focus to the
     19        anchor element instead of hanging the web content process or hitting a debug assertion.
     20
    1212019-08-20  Ryan Haddad  <ryanhaddad@apple.com>
    222

  • trunk/Source/WebCore/ChangeLog

    r248907 r248909  
     12019-08-20  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Clicking the search icon on ae.com hangs the web content process
     4        https://bugs.webkit.org/show_bug.cgi?id=200889
     5        <rdar://problem/54359330>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        The hang occurs under FrameSelection::selectionAtSentenceStart, while computing an EditorState to send to the UI
     10        process. This act of determining whether the given positon is at the start of sentence entails moving backwards
     11        from the start of the current visible selection until the start of a paragraph or sentence is found, using
     12        VisiblePosition::previous to iterate backwards through VisiblePositions.
     13
     14        However, on this website, VisiblePosition::previous ends up just returning the current position, and we loop
     15        infinitely as a result because we never actually move backwards. This happens because VisiblePosition::previous
     16        first uses previousVisuallyDistinctCandidate to find a candidate Position before the current position, but when
     17        the position is canonicalized to create a VisiblePosition, it is moved back to its original Position as the deep
     18        equivalent.
     19
     20        In the attached test case (which is representative of the relevant part of the DOM on ae.com), we try to find
     21        the previous VisiblePosition from (#c, 0). The previous visually distinct candidate we initially find is
     22        (#b, 0), since:
     23
     24        1. The enclosing renderer is a RenderBlock with a non-zero height.
     25        2. The enclosing renderer has no rendered children.
     26        3. The position is at the first editing position in the node (i.e. the span element).
     27
     28        However, when canonicalizing the position, we find that neither the upstream nor the downstream position is a
     29        candidate because both the upstream and downstream nodes end up well outside of the span (the upstream node ends
     30        up being at the start of the body element, and the downstream position ends up right before the start of #c's
     31        container). The downstream position is at the end of a text node with a leading newline, it's not a candidate
     32        because its last caret offset is less than the length of the text node.
     33
     34        As a result, even though the given position (#b, 0) is a candidate itself, its downstream and upstream positions
     35        are not. Thus, VisiblePosition::canonicalPosition expands the scope of its candidate positions to the next
     36        closest candidate positions; the next candidate position is (#c, 0). Both of these candidates are outside of the
     37        containing block, so we (somewhat arbitrarily) break the tie by choosing the next visible position, bringing us
     38        back to (#c, 0).
     39
     40        There are several ways to fix this, one of which involves fixing the downstream/upstream positions of (#b, 0) so
     41        that they no longer jump out of the containing block of #b and cause (#b, 0) to be an invalid visible position
     42        despite being a candidate position. This can be achieved by adjusting the heuristic in
     43        endsOfNodeAreVisuallyDistinctPositions (used when moving upstream or downstream). Currently, this helper
     44        function returns false for #b because they contain a single (non-rendered) whitespace character. Removing this
     45        extraneous whitespace character actually causes the problem to stop reproducing, since #b and #c no longer
     46        contain any child nodes. This is important because the heuristic in Position::downstream attempts to keep the
     47        downstream position within the confines of the enclosing visual boundary, which (currently) ends up being the
     48        entire body element because endsOfNodeAreVisuallyDistinctPositions returns false for #b.
     49
     50        To avoid this scenario, we teach endsOfNodeAreVisuallyDistinctPositions to treat inline-block containers that
     51        are empty (that is, contain no rendered content) but may have children for editing in the same way as inline-
     52        block containers that don't have any children; in both scenarios, they may contain a candidate position, so we
     53        should treat the ends of the container node as being visually distinct.
     54
     55        Doing so causes the downstream position of (#b, 0) to be kept within the immediate containing span element,
     56        which then allows (#b, 0) to be a canonical VisiblePosition.
     57
     58        Tests:  fast/events/focus-anchor-with-tabindex-hang.html
     59                editing/selection/modify-backward-inline-block-containers.html
     60
     61        * editing/VisiblePosition.cpp:
     62        (WebCore::VisiblePosition::previous const):
     63
    1642019-08-20  Brent Fulgham  <bfulgham@apple.com>
    265

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

    r240905 r248909  
    626626        return false;
    627627   
     628    if (!node->renderer()->isReplaced() || !canHaveChildrenForEditing(*node) || !downcast<RenderBox>(*node->renderer()).height())
     629        return false;
     630
    628631    // There is a VisiblePosition inside an empty inline-block container.
    629     return node->renderer()->isReplaced() && canHaveChildrenForEditing(*node) && downcast<RenderBox>(*node->renderer()).height() && !node->firstChild();
     632    if (!node->hasChildNodes())
     633        return true;
     634
     635    return !Position::hasRenderedNonAnonymousDescendantsWithHeight(downcast<RenderElement>(*node->renderer()));
    630636}
    631637
Note: See TracChangeset for help on using the changeset viewer.