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.
Created attachment 71778 [details] the patch The test case crashes Safari 5 win32.
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?
(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
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.
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.
(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; }
Comment on attachment 72703 [details] updated with much smaller test case You are right, I jumped the gun here, resetting r?
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. :)
Created attachment 76400 [details] Updated
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.
Attachment 76400 [details] was posted by a committer and has review+, assigning to Yong Li for commit.
Comment on attachment 76400 [details] Updated Clearing flags on attachment: 76400 Committed r74145: <http://trac.webkit.org/changeset/74145>
All reviewed patches have been landed. Closing bug.