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

Fri Jun 18 08:46:22 PDT 2010

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


--- Comment #11 from hans at chromium.org  2010-06-18 08:46:21 PST ---
(In reply to comment #10)
> (From update of attachment 57648 [details])
> Ok almost there!
Thank you for the thorough review.

> 
> 
> ---------------------------------
> 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)?
Done.

> 
> 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/
Going for "very large" here as well.

> 
> 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/
Done.

> 
> 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?
I have tried this, but it is not straight-forward, at least not the way the test works now. The problem is that for example ~FillLayer() requires a higher number of layers to crash (350k on my machine), making the layout test impractical to have in the test suite. I am not sure how much trouble it is worth to have automatic tests for them.

> 
> 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.
Done.

> 
> 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.
Done.

> 
> 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/
Done.

> 
> 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).
Done.

> 
> Also, I don't understand why 8 is considered high. Why not 4 or 16? (Should it be a constant defined in the file?)
I have done some experiments using the Chrome page cycler tests and the layout tests. I hit this function about 120k times. Nine layers occurred once, four layers about 20 times, two layers about 60 times, and the rest of the time the number of layers was one. So I believe 8 is high enough to accommodate all common cases. Changing the ChangeLog text slightly to make it more clear, and also added a comment about this to the source.

> 
> Lastly, have you done any perf testing of this change?
Running the Chrome page cyclers, I have not been able to detect any change in performance with this patch.

> 
> 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.
Done.

> 
> 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).
Done.

> 
> 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;
Done.

> 
> 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.
Done.

> 
> 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;
Done.

> 
> 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.
Ok.

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