Bug 48255

Summary:

Stack overflow when there are too many sibling inline boxes

Product:

WebKit

Reporter:

Yong Li <yong.li.webkit>

Component:

DOM

Assignee:

Yong Li <yong.li.webkit>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric, kling, staikos

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	abarth: review-
updated with much smaller test case	eric: review-
Updated	none

Yong Li

Reported 2010-10-25 11:41:59 PDT

The root reason is this function in InlineBox: virtual void setConstructed() { m_constructed = true; if (m_next) m_next->setConstructed(); } However, it is not very normal that a page contains that many sibling inline boxes. Not sure if it is worth a fix. I'll upload a patch anyway.

Attachments
the patch (1.01 MB, patch) 2010-10-25 11:49 PDT, Yong Li	abarth: review-	Details Formatted Diff Diff
updated with much smaller test case (3.08 KB, patch) 2010-11-02 11:23 PDT, Yong Li	eric: review-	Details Formatted Diff Diff
Updated (3.50 KB, patch) 2010-12-13 10:00 PST, Yong Li	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yong Li

Comment 1 2010-10-25 11:49:43 PDT

Created attachment 71778 [details] the patch The test case crashes Safari 5 win32.

Adam Barth

Comment 2 2010-10-31 18:36:30 PDT

Comment on attachment 71778 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=71778&action=review > LayoutTests/fast/overflow/lots-of-sibling-inline-boxes.html:3 > +Make sure browser doesn't crash with lots of inline boxes in parallel. Can you generate this test file using document.write instead of having infinite <a> tags in the source?

Yong Li

Comment 3 2010-11-02 08:52:01 PDT

(In reply to comment #2) > (From update of attachment 71778 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71778&action=review > > LayoutTests/fast/overflow/lots-of-sibling-inline-boxes.html:3 > > +Make sure browser doesn't crash with lots of inline boxes in parallel. > Can you generate this test file using document.write instead of having infinite <a> tags in the source? Good point. I will give a try

Yong Li

Comment 4 2010-11-02 11:23:56 PDT

Created attachment 72703 [details] updated with much smaller test case It needs 130k inline boxes to bring Safari down. So still not certain it is worth doing. Probably it is good for mobile platforms in the case call stack is much smaller than desktop.

Andreas Kling

Comment 5 2010-11-07 20:07:35 PST

Comment on attachment 72703 [details] updated with much smaller test case View in context: https://bugs.webkit.org/attachment.cgi?id=72703&action=review > WebCore/rendering/InlineBox.h:165 > - virtual void setConstructed() > - { > - m_constructed = true; > - if (m_next) > - m_next->setConstructed(); > - } > + virtual void setConstructed() { m_constructed = true; } This looks like a behavior change to me, InlineBox::setConstructed() will no longer affect sibling boxes. Is this correct (and safe)? > WebCore/rendering/InlineBox.h:284 > + InlineBox* next() { return m_next; } This method should be const.

Yong Li

Comment 6 2010-11-09 07:28:37 PST

(In reply to comment #5) > (From update of attachment 72703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72703&action=review > > WebCore/rendering/InlineBox.h:165 > > - virtual void setConstructed() > > - { > > - m_constructed = true; > > - if (m_next) > > - m_next->setConstructed(); > > - } > > + virtual void setConstructed() { m_constructed = true; } > This looks like a behavior change to me, InlineBox::setConstructed() will no longer affect sibling boxes. > Is this correct (and safe)? It is safe. > > WebCore/rendering/InlineBox.h:284 > > + InlineBox* next() { return m_next; } > This method should be const. It is returning a non-const pointer. If we make such methods "const", it is potentially possible that some code gets non-const pointer through const pointer without a const_cast. Are we sure we want to do that? I think usually we should add another const method when we need it: const InlineBox* next() const { return m_next; }

Andreas Kling

Comment 7 2010-11-09 07:36:22 PST

Comment on attachment 72703 [details] updated with much smaller test case You are right, I jumped the gun here, resetting r?

Eric Seidel (no email)

Comment 8 2010-12-12 02:46:29 PST

Comment on attachment 72703 [details] updated with much smaller test case View in context: https://bugs.webkit.org/attachment.cgi?id=72703&action=review This needs an expectation file and should be dump as text. > LayoutTests/fast/overflow/lots-of-sibling-inline-boxes.html:2 > +<html> > +<body> You dont' actually need these. :) > LayoutTests/fast/overflow/lots-of-sibling-inline-boxes.html:4 > +<script> This should be dumpAsText(). > LayoutTests/fast/overflow/lots-of-sibling-inline-boxes.html:9 > +</body> > +</html> Or these. EOF newlines are nice for making diff not complain. :)

Yong Li

Comment 9 2010-12-13 10:00:26 PST

Created attachment 76400 [details] Updated

Darin Adler

Comment 10 2010-12-13 10:27:35 PST

Comment on attachment 76400 [details] Updated If our aim is to eliminate unnecessary recursion, it would be nice to walk the entire tree without recursing. It’s good to eliminate recursing for each sibling, yet this still recurses if children in turn have children. The trick is to write a traversal function like Node::traverseNextNode or RenderObject::nextInPreOrder and use it. If we do that we might also not want setConstructed to be a virtual function any more.

Eric Seidel (no email)

Comment 11 2010-12-14 15:14:43 PST

Attachment 76400 [details] was posted by a committer and has review+, assigning to Yong Li for commit.

WebKit Commit Bot

Comment 12 2010-12-15 15:06:53 PST

Comment on attachment 76400 [details] Updated Clearing flags on attachment: 76400 Committed r74145: <http://trac.webkit.org/changeset/74145>

WebKit Commit Bot

Comment 13 2010-12-15 15:07:01 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.