Bug 89526 – Crash at WebCore::TextIterator::handleTextBox

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alice Cheng]

This crash seems to be happening mostly in Mail, in relation with autocorrection.
Here is the call stack,


>  1 com.apple.WebCore              0x7fff8ec1fa64 WebCore::TextIterator::handleTextBox() + 0x24
   2 com.apple.WebCore              0x7fff8ec1e81b WebCore::TextIterator::advance() + 0x10b
   3 com.apple.WebCore              0x7fff8ec7aca6 WebCore::CharacterIterator::advance(int) + 0x46
   4 com.apple.WebCore              0x7fff8ee9ff01 WebCore::characterSubrange(WebCore::CharacterIterator&, int, int) + 0x21
   5 com.apple.WebCore              0x7fff8ee9fec0 WebCore::TextIterator::subrange(WebCore::Range*, int, int) + 0x80
   6 com.apple.WebCore              0x7fff8ee9fe06 WebCore::TextCheckingParagraph::subrange(int, int) const + 0x36
   7 com.apple.WebCore              0x7fff8ec8be9c WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges(unsigned int, WebCore::Range*, WebCore::Range*) + 0xd8c
   8 com.apple.WebCore              0x7fff8ee7a014 WebCore::Editor::markMisspellingsAfterTypingToWord(WebCore::VisiblePosition const&, WebCore::VisibleSelection const&, bool) + 0x344
   9 com.apple.WebCore              0x7fff8ee797fc WebCore::TypingCommand::markMisspellingsAfterTyping(WebCore::TypingCommand::ETypingCommand) + 0x27c
  10 com.apple.WebCore              0x7fff8ee80d0b WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 0xfb
  11 com.apple.WebCore              0x7fff8ee80ba0 WebCore::TypingCommand::insertText(WTF::String const&, bool) + 0xc0
  12 com.apple.WebCore              0x7fff8ee80886 WebCore::TypingCommand::insertText(WebCore::Document*, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 0x296
  13 com.apple.WebCore              0x7fff8eef8e90 WebCore::Editor::insertTextWithoutSendingTextEvent(WTF::String const&, bool, WebCore::TextEvent*) + 0x240
  14 com.apple.WebCore              0x7fff8eede03e WebCore::Editor::handleTextEvent(WebCore::TextEvent*) + 0x14e
  15 com.apple.WebCore              0x7fff8eeddedb WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent*) + 0x1b
  16 com.apple.WebCore              0x7fff8eb82e35 WebCore::Node::defaultEventHandler(WebCore::Event*) + 0x315
  17 com.apple.WebCore              0x7fff8eb82252 WebCore::EventDispatcher::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 0x412
  18 com.apple.WebCore              0x7fff8eb81e05 WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const + 0x25
  19 com.apple.WebCore              0x7fff8f30d5a6 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>) + 0x96
  20 com.apple.WebCore              0x7fff8eb81d57 WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 0x37
  21 com.apple.WebCore              0x7fff8ed0c036 WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, int&) + 0x76
  22 com.apple.WebCore              0x7fff8eef5b45 WebCore::EventHandler::handleTextInputEvent(WTF::String const&, WebCore::Event*, WebCore::TextEventInputType) + 0x125
  23 com.apple.WebCore              0x7fff8ef179f5 WebCore::Editor::insertText(WTF::String const&, WebCore::Event*) + 0x15
  24 com.apple.WebKit               0x7fff886e8bcf -[WebHTMLView(WebNSTextInputSupport) insertText:] + 0x20f
  25 com.apple.WebKit               0x7fff886ddee8 -[WebHTMLView(WebInternal) _executeSavedKeypressCommands] + 0xf8
  26 com.apple.WebKit               0x7fff886dcf6d -[WebHTMLView(WebInternal) _interpretKeyEvent:savingCommands:] + 0x22d
  27 com.apple.WebKit               0x7fff886dd627 WebEditorClient::handleKeyboardEvent(WebCore::KeyboardEvent*) + 0x57
  28 com.apple.WebCore              0x7fff8ee9f4f6 WebCore::EventHandler::defaultKeyboardEventHandler(WebCore::KeyboardEvent*) + 0x486
  29 com.apple.WebCore              0x7fff8eb82c34 WebCore::Node::defaultEventHandler(WebCore::Event*) + 0x114
  30 com.apple.WebCore              0x7fff8eb82252 WebCore::EventDispatcher::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 0x412
  31 com.apple.WebCore              0x7fff8eb81e05 WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const + 0x25
  32 com.apple.WebCore              0x7fff8f30d5a6 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>) + 0x96
  33 com.apple.WebCore              0x7fff8eb81d57 WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 0x37
  34 com.apple.WebCore              0x7fff8ed0c036 WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, int&) + 0x76
  35 com.apple.WebCore              0x7fff8ee9e542 WebCore::EventHandler::keyEvent(WebCore::PlatformKeyboardEvent const&) + 0x782
  36 com.apple.WebCore              0x7fff8ee9cad5 WebCore::EventHandler::keyEvent(NSEvent*) + 0x35
  37 com.apple.WebKit               0x7fff886dcbbc -[WebHTMLView keyDown:] + 0x10c
  38 com.apple.mail                 0x105d1cbcc -[MessageWebHTMLView keyDown:] + 0x0 (/SourceCache/Mail/Mail-1405/MessageViewer/MessageWebHTMLView.m:572)
  39 com.apple.AppKit               0x7fff93145c17 -[NSWindow sendEvent:] + 0x258c
  40 com.apple.mail                 0x105bd2101 -[TypeAheadWindow sendEvent:] + 0x0 (/SourceCache/Mail/Mail-1405/Compose/TypeAheadWindow.m:147)
  41 com.apple.AppKit               0x7fff930e049b -[NSApplication sendEvent:] + 0x1603
  42 com.apple.mail                 0x105b8e95a -[MailApp sendEvent:] + 0x0 (/SourceCache/Mail/Mail-1405/Application/MailApp.m:419)
  43 com.apple.AppKit               0x7fff9307fcb4 -[NSApplication run] + 0x280
  44 com.apple.AppKit               0x7fff932e5491 NSApplicationMain + 0x365
  45 com.apple.mail                 0x105be3a6c start + 0x0

[Alice Cheng]

<rdar://problem/10305315>

[Alice Cheng]

Created attachment 148466 [details]
Patch

Posting for preliminary review. Still working on the appropriate test for the patch. Thanks

[Alice Cheng]

Created attachment 149048 [details]
Patch with a test

Patch with a test

[WebKit Review Bot]

Attachment 149048 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 149048 [details]
Patch with a test

View in context: https://bugs.webkit.org/attachment.cgi?id=149048&action=review

Does the test cover both fixes? I’m usually concerned when there is a single test case but two bug fixes.

I’m going to say r=me, but please do write some “why” comments as I request below.

> Source/WebCore/editing/AlternativeTextController.cpp:279
> +    int paragraphStartIndex = TextIterator::rangeLength(Range::create(m_frame->document(), m_frame->document(), 0, paragraphRangeContainingCorrection.get()->startContainer(), paragraphRangeContainingCorrection.get()->startOffset()).get());
>      applyCommand(SpellingCorrectionCommand::create(rangeWithAlternative, alternative));
> +    paragraphRangeContainingCorrection = TextIterator::rangeFromLocationAndLength(m_frame->document(), paragraphStartIndex, correctionStartOffsetInParagraph + alternative.length());

This needs a “why” comment. It’s important to recompute paragraphRangeContainingCorrection after applying the command, but we need to state why.

> Source/WebCore/editing/Editor.cpp:2125
> +                int paragraphStartIndex = TextIterator::rangeLength(Range::create(m_frame->document(), m_frame->document(), 0, paragraph.paragraphRange()->startContainer(), paragraph.paragraphRange()->startOffset()).get());
> +                int paragraphLength = TextIterator::rangeLength(paragraph.paragraphRange().get());
>                  applyCommand(SpellingCorrectionCommand::create(rangeToReplace, result->replacement));
> -
> +                RefPtr<Range> newParagraphRange = TextIterator::rangeFromLocationAndLength(m_frame->document(), paragraphStartIndex, paragraphLength+replacementLength-resultLength);

This needs a “why” comment. It’s important to recompute paragraph after applying the command, but we need to state why.

> LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:13
> +    //do typo

WebKit coding style says we should use a sentence format here:

    // Insert some text with a typographical error in it, so autocorrection occurs.

> LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:19
> +    //test marking

This comment should be omitted. It doesn’t add anything. The code below clearly checks if hasAutocorrectedMarker is true, so we should only include a comment if we have something else to say other than what the code itself says.

> LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:22
> +    if(window.internals){
> +        shouldBeTrue('internals.hasAutocorrectedMarker(document, 0, 1)');
> +    }

WebKit coding style means there should be a space after the “if” and no braces around a single line if statement body.

[Alice Cheng]

Created attachment 149546 [details]
Patch revised

[WebKit Review Bot]

Attachment 149546 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/editing/Editor.cpp:2125:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 149546 [details]
Patch revised

View in context: https://bugs.webkit.org/attachment.cgi?id=149546&action=review

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/editing/AlternativeTextController.cpp:279
> +    // Recalculate pragraphRangeContainingCorrection, since SpellingCorrectionCommand modified the DOM, such that the original paragraphRangeContainingCorrection is no longer valid, especially in blockquotes.

Also I'm not certain "especially in blockquote" adds any valuable information here. If anything, it's probably better refer to the bug like "See bug 89526" but I would just omit that if I were you.

>> Source/WebCore/editing/Editor.cpp:2125
>> +                //Recalculate newParagraphRange, since SpellingCorrectionCommand modifies the DOM, such that the original paragraph range is no longer valid, especially in blockquotes.
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Ditto, and please fix this style.

> LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:20
> +    if(window.internals)
> +        shouldBeTrue('internals.hasAutocorrectedMarker(document, 0, 1)');

Doesn't auto correction occurs asynchronously? Most importantly, did you verify that the crash reproduces when this test is ran inside DRT?

[Ryosuke Niwa]

Oh, now I realize that you were just addressing Darin's review comments. In the future, you could just make changes to address review comments and ask for commit-queue (if you're using webkit-patch, pass --no-review) but I guess it was helpful in this case because there are some nits that could be fixed.

[Adele Peterson]

I led Alice astray :) Alice, you should still address Ryosuke's comments and re-post a patch.

(In reply to comment #9)
> Oh, now I realize that you were just addressing Darin's review comments. In the future, you could just make changes to address review comments and ask for commit-queue (if you're using webkit-patch, pass --no-review) but I guess it was helpful in this case because there are some nits that could be fixed.

[Alice Cheng]

Created attachment 149597 [details]
Patch revised 2

Addressed last two reviews, and posting for commit-queue

[Alice Cheng]

Created attachment 149607 [details]
Patch revised 2

addressed previous reviews, and posting for commit-queue

[WebKit Review Bot]

Comment on attachment 149607 [details]
Patch revised 2

Clearing flags on attachment: 149607

Committed r121299: <http://trac.webkit.org/changeset/121299>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.