Bug 61978 – --webkit-visual-word crash on mixed editability

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

previousWordPosition/nextWordPosition could return a NULL position, 
For example:
<div contenteditable ">abc hijopq</div>
<div dir=ltr class="test_move_by_word">abc hijopq</div>


Need to check against NULL before calling getInlineBoxAndOffset().

[Xiaomei Ji]

also crash on:
<div dir=ltr class="test_move_by_word">abc hijopq</div>
<div contenteditable dir=ltr class="test_move_by_word">abc hijopq</div>


how should the following behave? 
<div dir=ltr class="test_move_by_word">abc hijopq</div>
<div dir=ltr class="test_move_by_word">abc hijopq</div>

[Xiaomei Ji]

one more case reported in crbug.com/86332.

<script src="resources/move-by-word-visually.js"></script>
<script>

onload = function() {
    try {
        runTest();
    } finally {
    }
};

</script>
<div contenteditable></div>
00<base class="test_move_by_word" title="28 -4500000000 14 21 4|4 21 14 -4500000000">

[Xiaomei Ji]

Created attachment 98417 [details]
patch w/ layout test

Will add more test after issue 61344 (--webkit-word-visual across multi-line) is fixed.

[Ryosuke Niwa]

Comment on attachment 98417 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=98417&action=review

r- due to various nits.

> Source/WebCore/ChangeLog:9
> +        Fix 2 --webkit-visual-word crashes:
> +        --webkit-visual-word crash on mixed editability and
> +        https://bugs.webkit.org/show_bug.cgi?id=61978
> +        --webkit-visual-word crashes (VisiblePosition.getInlineBoxAndOffset could return null box)
> +        https://bugs.webkit.org/show_bug.cgi?id=62814

Nit: We normally just list two bug title + bug url separated by a blank line.

> Source/WebCore/editing/visible_units.cpp:1305
> +    bool positionInBox = positionIsInBox(wordBreak, box, offsetOfWordBreak);
> +    if (positionInBox && (previousOffset == invalidOffset || previousOffset < offsetOfWordBreak))

There's no need to declare a boolean here you can call positionIsInBox inside the if statement.  For that matter, this entire if statement is unnecessary.  You should just do:
return positionIsInBox(wordBreak, box, offsetOfWordBreak) && (previousOffset == invalidOffset || previousOffset < offsetOfWordBreak).

> Source/WebCore/editing/visible_units.cpp:1516
> +    bool positionInBox = positionIsInBox(wordBreak, box, offsetOfWordBreak);
> +    return positionInBox && offsetOfWordBreak != box->caretMaxOffset() && offsetOfWordBreak != box->caretMinOffset();

Ditto about the boolean.

[Xiaomei Ji]

Created attachment 100400 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 100400 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=100400&action=review

Due to various nits and missing null check.

> Source/WebCore/ChangeLog:10
> +        Fix 2 --webkit-visual-word crashes:
> +        --webkit-visual-word crash on mixed editability and
> +        --webkit-visual-word crashes (VisiblePosition.getInlineBoxAndOffset could return null box)
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=61978
> +        https://bugs.webkit.org/show_bug.cgi?id=62814

Nit: should be:
--webkit-visual-word crash on mixed editability
https://bugs.webkit.org/show_bug.cgi?id=61978

--webkit-visual-word crashes (VisiblePosition.getInlineBoxAndOffset could return null box)
https://bugs.webkit.org/show_bug.cgi?id=62814

> Source/WebCore/editing/visible_units.cpp:1230
> +    if (positionIsInBox(wordBreak, box, offsetOfWordBreak))
> +        return wordBreak;
> +    return VisiblePosition();

Seems like we should have a version of positionIsInBox that doesn't take offsetOfWordBreak.  Also I'd use tertiary here.

> Source/WebCore/editing/visible_units.cpp:1305
> +    if (positionIsInBox(wordBreak, box, offsetOfWordBreak))
>          return wordBreak;
>      return VisiblePosition();    

Ditto about using tertiary.

> Source/WebCore/editing/visible_units.cpp:1312
> +           && (previousOffset == invalidOffset || previousOffset < offsetOfWordBreak);

Nit: && should be intended by 4 spaces, and should NOT be aligned with positionIsInBox.

> Source/WebCore/editing/visible_units.cpp:1521
> +           && offsetOfWordBreak != box->caretMaxOffset() && offsetOfWordBreak != box->caretMinOffset();

Ditto about indentation.

> Source/WebCore/editing/visible_units.cpp:1604
> +    // FIXME: do I need to check visiblePosition.isNotNull() ?

Yes, you should.

[Xiaomei Ji]

Created attachment 100873 [details]
patch w/ layout test

[Xiaomei Ji]

Created attachment 100892 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 100892 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=100892&action=review

> Source/WebCore/ChangeLog:3
> +        Fix 2 --webkit-visual-word crashes:

Nit: we don't usually have this.

> Source/WebCore/ChangeLog:19
> +        Check previousBoundary/nextBoundary against NULL.
> +
> +        Have a static function to encapsulate the usage of getInlineBoxAndOffset and check the 
> +        visible position is not NULL before passing to getInlineBoxAndOffset.
> +
> +        Check the box returned from getInlineBoxAndOffset is not NULL before accessing.

Can we combine these into one paragraph?  I feel like it's scattered.

> LayoutTests/editing/selection/move-by-word-visually-null-box.html:13
> +        document.body.innerHTML = "Test Passed";

Maybe you should say "Crash test passed" instead to make it clear that this is a crash test?

> LayoutTests/platform/qt/Skipped:2486
> +editing/selection/move-by-word-visually-null-box.html

Why this is test skilled in Qt?

[Xiaomei Ji]

Comment on attachment 100892 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=100892&action=review

>> LayoutTests/platform/qt/Skipped:2486
>> +editing/selection/move-by-word-visually-null-box.html
> 
> Why this is test skilled in Qt?

QT has different word break behavior need investigate. This specific test might be ok to not skipped since it is just NULL tests against passing in NULL visiblePosition and against NULL box returned from VisiblePosition::getInlineBoxAndOffset().

[Xiaomei Ji]

Committed r91077: <http://trac.webkit.org/changeset/91077>