Bug 37950

Summary: Crash in WebCore::TextIterator::handleTextNode() encountered in Google rich-text products
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, enrica, jparent, mitz, playmobil, tony
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
test case (will crash)
none
Patch
none
Patch mitz: review+

Ojan Vafai
Reported 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);
Attachments
test case (will crash) (496 bytes, text/html)
2010-04-21 19:04 PDT, Alexey Proskuryakov
no flags
Patch (3.15 KB, patch)
2010-04-22 23:21 PDT, Tony Chang
no flags
Patch (3.12 KB, patch)
2010-04-22 23:42 PDT, Tony Chang
mitz: review+
Alexey Proskuryakov
Comment 1 2010-04-21 19:04:36 PDT
Created attachment 54013 [details] test case (will crash) Same test as an attachment.
Alexey Proskuryakov
Comment 2 2010-04-21 19:05:17 PDT
Alexey Proskuryakov
Comment 3 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])
Julie Parent
Comment 4 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).
Ojan Vafai
Comment 5 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.
Tony Chang
Comment 6 2010-04-22 23:21:01 PDT
Tony Chang
Comment 7 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.
mitz
Comment 8 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();
Tony Chang
Comment 9 2010-04-22 23:42:46 PDT
Tony Chang
Comment 10 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.
Tony Chang
Comment 11 2010-04-23 00:04:47 PDT
Note You need to log in before you can comment on or make changes to this bug.