Bug 25312 – REGRESSION: Infinite loop in WebCore::Position::upstream while selecting a block of text

• Component: New Bugs
• Status: RESOLVED FIXED

[Finnur Thorarinsson]

Please disregard the version above. I filed this against Chromium, which is where this reproduces - but eseidel asked that I file this here so we can CC members of WebKit on this bug. This was originally filed as http://code.google.com/p/chromium/issues/detail?id=10805.

Repro:

In the dev build of Google Chrome, I selected a block of text on Facebook and noticed that the renderer became unresponsive, spinning the CPU at 100%. I reproduced this over and over with no problem (across restart of Chrome and across an update to the latest dev build, as it was reproducing on the previous one as well). 

I took a process snapshot once (see attachment on Chromium bug) and was able to step through with WinDbg. I noticed that once I try to Step Out of WebCore::Position::upstream it starts spinning the CPU. 

ChildEBP RetAddr  
00caf844 01f1ab81 chrome_1c30000!WebCore::canHaveChildrenForEditing+0xc2
00caf84c 01f1ab6e chrome_1c30000!WebCore::editingIgnoresContent+0x8
00caf850 01ebf97d chrome_1c30000!WebCore::isAtomicNode+0x16
00caf858 01ebfa34 chrome_1c30000!WebCore::isStreamer+0x10
00caf8a4 01ea01d9 chrome_1c30000!WebCore::Position::upstream+0xaa
00caf8e0 01e9f23b chrome_1c30000!WebCore::VisiblePosition::canonicalPosition+0x35
00caf910 01e9f200 chrome_1c30000!WebCore::VisiblePosition::init+0x26
00caf928 01ea93a0 chrome_1c30000!WebCore::VisiblePosition::VisiblePosition+0x2e
00caf944 01f3054c chrome_1c30000!WebCore::RenderObject::createVisiblePosition+0xa1
00caf968 01ea8933 chrome_1c30000!WebCore::RenderText::positionForPoint+0x173
00caf980 01f29f25 chrome_1c30000!WebCore::RenderObject::positionForCoordinates+0x1d
00caf99c 01f29fef chrome_1c30000!WebCore::positionForPointWithInlineChildren+0x7f
00caf9cc 01ea8933 chrome_1c30000!WebCore::RenderBlock::positionForPoint+0xb2
00caf9e4 01f29e9f chrome_1c30000!WebCore::RenderObject::positionForCoordinates+0x1d
00cafa0c 01f2a041 chrome_1c30000!WebCore::positionForPointRespectingEditingBoundaries+0xaf
00cafa34 01ea8933 chrome_1c30000!WebCore::RenderBlock::positionForPoint+0x104
00cafa4c 01f29e9f chrome_1c30000!WebCore::RenderObject::positionForCoordinates+0x1d
00cafa74 01f2a041 chrome_1c30000!WebCore::positionForPointRespectingEditingBoundaries+0xaf
00cafa98 01eb9bf3 chrome_1c30000!WebCore::RenderBlock::positionForPoint+0x104
00cafb20 01eb9b23 chrome_1c30000!WebCore::EventHandler::updateSelectionForMouseDrag+0x49

Basically, it is looping forever in Position::upstream because it seems to never go into any of the if 
statements within the for loop. It just loops forever.

I stepped into PositionIterator::decrement(). m_parent points to a node, but m_child is 
NULL. As a result, decrement() just calls Position::uncheckedPreviousOffset(), which 
effectively decrements m_offset... 

However, m_offset is this ridiculously large number (2061442196 and shrinking)... Does 
that make any sense?

NOTE: I am not able to repro this on my development machine and not able to repro this in Safari 4 on my laptop either. There is something about my Thinkpad laptop that triggers this, it seems.

[Eric Seidel (no email)]

I think it's more likely that somehow the copy of chromium on your thinkpad has a different copy of WebKit.  It's possible that this regressed briefly and has been since fixed?

I've CC'd the other two who've worked in this function recently.

[Justin Garcia]

I think ddkilzer ran into this, let me look...

[David Kilzer (:ddkilzer)]

<rdar://problem/6805717>

[Justin Garcia]

I don't know that Position checks it's offset to see if it's valid.  Perhaps this could be the problem (from RenderObject::createVisiblePosition):

        // Find non-anonymous content before.
        renderer = child;
        while ((renderer = renderer->previousInPreOrder())) {
            if (renderer == parent)
                break;
            if (Node* node = renderer->node())
                return VisiblePosition(node, numeric_limits<int>::max(), DOWNSTREAM);
        }

[Darin Adler]

(In reply to comment #4)
> I don't know that Position checks it's offset to see if it's valid.  Perhaps
> this could be the problem (from RenderObject::createVisiblePosition):
> 
>         // Find non-anonymous content before.
>         renderer = child;
>         while ((renderer = renderer->previousInPreOrder())) {
>             if (renderer == parent)
>                 break;
>             if (Node* node = renderer->node())
>                 return VisiblePosition(node, numeric_limits<int>::max(),
> DOWNSTREAM);
>         }
> 

Position doesn't check the offset, but I think VisiblePosition should.

If it doesn’t, then my bad; I wrote the code above and maybe that did cause the bug!

[Darin Adler]

<rdar://problem/6788905>

[Darin Adler]

I probably broke this in http://trac.webkit.org/changeset/41928

[Darin Adler]

The fix is probably to use lastEditingOffsetForNode instead of numeric_limits<int>::max() in createVisiblePosition. I’d just jump in and do the fix myself, but we also need a regression tests.

[Justin Garcia]

> Position doesn't check the offset

Ya, and I don't think we should here because I believe we allow invalid positions for ending selections that won't be valid until Undo.

> but I think VisiblePosition should.

maybe.  although validation here would require an extra call to nodeIndex/childnodecount in some situations.  probably just fix it at the call site.

[Darin Adler]

Actually VisiblePosition *is* checking the offset; just very slowly. That’s where we’re hanging!

[Justin Garcia]

> Actually VisiblePosition *is* checking the offset; just very slowly. That’s
> where we’re hanging!

by 'check' i just meant verify that the offset is no greater than lastEditingOffsetForNode...

[Adele Peterson]

Created attachment 29717 [details]
patch

I haven't been able to reproduce this, but I think there's value on getting this change in anyway to confirm it fixes the problem.

[Adele Peterson]

Committed revision 42794.

Please let me know if anyone can still reproduce this after this revision.  Thanks!