Bug 41485 – Crash in replaceSelectionCommand with RTL text

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

[Julie Parent]

Created attachment 60288 [details]
Test case for crash

Repro steps:
1. Text that is followed by a space, in RTL text, in white-space:pre contentEditable/text input/textarea (Example: <div id='foo' style='white-space:pre' contentEditable >text </div>)
1. Select the word (example: "text", not the space after it)
2. Do anything that calls replaceSelection.

Result: Crash.
Crash seems to happens because in DeleteSelectionCommand, deleteInsignificantTextDownstream deletes the whitespace, but then fixupWhitespace still expects the space to be there, and crashes because there is no longer a text node (the text has been deleted, and the whitespace, so there is no longer a text node).

The core bug seems to be in CompositeEditCommand::deleteInsignificantText, where it determines that it should remove the space.  I *think* the issue is that it needs to iterate through the text boxes in reverse order for RTL case, but I'm not positive.  Testing that out now.

[Julie Parent]

Created attachment 60303 [details]
Patch - reverses the order we iterate over the text boxes in when there is rtl text.

[WebKit Review Bot]

Attachment 60303 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

> +document.getSelection().setBaseAndExtent(foo.firstChild, 0, foo.firstChild, 4);
> +document.execCommand('InsertImage', false, "../resources/abe.png");
> +</script>
> \ No newline at end of file
> 
> Property changes on: LayoutTests/editing/execCommand/insert-image-on-top-of-rtl.html
> ___________________________________________________________________
> Added: svn:executable
>    + *
> 

Drive by nit: no newline at end of file (just to get rid of the annoying warning from svn) and remove the svn:executable prop?

Why is this correct? Is it correcrt to always iterate from last to first? If not, why not?

[Julie Parent]

Comment on attachment 60303 [details]
Patch - reverses the order we iterate over the text boxes in when there is rtl text.

Removing r?, as I've found some issues with this.

[Julie Parent]

(Added original author and reviewer of the code in question)

Additional details on what is going on here, and why my initial patch is wrong:

The crash only happens inside a dir=rtl area, when the text is english, with a space.  Specifically, given the following 4 cases:
1. RTL region, english text: <div dir='rtl' style='white-space:pre' contentEditable >the </div>
2. LTR region, english text: <div dir='ltr' style='white-space:pre' contentEditable >the </div>
3. RTL region, arabic text: <div dir='rtl' style='white-space:pre' contentEditable >אחת </div>
4. LTR region, arabic text: <div dir='rtl' style='white-space:pre' contentEditable >אחת </div>

Currently, the crash only happens with case #1.  With my patch, #1 doesn't crash, but #4 does.

For these cases, we have the following InlineTextBoxes.

Case 1:
Box: start 3, len 1.  Next box: start 0, len 3 (box for the space with direction = RTL, box for "the" with direction = LTR)
Case 2:
Box: start 0, len 4 (box for the whole thing "the ", direction LTR)
Case 3:
Box: start 0, len 4 (box for the whole thing "אחת ", direction RTL)
Case 4:
Box: start 0, len 3. Next box: start 3, len 1 (box for "אחת" with direction RTL, box for space direction LTR)

So in the case of single directionality, there is a single box spanning the entire thing.  In the cases of mixed directionality, there is a box for the word and a box for the space.

Inside CompositeEditCommand::deleteInsignificantText, it tries to compute "gaps" using these boxes.  In case #1, it computes 0-3 aka, "the" as a gap, although it is text.  I think that is what goes wrong, but I don't really understand the code well enough.

My patch changed it to start with the last box rather than the first in the case of RTL, but that just swapped cases 1 and 4, so clearly isn't correct.

Anyone familiar with that code have any thoughts?  This is 6 years old, from http://trac.webkit.org/changeset/7875, and doesn't seem like it has been touched much.

[Julie Parent]

Created attachment 61120 [details]
Patch - New approach, sort the text boxes before using them.

[Julie Parent]

Any chance someone can review this?  This is a pretty frequent crasher, since this code path can be hit in a number of ways - paste, execCommands that replace the selection, Chromium's spell check when you replace the word (Safari didn't seem to crash with spell check), etc.

[Darin Adler]

Enrica is on vacation. She returns Monday.

[Julie Parent]

Enrica, can you take a look at this?  (Welcome back from vacation!)

[Eric Seidel (no email)]

Comment on attachment 61120 [details]
Patch - New approach, sort the text boxes before using them.

Talking about this with jparent in person this afternoon.

It seems strange to me that Editing would really need to walk the linebox tree like this.  And that walking the linebox in the manner would end up in an editing-only file.  The linebox-based approach may be wrong?

Would be nice to hear what Mitz has to say.

[Justin Garcia]

Comment on attachment 61120 [details]
Patch - New approach, sort the text boxes before using them.

r=me

[Justin Garcia]

(In reply to comment #11)
> It seems strange to me that Editing would really need to walk the linebox tree like this.  And that walking the linebox in the manner would end up in an editing-only file.

I don't know, we do this all over editing.  The code that the patch cleans up in TextIterator, for example.

[Enrica Casucci]

(In reply to comment #13)
> (In reply to comment #11)
> > It seems strange to me that Editing would really need to walk the linebox tree like this.  And that walking the linebox in the manner would end up in an editing-only file.
> 
> I don't know, we do this all over editing.  The code that the patch cleans up in TextIterator, for example.

I agree with Justin. Julie's approach does not introduce the use of line boxes, it just makes it right.
It looks ok to me.

[Adam Barth]

Comment on attachment 61120 [details]
Patch - New approach, sort the text boxes before using them.

Shall we land this patch then?

[WebKit Commit Bot]

Comment on attachment 61120 [details]
Patch - New approach, sort the text boxes before using them.

Clearing flags on attachment: 61120

Committed r65144: <http://trac.webkit.org/changeset/65144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.