Bug 37950 – Crash in WebCore::TextIterator::handleTextNode() encountered in Google rich-text products

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ojan Vafai]

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);

[Alexey Proskuryakov]

Created attachment 54013 [details]
test case (will crash)

Same test as an attachment.

[Alexey Proskuryakov]

<rdar://problem/7892035>

[Alexey Proskuryakov]

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

[Julie Parent]

I can repro this in webkit back as far as r31256 (don't have working builds from any further back).

[Ojan Vafai]

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.

[Tony Chang]

Created attachment 54133 [details]
Patch

[Tony Chang]

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 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();

[Tony Chang]

Created attachment 54135 [details]
Patch

[Tony Chang]

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

[Tony Chang]

Committed r58153: <http://trac.webkit.org/changeset/58153>