Bug 151442 – Selection.deleteFromDocument should not leave a selection character

• Component: HTML Editing
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is what happens with test imported/blink/editing/selection/deleteFromDocument-crash.html. It first run selectAll in the focused text area, and removeFromDocument() leaves the last character of the selection in the text area. This is what is causing a failure in GTK+ and win ports. The reason why it passes for mac is because the mac editor behaviour considers selections as not directional. For non directional selections, FrameSelection::willBeModified() when direction is backwards switches the selection base extent. So, when the caret is moved once character backwards in the case of mac, the cursor position is 0 and it doesn't have any effect. For the other ports, the position is the last one (extent). This was fixed in blink r172511, so we could just merge it in WebKit.

[Carlos Garcia Campos]

Created attachment 265857 [details]
Patch

[Build Bot]

Comment on attachment 265857 [details]
Patch

Attachment 265857 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/449673

New failing tests:
imported/blink/editing/selection/deleteFromDocument-undo-crash.html

[Build Bot]

Created attachment 265861 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 265857 [details]
Patch

EWS shows the test deleteFromDocument-undo-crash.html is hitting an assertion on Mac debug build. We’ll need to find out what that assertion is to fix it.

[Carlos Garcia Campos]

Yes, I'll check what assert we are hitting.

[Carlos Garcia Campos]

It looks like a different issue revealed by the new test. I can reproduce it without the patch. See https://code.google.com/p/chromium/issues/detail?id=504932

[Carlos Garcia Campos]

Created attachment 265957 [details]
Updated patch

It skips the new test for Debug.

[Darin Adler]

Comment on attachment 265957 [details]
Updated patch

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

> LayoutTests/TestExpectations:455
> +webkit.org/b/151488 [ Debug ] imported/blink/editing/selection/deleteFromDocument-undo-crash.html [ Skip ]

I’d prefer that we investigate this first, and fix this before making the rest of this change. I don’t want to rush to get this change in and knowingly introduce a crash.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 265957 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265957&action=review
> 
> > LayoutTests/TestExpectations:455
> > +webkit.org/b/151488 [ Debug ] imported/blink/editing/selection/deleteFromDocument-undo-crash.html [ Skip ]
> 
> I’d prefer that we investigate this first, and fix this before making the
> rest of this change. I don’t want to rush to get this change in and
> knowingly introduce a crash.

Note that this doesn't introduce a crash, but a test that reveals an existing crash. The new test crashes in debug build even without the changes introduced in this patch.

[Darin Adler]

(In reply to comment #9)
> Note that this doesn't introduce a crash, but a test that reveals an
> existing crash. The new test crashes in debug build even without the changes
> introduced in this patch.

Still seems that we should fix that bug before making this behavior change, though. Should not be a challenging project and there is no hurry to change this behavior since we have had it for a long time.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > Note that this doesn't introduce a crash, but a test that reveals an
> > existing crash. The new test crashes in debug build even without the changes
> > introduced in this patch.
> 
> Still seems that we should fix that bug before making this behavior change,
> though. Should not be a challenging project and there is no hurry to change
> this behavior since we have had it for a long time.

This behavior has always been there, but we didn't know it caused a crash (a real one, not an assert, so it happens in release as well) until editing/selection/deleteFromDocument-crash.html was imported from blink. That test crashes for all ports not using the mac editing behavior. This patch also adds editing/selection/deleteFromDocument-undo-crash.html, but because it's a blink merge and the test was part of the commit. So, what I can do, is fixing this real crash for the existing test, and add the new test as part of the bug #151488 with the patch to fix it.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Note that this doesn't introduce a crash, but a test that reveals an
> > > existing crash. The new test crashes in debug build even without the changes
> > > introduced in this patch.
> > 
> > Still seems that we should fix that bug before making this behavior change,
> > though. Should not be a challenging project and there is no hurry to change
> > this behavior since we have had it for a long time.
> 
> This behavior has always been there, but we didn't know it caused a crash (a
> real one, not an assert, so it happens in release as well) until
> editing/selection/deleteFromDocument-crash.html was imported from blink.
> That test crashes for all ports not using the mac editing behavior. This
> patch also adds editing/selection/deleteFromDocument-undo-crash.html, but
> because it's a blink merge and the test was part of the commit. So, what I
> can do, is fixing this real crash for the existing test, and add the new
> test as part of the bug #151488 with the patch to fix it.

I'm wrong, I think I was confusing this with other test, this is just a failure, not a crash.

[Michael Catanzaro]

Comment on attachment 265957 [details]
Updated patch

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

I see you've opened bug #151488 to address Darin's concerns about the new test crashing in debug builds, so I think this is fine for now, since it fixes a crash in release builds, adds a new test, cleans up an existing test, and matches the behavior of IE/FF/Blink.

> LayoutTests/editing/selection/deleteFromDocument.html:36
> +r.setEnd(span2.firstChild, 3);

Can you explain why you changed this?

[Carlos Garcia Campos]

Created attachment 276467 [details]
Rebased patch for landing

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 265957 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265957&action=review
> 
> I see you've opened bug #151488 to address Darin's concerns about the new
> test crashing in debug builds, so I think this is fine for now, since it
> fixes a crash in release builds, adds a new test, cleans up an existing
> test, and matches the behavior of IE/FF/Blink.
> 
> > LayoutTests/editing/selection/deleteFromDocument.html:36
> > +r.setEnd(span2.firstChild, 3);
> 
> Can you explain why you changed this?

No, because I'm not the author of the patch, as the ChangeLog says, this is a merge of Blink r172511

[Carlos Garcia Campos]

Committed r199585: <http://trac.webkit.org/changeset/199585>