Bug 52144 - Infinite recursion in WebCore::RenderInline::computeRectForRepaint
Summary: Infinite recursion in WebCore::RenderInline::computeRectForRepaint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-10 03:08 PST by Berend-Jan Wever
Modified: 2011-11-18 11:20 PST (History)
4 users (show)

See Also:

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 Details | Formatted Diff | Diff
Better change with dumpAsText. (3.70 KB, patch)
2011-11-17 13:05 PST, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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 --
Comment 1 Berend-Jan Wever 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.
Comment 2 Julien Chaffraix 2011-11-17 11:42:13 PST 
Created attachment 115638 [details]
Proposed change: land the test case.
Comment 3 Ryosuke Niwa 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?
Comment 4 Julien Chaffraix 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Julien Chaffraix 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.
Comment 7 Julien Chaffraix 2011-11-17 13:05:43 PST 
Created attachment 115661 [details]
Better change with dumpAsText.
Comment 8 Tony Chang 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.
Comment 9 Julien Chaffraix 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...
Comment 10 WebKit Review Bot 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>
Comment 11 WebKit Review Bot 2011-11-18 11:20:54 PST 
All reviewed patches have been landed.  Closing bug.