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.
Created attachment 60303 [details] Patch - reverses the order we iterate over the text boxes in when there is rtl text.
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.
> +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?
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.
(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.
Created attachment 61120 [details] Patch - New approach, sort the text boxes before using them.
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.
Enrica is on vacation. She returns Monday.
Enrica, can you take a look at this? (Welcome back from vacation!)
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.
Comment on attachment 61120 [details] Patch - New approach, sort the text boxes before using them. r=me
(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.
(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.
Comment on attachment 61120 [details] Patch - New approach, sort the text boxes before using them. Shall we land this patch then?
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>
All reviewed patches have been landed. Closing bug.