25312 2009-04-21 13:41:47 -0700 REGRESSION: Infinite loop in WebCore::Position::upstream while selecting a block of text 2009-04-23 15:47:31 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) PC Windows XP RESOLVED FIXED InRadar, NeedsReduction P1 Critical --- 1 finnur.webkit webkit-unassigned adele darin ddkilzer eric justin.garcia oldest_to_newest 118310 0 finnur.webkit 2009-04-21 13:41:47 -0700 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. 118311 1 eric 2009-04-21 14:08:50 -0700 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. 118312 2 justin.garcia 2009-04-21 14:11:23 -0700 I think ddkilzer ran into this, let me look... 118406 3 ddkilzer 2009-04-22 12:46:23 -0700 <rdar://problem/6805717> 118529 4 justin.garcia 2009-04-23 13:51:01 -0700 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); } 118530 5 darin 2009-04-23 13:56:34 -0700 (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! 118531 6 darin 2009-04-23 13:57:46 -0700 <rdar://problem/6788905> 118532 7 darin 2009-04-23 13:59:09 -0700 I probably broke this in http://trac.webkit.org/changeset/41928 118533 8 darin 2009-04-23 14:01:36 -0700 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. 118535 9 justin.garcia 2009-04-23 14:10:05 -0700 > 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. 118543 10 darin 2009-04-23 14:17:56 -0700 Actually VisiblePosition *is* checking the offset; just very slowly. That’s where we’re hanging! 118544 11 justin.garcia 2009-04-23 14:20:34 -0700 > 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... 118553 12 29717 adele 2009-04-23 15:42:43 -0700 Created attachment 29717 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. 118554 13 adele 2009-04-23 15:47:31 -0700 Committed revision 42794. Please let me know if anyone can still reproduce this after this revision. Thanks! 29717 2009-04-23 15:42:43 -0700 2009-04-23 15:43:09 -0700 patch patch_4-23-09.txt text/plain 1391 adele SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDQyNzkx KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMDktMDQt MjMgIEFkZWxlIFBldGVyc29uICA8YWRlbGVAYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeCBmb3IgPHJkYXI6Ly9wcm9ibGVtLzY3 ODg5MDU+IFJFR1JFU1NJT04gKDQxOTI4Pyk6IGhhbmcgaW4gUG9zaXRpb246OnVwc3RyZWFtCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNTMxMgorCisg ICAgICAgIEkgd2FzIHVuYWJsZSB0byByZXByb2R1Y2UgdGhlIHByb2JsZW0sIGJ1dCBJJ20gcHJl dHR5IHN1cmUgdGhpcyB3aWxsIGZpeCBpdC4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJP YmplY3QuY3BwOiAoV2ViQ29yZTo6UmVuZGVyT2JqZWN0OjpjcmVhdGVWaXNpYmxlUG9zaXRpb24p OgorICAgICAgICBTaW5jZSBWaXNpYmxlUG9zaXRpb24gZG9lc24ndCBlbnN1cmUgdGhlIG9mZnNl dCBpcyBnb29kLCB3ZSBzaG91bGRuJ3QgcGFzcyBtYXggaW50IGFzIGFuIG9mZnNldC4KKwogMjAw OS0wNC0yMyAgRGltaXRyaSBHbGF6a292ICA8ZGdsYXprb3ZAY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgpJbmRleDogcmVuZGVyaW5nL1JlbmRlck9iamVj dC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAJKHJldmlzaW9u IDQyNzkwKQorKysgcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTIzMjgsNyArMjMyOCw3IEBAIFZpc2libGVQb3NpdGlvbiBSZW5kZXJPYmplY3Q6OmNyZWF0ZVZp c2kKICAgICAgICAgICAgIGlmIChyZW5kZXJlciA9PSBwYXJlbnQpCiAgICAgICAgICAgICAgICAg YnJlYWs7CiAgICAgICAgICAgICBpZiAoTm9kZSogbm9kZSA9IHJlbmRlcmVyLT5ub2RlKCkpCi0g ICAgICAgICAgICAgICAgcmV0dXJuIFZpc2libGVQb3NpdGlvbihub2RlLCBudW1lcmljX2xpbWl0 czxpbnQ+OjptYXgoKSwgRE9XTlNUUkVBTSk7CisgICAgICAgICAgICAgICAgcmV0dXJuIFZpc2li bGVQb3NpdGlvbihsYXN0RGVlcEVkaXRpbmdQb3NpdGlvbkZvck5vZGUobm9kZSksIERPV05TVFJF QU0pOwogICAgICAgICB9CiAKICAgICAgICAgLy8gVXNlIHRoZSBwYXJlbnQgaXRzZWxmIHVubGVz cyBpdCB0b28gaXMgYW5vbnltb3VzLgo=