[webkit-reviews] review denied: [Bug 25703] Stack overflow crash rendering element with mega-huge number of background layers : [Attachment 57648] Patch

Mon Jun 14 16:32:11 PDT 2010

David Levin <levin at chromium.org> has denied hans at chromium.org's request for
review:
Bug 25703: Stack overflow crash rendering element with mega-huge number of
background layers
https://bugs.webkit.org/show_bug.cgi?id=25703

Attachment 57648: Patch
https://bugs.webkit.org/attachment.cgi?id=57648&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.