Bug 61978 - --webkit-visual-word crash on mixed editability
Summary: --webkit-visual-word crash on mixed editability
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2011-06-02 16:25 PDT by Xiaomei Ji
Modified: 2011-07-15 10:24 PDT (History)
1 user (show)

See Also:


Attachments
patch w/ layout test (14.04 KB, patch)
2011-06-23 15:15 PDT, Xiaomei Ji
rniwa: review-
Details | Formatted Diff | Diff
patch w/ layout test (14.14 KB, patch)
2011-07-11 18:10 PDT, Xiaomei Ji
rniwa: review-
Details | Formatted Diff | Diff
patch w/ layout test (14.76 KB, patch)
2011-07-14 15:25 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (14.94 KB, patch)
2011-07-14 16:27 PDT, Xiaomei Ji
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-06-02 16:25:54 PDT
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().
Comment 1 Xiaomei Ji 2011-06-02 17:04:28 PDT
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>
Comment 2 Xiaomei Ji 2011-06-16 12:31:32 PDT
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">
Comment 3 Xiaomei Ji 2011-06-23 15:15:24 PDT
Created attachment 98417 [details]
patch w/ layout test

Will add more test after issue 61344 (--webkit-word-visual across multi-line) is fixed.
Comment 4 Ryosuke Niwa 2011-07-11 16:59:49 PDT
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.
Comment 5 Xiaomei Ji 2011-07-11 18:10:58 PDT
Created attachment 100400 [details]
patch w/ layout test
Comment 6 Ryosuke Niwa 2011-07-14 14:31:27 PDT
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.
Comment 7 Xiaomei Ji 2011-07-14 15:25:41 PDT
Created attachment 100873 [details]
patch w/ layout test
Comment 8 Xiaomei Ji 2011-07-14 16:27:47 PDT
Created attachment 100892 [details]
patch w/ layout test
Comment 9 Ryosuke Niwa 2011-07-14 16:43:43 PDT
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?
Comment 10 Xiaomei Ji 2011-07-14 18:25:13 PDT
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().
Comment 11 Xiaomei Ji 2011-07-15 10:24:09 PDT
Committed r91077: <http://trac.webkit.org/changeset/91077>