Bug 10144 – REGRESSION: Reproducible assertion failure in DeleteSelectionCommand::fixupWhitespace()

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

[David Kilzer (:ddkilzer)]

Steps to reproduce:

1. Open Bug 8278.  http://bugzilla.opendarwin.org/show_bug.cgi?id=8278
2. Click "[reply]" link on Bug 8278 Comment #5.  http://bugzilla.opendarwin.org/show_bug.cgi?id=8278#c5
3. Make sure the cursor is at the very end of the text in the "Additional Comments" textarea.
4. Hit Delete key four times.

Expected results:

Four characters are deleted with no assertion error.

Actual results:

An assertion error occurs when the fourth character ('>') is deleted:

=================
ASSERTION FAILED: !textNode->renderer() || textNode->renderer()->style()->collapseWhiteSpace() (/Users/ddkilzer/Projects/Cocoa/WebKit/WebCore/editing/DeleteSelectionCommand.cpp:399 void WebCore::DeleteSelectionCommand::fixupWhitespace())
=================

Regression:

Does not occur on production Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC).

Notes:

Reproduced on locally-built debug build of WebKit r15648.

[David Kilzer (:ddkilzer)]

Created attachment 9743 [details]
Crash log from assertion failure

[Darin Adler]

I'm the one to blame for this assertion. Added just recently.

[Darin Adler]

Once you get past the assertion, you find that you can't delete any further -- the line boxes are messed up. I have a tentative fix. It affects a lot of layout tests, and I don't yet have a reduction to use as a test case.

[Darin Adler]

Created attachment 9758 [details]
patch, but no change log or test cases or test result updates yet

[Stephanie Lewis]

radar 4823044

[Beth Dakin]

Created attachment 11451 [details]
Darin's patch, merges with today's TOT

This is just Darin's patch merged with today's TOT.

[Beth Dakin]

Created attachment 11452 [details]
Manual test case 

I am having a hard time turning this into an automated test! If you click at the bottom of the textarea and hit delete a couple of times, you will see this crash.

Created attachment 11460 [details]
Automated test (will assert)

Comment on attachment 11460 [details]
Automated test (will assert)

This test is self-contained. If you put it under LayoutTests/editing/ you can rewrite it to use editing.js.

[Beth Dakin]

This patch causes some layout test regressions that may or may not be good changes. I do not know enough about editing to know for sure, but some of the changes definitely seem wrong. Also, in the textarea manual test example, where we used to assert, we now instead stop deleting entriely. Though it is hard to believe that this could be caused by Darin's patch, it appears to be since I do not see this problem in nightlies. 

I debugged the no-longer-deleting problem a bit found that TypingCommand::deleteKeyPressed creates a caret instead of a range whe we stop deleting because Selection::validate find that m_start.upstream() == m_end.upstream(). In the error case, upstream() goes to the very end and returns lstVisible. When it is working correvtly (without the patch), we hit this case:

// return current position if it is in rendered text
if (renderer->isText() && static_cast<RenderText *>(renderer)->firstTextBox())

and return Position(currentNode, renderer->caretMaxOffset()). We don't hit this WITH the patch because firstTextBox() is null. I tried the simple solution of calling update layout, and that does not fix the problem.

Oh! And! Making this bug extra fun, it does not reproduce if you try to set any breakpoints. So I did all of this debugging with printfs. Man, I just love editing.

I think someone with more editing know-how should take this one from here. Hey Justin, wanna give this bug some love??

[Justin Garcia]

One thing regarding Darin's patch, it doesn't seem right to turn the ASSERT into a if (blah) return.  We should be able to assert that, if a leading/trailing space is no longer a rendered character after the deletion operation, then it was in collapsible whitespace.

[Joost de Valk (AlthA)]

This bug seems to be without a father / mother atm, who want's to be it's new parent?

This bug is ultimately the result of deleting a character that serves as a line break position without updating the affected line. This first inconsistency quickly leads to a inline box tree disaster. I have a patch that fixes it. Running the layout tests now...

Created attachment 13119 [details]
Patch, including change log and test

The layout test fails even on release builds (deleting two lines instead of one).

[Maciej Stachowiak]

Comment on attachment 13119 [details]
Patch, including change log and test

r=me

This seems like a pretty non-obvious way to say "broke at a newline that has been deleted":

prevRootBox->lineBreakObj()->isText() && prevRootBox->lineBreakPos() >= static_cast<RenderText*>(prevRootBox->lineBreakObj())->textLength()

May I suggest making an inline function for this condition, something like "brokeAtDeletedNewline()"? This might make the comment less necessary (although it's still not obvious that "curr = prevRootBox" means "treat previous line as dirty".

[Maciej Stachowiak]

I landed this, without making any of my suggested style changes.