RESOLVED FIXED 48255
Stack overflow when there are too many sibling inline boxes 
https://bugs.webkit.org/show_bug.cgi?id=48255
Summary Stack overflow when there are too many sibling inline boxes
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-
DetailsFormatted DiffDiff
updated with much smaller test case (3.08 KB, patch)
2010-11-02 11:23 PDT, Yong Li 		eric: review-
DetailsFormatted DiffDiff
Updated (3.50 KB, patch)
2010-12-13 10:00 PST, Yong Li 		no flags
DetailsFormatted DiffDiff
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.