[Webkit-unassigned] [Bug 25703] Stack overflow crash rendering element with mega-huge number of background layers

Mon Jun 14 16:32:13 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=25703

David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57648|review?                     |review-
               Flag|                            |

--- Comment #10 from David Levin <levin at chromium.org>  2010-06-14 16:32:12 PST ---
(From update of attachment 57648)
Ok almost there!

---------------------------------
http://wkrietveld.appspot.com/25703/diff/1/2
File LayoutTests/ChangeLog (right):

http://wkrietveld.appspot.com/25703/diff/1/2#newcode5
LayoutTests/ChangeLog:5: Stack overflow crash rendering element with mega-huge number of background layers
"mega-huge" -- gianormous even?

How about "very large" (or even just put a number here like 100K)?

http://wkrietveld.appspot.com/25703/diff/1/2#newcode8
LayoutTests/ChangeLog:8: Adding layout test with huge number of background layers to make sure
a/huge number of/100k/

http://wkrietveld.appspot.com/25703/diff/1/2#newcode9
LayoutTests/ChangeLog:9: it does not cause stack overflow crash.
s/cause stack/cause a stack/

http://wkrietveld.appspot.com/25703/diff/1/4
File LayoutTests/css3/many-background-layers.html (right):

http://wkrietveld.appspot.com/25703/diff/1/4#newcode1
LayoutTests/css3/many-background-layers.html:1: <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN">
I think this test only verifies that RenderBox::paintFillLayers is correct (since that is all you fixed you you first had this test).

Is there any way to expand it to cover the other methods that you fixed?

http://wkrietveld.appspot.com/25703/diff/1/4#newcode4
LayoutTests/css3/many-background-layers.html:4: <title>Huge amout of background layers</title>
s/amout/amount/
I would use "number" here anyway.

http://wkrietveld.appspot.com/25703/diff/1/4#newcode21
LayoutTests/css3/many-background-layers.html:21: If you can read this, I have not crashed.
Something like
  "If this has not crashed, then the test passed."
would be more typical.

http://wkrietveld.appspot.com/25703/diff/1/5
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/25703/diff/1/5#newcode8
WebCore/ChangeLog:8: Having a large amount of background layers triggered a stack overflow
s/amount/number/

http://wkrietveld.appspot.com/25703/diff/1/5#newcode15
WebCore/ChangeLog:15: (WebCore::RenderBox::paintFillLayers): Iterate, rather than recurse, over subsequent layers. Use WTF::Vector with inline capacity of 8 so that heap allocation is not needed unless the number of layers is high.
Typically lines are wrapped by hand in the ChangeLog (see other entries).

Also, I don't understand why 8 is considered high. Why not 4 or 16? (Should it be a constant defined in the file?)

Lastly, have you done any perf testing of this change?

http://wkrietveld.appspot.com/25703/diff/1/5#newcode18
WebCore/ChangeLog:18: (WebCore::FillLayer::~FillLayer): Remove recursion.
If you have the same comment as before, "Ditto." works.

http://wkrietveld.appspot.com/25703/diff/1/6
File WebCore/rendering/RenderBox.cpp (right):

http://wkrietveld.appspot.com/25703/diff/1/6#newcode761
WebCore/rendering/RenderBox.cpp:761: WTF::Vector<const FillLayer*, 8> layers;
The WTF prefix isn't needed (and is discouraged).

http://wkrietveld.appspot.com/25703/diff/1/7
File WebCore/rendering/style/FillLayer.cpp (right):

http://wkrietveld.appspot.com/25703/diff/1/7#newcode77
WebCore/rendering/style/FillLayer.cpp:77: if (!shallowCopy) {
Return quickly.

If (shallowCopy)
    return;

http://wkrietveld.appspot.com/25703/diff/1/7#newcode140
WebCore/rendering/style/FillLayer.cpp:140: // to propagate patterns into layers.  All layer comparisons happen after values have all been filled in anyway.
I know it was this way before but a single space after a period is preferred.

http://wkrietveld.appspot.com/25703/diff/1/7#newcode156
WebCore/rendering/style/FillLayer.cpp:156: if (thisLayer->m_next && otherLayer->m_next) {
Consider returning early.

if (!thisLayer->m_next || !otherLayer->m_next)
    return thisLayer->m_next == otherLayer->m_next;

thisLayer = thisLayer->m_next;
otherLayer = otherLayer->m_next;

http://wkrietveld.appspot.com/25703/diff/1/8
File WebCore/rendering/style/FillLayer.h (right):

http://wkrietveld.appspot.com/25703/diff/1/8#newcode121
WebCore/rendering/style/FillLayer.h:121: FillLayer(const FillLayer& o, bool shallowCopy = false);
This extra bool parameter bothers me but after considering several other possibilities, I guess it is ok.

Normally, for bool parameters, we'd like to change them to be enums but it is only set from within the constructor.

Also I considered removing it and just calling the empty constructor with copying everything manually and not setting it using the initialize syntax but that didn't seem as elegant and likely had other minor side effects (like m_image getting initialized to 0 and then to another value.

So in the end... ok.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.