Bug 37950 - Crash in WebCore::TextIterator::handleTextNode() encountered in Google rich-text products
Summary: Crash in WebCore::TextIterator::handleTextNode() encountered in Google rich-t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-04-21 14:08 PDT by Ojan Vafai
Modified: 2010-04-23 00:04 PDT (History)
6 users (show)

See Also:


Attachments
test case (will crash) (496 bytes, text/html)
2010-04-21 19:04 PDT, Alexey Proskuryakov
no flags Details
Patch (3.15 KB, patch)
2010-04-22 23:21 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2010-04-22 23:42 PDT, Tony Chang
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-04-21 14:08:29 PDT
Crashes in a WebKit nightly and in Chrome dev channel. Does not crash in Safari 4. I'm guessing this is a recent regression since the repro steps are so simple.

You can see the crash in gmail doing the following steps:
1. Click to compose a new message
2. Type multiple words in a RTL language, e.g. Hebrew: "אחת שתיים שלוש"
4. Select one of the words
5. Click the Link button


A JS snippet that reproduces the crash (thanks Julie!):

var div = document.createElement('div');
div.contentEditable = 'true';
div.innerHTML = "אחת ש<a id='bar'>תיים </a>שלוש";
document.body.appendChild(div);
div.focus();

var sel = window.getSelection();
sel.selectAllChildren(bar);
var range = sel.getRangeAt(0);
range.insertNode(document.createElement('span'));
range.detach();

// This part can be any number of actions, e.g., clicking anywhere on the page also crashes.
sel.selectAllChildren(bar);
Comment 1 Alexey Proskuryakov 2010-04-21 19:04:36 PDT
Created attachment 54013 [details]
test case (will crash)

Same test as an attachment.
Comment 2 Alexey Proskuryakov 2010-04-21 19:05:17 PDT
<rdar://problem/7892035>
Comment 3 Alexey Proskuryakov 2010-04-21 19:06:43 PDT
Asserts in debug build:

ASSERTION FAILED: i < size()
(/Users/ap/Safari/OpenSource/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/Vector.h:533 T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = WebCore::InlineTextBox*, long unsigned int inlineCapacity = 0ul])
Comment 4 Julie Parent 2010-04-22 10:39:58 PDT
I can repro this in webkit back as far as r31256 (don't have working builds from any further back).
Comment 5 Ojan Vafai 2010-04-22 10:43:40 PDT
Huh...I was wrong. This does happen in Safari 4. I guess I'm not sure if this is a regression. Can't get a WebKit nightly to run older than r31256.
Comment 6 Tony Chang 2010-04-22 23:21:01 PDT
Created attachment 54133 [details]
Patch
Comment 7 Tony Chang 2010-04-22 23:23:57 PDT
I don't know this code well, but it seems like a simple NULL pointer in the RTL case.  If the text was not RTL, renderer->firstTextBox() would be NULL and nothing would happen.  I changed the code before the crash to catch the case with no text boxes and exit early.
Comment 8 mitz 2010-04-22 23:35:54 PDT
Comment on attachment 54133 [details]
Patch

Thanks for tackling this bug!

> -    if (!renderer->firstTextBox() && str.length() > 0) {
> -        m_lastTextNodeEndedWithCollapsedSpace = true; // entire block is collapsed space
> +    if (!renderer->firstTextBox()) {
> +        if (str.length() > 0)
> +            m_lastTextNodeEndedWithCollapsedSpace = true; // entire block is collapsed space
>          return true;
>      }

I am afraid that this fix may be wrong, because it doesn’t reset m_textBox in this case. I would feel much better with a fix that didn’t return early here, but instead just changed this

m_textBox = renderer->containsReversedText() ? m_sortedTextBoxes[0] : renderer->firstTextBox();

to say

m_textBox = renderer->containsReversedText() ? (m_sortedTextBoxes.size() ? m_sortedTextBoxes[0] : 0) : renderer->firstTextBox();
Comment 9 Tony Chang 2010-04-22 23:42:46 PDT
Created attachment 54135 [details]
Patch
Comment 10 Tony Chang 2010-04-22 23:46:01 PDT
(In reply to comment #8)
> I am afraid that this fix may be wrong, because it doesn’t reset m_textBox in
> this case. I would feel much better with a fix that didn’t return early here,
> but instead just changed this
> 
> m_textBox = renderer->containsReversedText() ? m_sortedTextBoxes[0] :
> renderer->firstTextBox();
> 
> to say
> 
> m_textBox = renderer->containsReversedText() ? (m_sortedTextBoxes.size() ?
> m_sortedTextBoxes[0] : 0) : renderer->firstTextBox();

Good point.  Updated.
Comment 11 Tony Chang 2010-04-23 00:04:47 PDT
Committed r58153: <http://trac.webkit.org/changeset/58153>