Bug 10144 - REGRESSION: Reproducible assertion failure in DeleteSelectionCommand::fixupWhitespace()
Summary: REGRESSION: Reproducible assertion failure in DeleteSelectionCommand::fixupWh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Justin Garcia
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-07-28 06:48 PDT by David Kilzer (:ddkilzer)
Modified: 2007-02-11 20:17 PST (History)
6 users (show)

See Also:


Attachments
Crash log from assertion failure (21.00 KB, text/plain)
2006-07-28 06:49 PDT, David Kilzer (:ddkilzer)
no flags Details
patch, but no change log or test cases or test result updates yet (7.18 KB, patch)
2006-07-29 15:40 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Darin's patch, merges with today's TOT (5.24 KB, patch)
2006-11-09 22:26 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Manual test case (64 bytes, text/html)
2006-11-09 22:28 PST, Beth Dakin
no flags Details
Automated test (will assert) (285 bytes, text/html)
2006-11-10 06:04 PST, mitz
no flags Details
Patch, including change log and test (30.36 KB, patch)
2007-02-11 09:18 PST, mitz
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2006-07-28 06:48:12 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2006-07-28 06:49:30 PDT
Created attachment 9743 [details]
Crash log from assertion failure
Comment 2 Darin Adler 2006-07-29 10:00:14 PDT
I'm the one to blame for this assertion. Added just recently.
Comment 3 Darin Adler 2006-07-29 15:39:32 PDT
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.
Comment 4 Darin Adler 2006-07-29 15:40:26 PDT
Created attachment 9758 [details]
patch, but no change log or test cases or test result updates yet
Comment 5 Stephanie Lewis 2006-11-06 19:38:04 PST
radar 4823044
Comment 6 Beth Dakin 2006-11-09 22:26:20 PST
Created attachment 11451 [details]
Darin's patch, merges with today's TOT

This is just Darin's patch merged with today's TOT.
Comment 7 Beth Dakin 2006-11-09 22:28:23 PST
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.
Comment 8 mitz 2006-11-10 06:04:57 PST
Created attachment 11460 [details]
Automated test (will assert)
Comment 9 mitz 2006-11-10 06:09:11 PST
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.
Comment 10 Beth Dakin 2006-11-10 14:29:56 PST
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??
Comment 11 Justin Garcia 2006-11-10 15:20:15 PST
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.
Comment 12 Joost de Valk (AlthA) 2007-01-19 05:07:40 PST
This bug seems to be without a father / mother atm, who want's to be it's new parent?
Comment 13 mitz 2007-02-11 08:37:45 PST
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...
Comment 14 mitz 2007-02-11 09:18:21 PST
Created attachment 13119 [details]
Patch, including change log and test

The layout test fails even on release builds (deleting two lines instead of one).
Comment 15 Maciej Stachowiak 2007-02-11 17:55:57 PST
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".
Comment 16 Maciej Stachowiak 2007-02-11 20:17:01 PST
I landed this, without making any of my suggested style changes.