Bug 52144

Summary: Infinite recursion in WebCore::RenderInline::computeRectForRepaint
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: jchaffraix, rniwa, tony, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Repro
none
Proposed change: land the test case.
none
Better change with dumpAsText. none

Berend-Jan Wever
Reported 2011-01-10 03:08:09 PST
Created attachment 78389 [details] Repro Chromium bug: http://code.google.com/p/chromium/issues/detail?id=69044 Repro: <script> function go() { document.execCommand("selectall", false); document.designMode="on"; document.execCommand("insertparagraph", false); document.execCommand("insertparagraph", false); document.execCommand("InsertImage", false); document.execCommand("selectall"); document.execCommand("Strikethrough", false); document.execCommand("outdent", false); document.execCommand("InsertHorizontalRule", false); document.execCommand("SelectAll"); document.execCommand("InsertOrderedList"); document.execCommand("insertunorderedlist", false); document.execCommand("insertorderedlist"); document.execCommand("InsertHorizontalRule", false); document.execCommand("delete"); document.execCommand("Delete"); document.execCommand("Delete", false); document.execCommand("InsertHorizontalRule", false); document.execCommand("insertorderedlist", false); document.execCommand("JustifyFull"); document.execCommand("insertorderedlist"); document.execCommand("insertunorderedlist"); document.execCommand("insertunorderedlist", false); document.execCommand("Outdent"); document.execCommand("selectall", false); document.execCommand("insertparagraph", false); document.execCommand("insertimage", false); document.execCommand("InsertUnorderedList", false); document.execCommand("insertorderedlist"); document.execCommand("insertimage", false); document.execCommand("inserthorizontalrule", false); document.execCommand("Outdent"); document.execCommand("outdent"); document.execCommand("insertorderedlist"); document.execCommand("outdent"); document.execCommand("insertorderedlist"); } </script> <body onload="go()"> </body> id: chrome.dll!WebCore::RenderInline::computeRectForRepaint RecursionSOV (1183ea20d0a009e322db9c207b75d525) description: Recursive function call in chrome.dll!WebCore::RenderInline::computeRectForRepaint: 11622 loops stack: -- Start of 11622 loops -- chrome.dll!WebCore::RenderInline::computeRectForRepaint -- End of loop -- chrome.dll!WebCore::RenderBox::computeRectForRepaint chrome.dll!WebCore::RenderReplaced::clippedOverflowRectForRepaint chrome.dll!WebCore::RenderObject::repaint chrome.dll!WebCore::RenderObjectChildList::removeChildNode chrome.dll!WebCore::RenderObject::removeChild chrome.dll!WebCore::RenderObject::destroy chrome.dll!WebCore::Node::detach chrome.dll!WebCore::Element::detach -- Start of 5809 loops -- chrome.dll!WebCore::ContainerNode::detach chrome.dll!WebCore::Element::detach -- End of loop -- -- Start of 11857 loops -- chrome.dll!WebCore::Element::detach chrome.dll!WebCore::ContainerNode::detach -- End of loop --
Attachments
Repro (1.71 KB, text/html)
2011-01-10 03:08 PST, Berend-Jan Wever 		no flags
Details
Proposed change: land the test case. (10.25 KB, patch)
2011-11-17 11:42 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Better change with dumpAsText. (3.70 KB, patch)
2011-11-17 13:05 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Berend-Jan Wever
Comment 1 2011-03-03 06:58:44 PST
More details: this is a recursive function call in chrome.dll!WebCore::RenderInline::computeRectForRepaint. I bet the repro causes a loop in the renderer tree. When it gets rendered, the computeRectForRepaint funtion calls itself over and over until the all stack space is used. The browser then crashes.
Julien Chaffraix
Comment 2 2011-11-17 11:42:13 PST
Created attachment 115638 [details] Proposed change: land the test case.
Ryosuke Niwa
Comment 3 2011-11-17 11:44:16 PST
Comment on attachment 115638 [details] Proposed change: land the test case. Can we convert this to a dumpAsText test? Since the bug is about infinite recursion, it doesn't seem like we need to have png results?
Julien Chaffraix
Comment 4 2011-11-17 11:49:47 PST
(In reply to comment #3) > (From update of attachment 115638 [details]) > Can we convert this to a dumpAsText test? Since the bug is about infinite recursion, it doesn't seem like we need to have png results? I don't think this would work. Look at the trace, the infinite recursion is triggered by a repaint. If we do dumpAsText, repaint will never be called. I agree it is unfortunate to land this sub-optimal test but I thought increasing our test coverage is important enough to warrant landing the test.
Ryosuke Niwa
Comment 5 2011-11-17 11:54:31 PST
(In reply to comment #4) > I don't think this would work. Look at the trace, the infinite recursion is triggered by a repaint. If we do dumpAsText, repaint will never be called. I agree it is unfortunate to land this sub-optimal test but I thought increasing our test coverage is important enough to warrant landing the test. Really? I thought we DO repaint and all. It's just that we don't dump/store the result.
Julien Chaffraix
Comment 6 2011-11-17 12:57:26 PST
(In reply to comment #5) > (In reply to comment #4) > > I don't think this would work. Look at the trace, the infinite recursion is triggered by a repaint. If we do dumpAsText, repaint will never be called. I agree it is unfortunate to land this sub-optimal test but I thought increasing our test coverage is important enough to warrant landing the test. > > Really? I thought we DO repaint and all. It's just that we don't dump/store the result. Double checking Chromium's code, it looks like we never paint if dumpAsText is called. However the repaint logic is exercised (invalidating the different rectangles as needed). I think you are right and we can convert that to dumpAsText safely. Let me change the patch.
Julien Chaffraix
Comment 7 2011-11-17 13:05:43 PST
Created attachment 115661 [details] Better change with dumpAsText.
Tony Chang
Comment 8 2011-11-17 16:24:44 PST
Comment on attachment 115661 [details] Better change with dumpAsText. Do you know which change fixed the bug? If so, it would be nice to make a mention of it in the ChangeLog.
Julien Chaffraix
Comment 9 2011-11-17 18:33:12 PST
(In reply to comment #8) > (From update of attachment 115661 [details]) > Do you know which change fixed the bug? If so, it would be nice to make a mention of it in the ChangeLog. It's a very good question. Unfortunately I did not bisect to know when it was fixed and I don't think I will have enough time to do the actual bisection...
WebKit Review Bot
Comment 10 2011-11-18 11:20:49 PST
Comment on attachment 115661 [details] Better change with dumpAsText. Clearing flags on attachment: 115661 Committed r100801: <http://trac.webkit.org/changeset/100801>
WebKit Review Bot
Comment 11 2011-11-18 11:20:54 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.