Bug 61012

Summary: REGRESSION (r83322): Many crashes in Mail.app in WebCore::Node::nodeIndex
Product: WebKit Reporter: Adele Peterson <adele>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, darin, dglazkov, enrica, rniwa, tony, webkit.review.bot
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61102    
Bug Blocks:    
Attachments:
Description Flags
testcase
none
work in progress
none
fixes the crash
darin: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 none

Adele Peterson
Reported 2011-05-17 18:29:51 PDT
Created attachment 93856 [details] testcase > 1 com.apple.WebCore 0x7fff8918f738 WebCore::Node::nodeIndex() const + 0x4 2 com.apple.WebCore 0x7fff8951b939 WebCore::positionInParentBeforeNode(WebCore::Node const*) + 0x19 3 com.apple.WebCore 0x7fff8939fa97 WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent() + 0x21 4 com.apple.WebCore 0x7fff8939946a WebCore::ReplaceSelectionCommand::doApply() + 0x331c 5 com.apple.WebCore 0x7fff8932695a WebCore::EditCommand::apply() + 0x94 I verified this was caused by the fix for the recent SplitElement crasher - http://trac.webkit.org/changeset/81887 Attaching a reproducible case that can be run in a browser. The markup includes a Mail-style blockquote, so my guess is we need some kind of special case for those. <rdar://problem/9236427>
Attachments
testcase (112 bytes, text/html)
2011-05-17 18:29 PDT, Adele Peterson 		no flags
Details
work in progress (10.60 KB, patch)
2011-05-18 18:07 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the crash (8.93 KB, patch)
2011-05-18 23:43 PDT, Ryosuke Niwa 		darin: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-02 (1.41 MB, application/zip)
2011-05-19 00:19 PDT, WebKit Review Bot 		no flags
Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adele Peterson
Comment 1 2011-05-17 18:30:38 PDT
I also had to disable some debug assertions in the text checking code to get the same backtrace.
Ryosuke Niwa
Comment 2 2011-05-17 18:33:28 PDT
Also see https://bugs.webkit.org/show_bug.cgi?id=60914.
Adele Peterson
Comment 3 2011-05-17 18:37:48 PDT
What's the relationship? (In reply to comment #2) > Also see https://bugs.webkit.org/show_bug.cgi?id=60914.
Ryosuke Niwa
Comment 4 2011-05-17 18:58:39 PDT
(In reply to comment #3) > What's the relationship? > > (In reply to comment #2) > > Also see https://bugs.webkit.org/show_bug.cgi?id=60914. I'm reverting these two changesets in that bug.
Adele Peterson
Comment 5 2011-05-18 11:15:45 PDT
I pasted the wrong link above. This was caused by the follow up fix in http://trac.webkit.org/changeset/83322 I'm hoping we can find a way to fix the crash without backing out the correctness change in r81887.
Adele Peterson
Comment 6 2011-05-18 11:20:11 PDT
Agh, I keep getting confused between all the revisions here. I didn't describe these changes correctly, but I maintain that I'd like to fix the crash before addressing the behavior changes.
Ryosuke Niwa
Comment 7 2011-05-18 12:41:49 PDT
Let me look into this later.
Ryosuke Niwa
Comment 8 2011-05-18 18:07:18 PDT
Created attachment 94018 [details] work in progress Here's my work in progress patch. I'd have to convert editing/pasteboard/paste-blockquote-into-blockquote-3.html to a dump-as-markup test first to verify the correctness.
Ryosuke Niwa
Comment 9 2011-05-18 23:43:08 PDT
Created attachment 94044 [details] fixes the crash
Ryosuke Niwa
Comment 10 2011-05-18 23:46:59 PDT
Unfortunately, I have to admit the defeat and only fix the crash. There's really tricky conflicts of conditions in merging the start of paragraph, and this is what I can do best for now. Also, I had to revert some of improvements done in http://trac.webkit.org/changeset/81887 (and subsequently in r83322) but bloated markup is better than crashing after all. My current plan is to land this patch and re-visit the bloating issue in https://bugs.webkit.org/show_bug.cgi?id=60988 and https://bugs.webkit.org/show_bug.cgi?id=34564.
WebKit Review Bot
Comment 11 2011-05-19 00:19:07 PDT
Comment on attachment 94044 [details] fixes the crash Attachment 94044 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8710842 New failing tests: editing/pasteboard/paste-text-011.html
WebKit Review Bot
Comment 12 2011-05-19 00:19:13 PDT
Created attachment 94049 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Darin Adler
Comment 13 2011-05-19 10:04:44 PDT
(In reply to comment #10) > Also, I had to revert some of improvements done in http://trac.webkit.org/changeset/81887 (and subsequently in r83322) but bloated markup is better than crashing after all. Disappointing.
Ryosuke Niwa
Comment 14 2011-05-19 10:08:14 PDT
(In reply to comment #13) > (In reply to comment #10) > > Also, I had to revert some of improvements done in http://trac.webkit.org/changeset/81887 (and subsequently in r83322) but bloated markup is better than crashing after all. > > Disappointing. Yeah, I'm sad too :( But we can make a huge improvement once we fix the bug 34564. Unfortunately, it's blocked by the bug 60988 and the patch for the bug 60988 is likely to mid-air collide with this one but I'm hopeful that we can make a progress.
Ryosuke Niwa
Comment 15 2011-05-19 10:22:29 PDT
Committed r86852: <http://trac.webkit.org/changeset/86852>
Ademar Reis
Comment 16 2011-05-19 13:40:28 PDT
Revision r86852 cherry-picked into qtwebkit-2.2 with commit 66e7630 <http://gitorious.org/webkit/qtwebkit/commit/66e7630>
Note You need to log in before you can comment on or make changes to this bug.