22168 2008-11-10 17:26:06 -0800 Chromium is seeing crashes using TextIterator 2008-12-02 11:07:43 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P2 Normal --- 1 eric webkit-unassigned justin.garcia mitz oldest_to_newest 98240 0 eric 2008-11-10 17:26:06 -0800 http://code.google.com/p/chromium/issues/detail?id=4122 I'm not sure why the bug is hidden, and I don't understand chromium's bug system enough to change it. So I'm just filing this here. We're seeing a crash when using TextIterator, after an advance() call, sometimes the resulting characters() is 0x02, the only way I can see that happening is if emitText is called with a node with a null string. The only way I see emitText ever being called where the string wasn't null checked is here: // Handle either a single newline character (which becomes a space), // or a run of characters that does not include a newline. // This effectively translates newlines to spaces without copying the text. if (str[runStart] == '\n') { emitCharacter(' ', m_node, 0, runStart, runStart + 1); m_offset = runStart + 1; } else { int subrunEnd = str.find('\n', runStart); if (subrunEnd == -1 || subrunEnd > runEnd) subrunEnd = runEnd; m_offset = subrunEnd; emitText(m_node, runStart, subrunEnd); } I'm not fully confident in my mental debugging, so I'd like to try and catch this in the wild with more stack information. Hence, I'm suggesting adding this ASSERT. There are various ways to fix this, including making it more explicit what strings we're checking. In the one call site which might be wrong, we check "str" length before calling emitText, but m_node does not necessarily still have an renderer() which returns that string, since m_node can be changed independently of "str". I'll post a patch with the ASSERT shortly. 98241 1 25036 eric 2008-11-10 17:31:40 -0800 Created attachment 25036 Add ASSERT to catch crash after advance() WebCore/ChangeLog | 11 +++++++++++ WebCore/editing/TextIterator.cpp | 3 ++- 2 files changed, 13 insertions(+), 1 deletions(-) 98246 2 mal 2008-11-10 18:23:37 -0800 Chromium bug 4122 contained some private data, so it was closed. The real bug is http://crbug.com/4123. We're seeing this crash in automated test runs, but not in user crash reports. It's possibly a side effect of the test system. 98260 3 eric 2008-11-10 21:28:44 -0800 Thank you mark. I still think WebKit should add this ASSERT, and eventually change our approach here. (i.e. change the arguments to this function to be less fragile) 98293 4 25036 darin 2008-11-11 09:09:39 -0800 Comment on attachment 25036 Add ASSERT to catch crash after advance() I see no harm in adding this assertion, but little benefit in doing so. The line that initializes m_lastCharacter will crash if str.characters() is 0; this will move that crash up a few lines but not detect any additional failure cases. The evidence does not match the theory that renderer->text() is a null string. In TextIterator::handleTextBox we've already fetched renderer->text() and dereferenced it by calling str[runStart] before calling emitText. So there's no real chance that it's a null string in that code path. If characters() is 0x02 that does not indicate a null string. Instead it indicates a StringImpl that has been deallocated or overwritten. characters() is 0 in a null string. r=me because there's no harm in adding the assertion. 101027 5 eric 2008-12-02 11:07:43 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/editing/TextIterator.cpp Committed r38908 25036 2008-11-10 17:31:40 -0800 2008-11-11 09:09:39 -0800 Add ASSERT to catch crash after advance() Add-ASSERT-to-catch-crash-after-advance-.patch text/plain 1365 eric MTA3MDAyZGZlZWVkOWFhNzA1NGJiZGYwYmM4Njk1Mjg3ZTAxYTA4NApkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA5OTllYzM2Li4yMDJmYzFi IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE0IEBACisyMDA4LTExLTEwICBFcmljIFNlaWRlbCAgPGVyaWNAd2Via2l0Lm9y Zz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGQg YW4gQVNTRVJUIHRvIHRyeSBhbmQgY2F0Y2ggdGhlIHJvb3QgY2F1c2Ugb2Y6CisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjE2OAorICAgICAgICBodHRw Oi8vY29kZS5nb29nbGUuY29tL3AvY2hyb21pdW0vaXNzdWVzL2RldGFpbD9pZD00MTIyCisKKyAg ICAgICAgKiBlZGl0aW5nL1RleHRJdGVyYXRvci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpUZXh0 SXRlcmF0b3I6OmVtaXRUZXh0KToKKwogMjAwOC0xMS0xMCAgQWRhbSBSb2JlbiAgPGFyb2JlbkBh cHBsZS5jb20+CiAKICAgICAgICAgRml4IEJ1ZyAyMjE2MTogQXNzZXJ0aW9uIGZhaWx1cmUgaW4g UmVuZGVyVGhlbWVXaW46OnN5c3RlbUNvbG9yIHdoZW4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvZWRp dGluZy9UZXh0SXRlcmF0b3IuY3BwIGIvV2ViQ29yZS9lZGl0aW5nL1RleHRJdGVyYXRvci5jcHAK aW5kZXggMDkzZTg1MS4uNjVhYjM1OCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9lZGl0aW5nL1RleHRJ dGVyYXRvci5jcHAKKysrIGIvV2ViQ29yZS9lZGl0aW5nL1RleHRJdGVyYXRvci5jcHAKQEAgLTY3 NiwxMCArNjc2LDExIEBAIHZvaWQgVGV4dEl0ZXJhdG9yOjplbWl0Q2hhcmFjdGVyKFVDaGFyIGMs IE5vZGUgKnRleHROb2RlLCBOb2RlICpvZmZzZXRCYXNlTm9kZSwKICAgICBtX2xhc3RDaGFyYWN0 ZXIgPSBjOwogfQogCi12b2lkIFRleHRJdGVyYXRvcjo6ZW1pdFRleHQoTm9kZSAqdGV4dE5vZGUs IGludCB0ZXh0U3RhcnRPZmZzZXQsIGludCB0ZXh0RW5kT2Zmc2V0KQordm9pZCBUZXh0SXRlcmF0 b3I6OmVtaXRUZXh0KE5vZGUqIHRleHROb2RlLCBpbnQgdGV4dFN0YXJ0T2Zmc2V0LCBpbnQgdGV4 dEVuZE9mZnNldCkKIHsKICAgICBSZW5kZXJUZXh0KiByZW5kZXJlciA9IHN0YXRpY19jYXN0PFJl bmRlclRleHQqPihtX25vZGUtPnJlbmRlcmVyKCkpOwogICAgIFN0cmluZyBzdHIgPSByZW5kZXJl ci0+dGV4dCgpOworICAgIEFTU0VSVChzdHIuY2hhcmFjdGVycygpKTsKIAogICAgIG1fcG9zaXRp b25Ob2RlID0gdGV4dE5vZGU7CiAgICAgbV9wb3NpdGlvbk9mZnNldEJhc2VOb2RlID0gMDsK