Summary: | Crash in WebCore::TextIterator::handleTextNode() encountered in Google rich-text products | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||||||
Component: | HTML Editing | Assignee: | 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
Ojan Vafai
2010-04-21 14:08:29 PDT
Created attachment 54013 [details]
test case (will crash)
Same test as an attachment.
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]) I can repro this in webkit back as far as r31256 (don't have working builds from any further back). 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. Created attachment 54133 [details]
Patch
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(); Created attachment 54135 [details]
Patch
(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. Committed r58153: <http://trac.webkit.org/changeset/58153> |