REGRESSION (r83322): Many crashes in Mail.app in WebCore::Node::nodeIndex

• Component: HTML Editing
• Status: RESOLVED FIXED

[Adele Peterson]

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>

[Adele Peterson]

I also had to disable some debug assertions in the text checking code to get the same backtrace.

[Ryosuke Niwa]

Also see https://bugs.webkit.org/show_bug.cgi?id=60914.

[Adele Peterson]

What's the relationship?

(In reply to comment #2)
> Also see https://bugs.webkit.org/show_bug.cgi?id=60914.

[Ryosuke Niwa]

(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]

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]

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]

Let me look into this later.

[Ryosuke Niwa]

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]

Created attachment 94044 [details]
fixes the crash

[Ryosuke Niwa]

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 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]

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]

(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]

(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]

Committed r86852: <http://trac.webkit.org/changeset/86852>

[Ademar Reis]

Revision r86852 cherry-picked into qtwebkit-2.2 with commit 66e7630 <http://gitorious.org/webkit/qtwebkit/commit/66e7630>