Bug 22168 – Chromium is seeing crashes using TextIterator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 25036 [details]
Add ASSERT to catch crash after advance()

 WebCore/ChangeLog                |   11 +++++++++++
 WebCore/editing/TextIterator.cpp |    3 ++-
 2 files changed, 13 insertions(+), 1 deletions(-)

[Mark Larson (Google)]

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.

[Eric Seidel (no email)]

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)

[Darin Adler]

Comment on attachment 25036 [details]
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.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/editing/TextIterator.cpp
Committed r38908