Bug 10144

Summary: REGRESSION: Reproducible assertion failure in DeleteSelectionCommand::fixupWhitespace()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: HTML EditingAssignee: Justin Garcia <justin.garcia>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, darin, dev+webkit, joost, justin.garcia, mitz
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Crash log from assertion failure
none
patch, but no change log or test cases or test result updates yet
none
Darin's patch, merges with today's TOT
none
Manual test case
none
Automated test (will assert)
none
Patch, including change log and test mjs: review+

David Kilzer (:ddkilzer)
Reported 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.
Attachments
Crash log from assertion failure (21.00 KB, text/plain)
2006-07-28 06:49 PDT, David Kilzer (:ddkilzer)
no flags
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
Darin's patch, merges with today's TOT (5.24 KB, patch)
2006-11-09 22:26 PST, Beth Dakin
no flags
Manual test case (64 bytes, text/html)
2006-11-09 22:28 PST, Beth Dakin
no flags
Automated test (will assert) (285 bytes, text/html)
2006-11-10 06:04 PST, mitz
no flags
Patch, including change log and test (30.36 KB, patch)
2007-02-11 09:18 PST, mitz
mjs: review+
David Kilzer (:ddkilzer)
Comment 1 2006-07-28 06:49:30 PDT
Created attachment 9743 [details] Crash log from assertion failure
Darin Adler
Comment 2 2006-07-29 10:00:14 PDT
I'm the one to blame for this assertion. Added just recently.
Darin Adler
Comment 3 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.
Darin Adler
Comment 4 2006-07-29 15:40:26 PDT
Created attachment 9758 [details] patch, but no change log or test cases or test result updates yet
Stephanie Lewis
Comment 5 2006-11-06 19:38:04 PST
radar 4823044
Beth Dakin
Comment 6 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.
Beth Dakin
Comment 7 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.
mitz
Comment 8 2006-11-10 06:04:57 PST
Created attachment 11460 [details] Automated test (will assert)
mitz
Comment 9 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.
Beth Dakin
Comment 10 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??
Justin Garcia
Comment 11 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.
Joost de Valk (AlthA)
Comment 12 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?
mitz
Comment 13 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...
mitz
Comment 14 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).
Maciej Stachowiak
Comment 15 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".
Maciej Stachowiak
Comment 16 2007-02-11 20:17:01 PST
I landed this, without making any of my suggested style changes.
Note You need to log in before you can comment on or make changes to this bug.