Bug 38851 – Large SVG rect with shadow fails to render

• Component: SVG
• Status: RESOLVED FIXED

[W. James MacLean]

Created attachment 55559 [details]
SVG large rect with shadow

Addition of a shadow to a large rect (x=0,y=0,height=100,width=2147483647) causes it to no longer render.

Platform: Chromium (most recent build)

Steps to reproduce: Load the attached file, rect-with-shadow-maxing.svg in Chromium.

Expected output: A purple rectangle at the top of the viewport, with a grey shadow underneath. The rect will fill the width of the viewport.

Actual output: No rectangle or shadow is rendered.

Additional Information: This is a contrived example to demonstrate the danger of unsafe float->int type conversions in the function SVGRenderStyle::inflateForShadow(FloatRect &). This function converts the FloatRect into integer values, then back to float with the shadow dimensions added. By choosing x=0 and width=2147483647 this ensures that adding any non-zero shadow to the right-hand side results in a value too large to store in an int, thus returning -2147483648 for the width of the inflated rect.

To observe that this error does not occur when the shadow is removed, simply remove "-webkit-svg-shadow: 5px 5px 5px grey" from the rect specification.

Although submitted as a Chromium/WebKit bug, it is expected this same effect will be seen with other WebKit-based browsers on other platforms.

Although found while debugging Bug 25645, this bug is independent of it.

[Rob Buis]

Hello James,

(In reply to comment #0)
> Created an attachment (id=55559) [details]
> SVG large rect with shadow
> 
> Addition of a shadow to a large rect (x=0,y=0,height=100,width=2147483647) causes it to no longer render.
> 
> Platform: Chromium (most recent build)
> 
> Steps to reproduce: Load the attached file, rect-with-shadow-maxing.svg in Chromium.
> 
> Expected output: A purple rectangle at the top of the viewport, with a grey shadow underneath. The rect will fill the width of the viewport.
> 
> Actual output: No rectangle or shadow is rendered.

I tried using floats instead of ints for storing the 4 intermediate shadow values, then the following addition is all done using floats and the .svg shows up fine. I doubt there would be performance problems with such a change, and I guess shadows are rare in typical svgs, so do you think that would be good enough to fix this bug? I can make a patch for that, but the replacement is so simple that I think you can imagine it easily :) Let me know what you think.
Cheers,

Rob.

[W. James MacLean]

(In reply to comment #1)
> Hello James,
> 
> I tried using floats instead of ints for storing the 4 intermediate shadow values, then the following addition is all done using floats and the .svg shows up fine. I doubt there would be performance problems with such a change, and I guess shadows are rare in typical svgs, so do you think that would be good enough to fix this bug? I can make a patch for that, but the replacement is so simple that I think you can imagine it easily :) Let me know what you think.
> Cheers,
> 
> Rob.

Looks good to me! Did you want to submit a patch on this bug?

J

[Rob Buis]

Hello James,

(In reply to comment #2)
> (In reply to comment #1)
> > Hello James,
> > 
> > I tried using floats instead of ints for storing the 4 intermediate shadow values, then the following addition is all done using floats and the .svg shows up fine. I doubt there would be performance problems with such a change, and I guess shadows are rare in typical svgs, so do you think that would be good enough to fix this bug? I can make a patch for that, but the replacement is so simple that I think you can imagine it easily :) Let me know what you think.
> > Cheers,
> > 
> > Rob.
> 
> Looks good to me! Did you want to submit a patch on this bug?
> 
> J

Sure can do, if nobody beats me to it :) I need some sleep now, but can defintely post a patch tomorrow.
Cheers,

Rob.

[Rob Buis]

Created attachment 57192 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 57192 [details]
Patch

Damn, my comment disappeared, retyping, but not that explicit anymore :( Anyhow, r- because of some nitpicks:

LayoutTests/svg/filters/shadow-on-rect-large.svg:2
 +  <rect x="0" y="0" width="2147483647" height="300" fill="green" style="-webkit-svg-shadow:5px 5px 5px grey;"/>
Please add a comment here, stating that it passes if it does not crash, and maybe add the bug numbe.r

The code change itself is fine, though it could be optimized. How about just using:
repaintRect.move(shadowLeft, shadowRight);
repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));

That would save any intermediate int/float value, and is easier to read IMHO.
What do you think?

Cheers,
Niko

[Rob Buis]

Hello Niko,

Thanks for speedy review.

(In reply to comment #5)
> (From update of attachment 57192 [details])
> Damn, my comment disappeared, retyping, but not that explicit anymore :( Anyhow, r- because of some nitpicks:
> 
> LayoutTests/svg/filters/shadow-on-rect-large.svg:2
>  +  <rect x="0" y="0" width="2147483647" height="300" fill="green" style="-webkit-svg-shadow:5px 5px 5px grey;"/>
> Please add a comment here, stating that it passes if it does not crash, and maybe add the bug numbe.r

I sort of knew when I uploaded the patch that that was lacking :) Notice there is no crash though, just nothing is rendered! Do you think with that knowledge the test can be reduced further, with no or a smaller pixel result (maybe 1x1)?
 
> The code change itself is fine, though it could be optimized. How about just using:
> repaintRect.move(shadowLeft, shadowRight);
> repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));
> 
> That would save any intermediate int/float value, and is easier to read IMHO.
> What do you think?

I have to see when I get home. I am not sure you know, but when doing bug fixes is it ok/desired to optimize the code in the change? I guess it does make sense when the change is in the same code section, just wondering whether anything solid is written about that.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #6)
> Hello Niko,
> 
> Thanks for speedy review.
> 
> (In reply to comment #5)
> > (From update of attachment 57192 [details] [details])
> > Damn, my comment disappeared, retyping, but not that explicit anymore :( Anyhow, r- because of some nitpicks:
> > 
> > LayoutTests/svg/filters/shadow-on-rect-large.svg:2
> >  +  <rect x="0" y="0" width="2147483647" height="300" fill="green" style="-webkit-svg-shadow:5px 5px 5px grey;"/>
> > Please add a comment here, stating that it passes if it does not crash, and maybe add the bug numbe.r
> 
> I sort of knew when I uploaded the patch that that was lacking :) Notice there is no crash though, just nothing is rendered! Do you think with that knowledge the test can be reduced further, with no or a smaller pixel result (maybe 1x1)?

Oh sorry, of course it doesn't crash, misread :-) I'm not sure whether the test can be further reduced, I think it's okay this way - the final size is 800x600 anyways, and when the bug would appear again in future, we'd easily spot because of large differences in the pixel tests.


> 
> > The code change itself is fine, though it could be optimized. How about just using:
> > repaintRect.move(shadowLeft, shadowRight);
> > repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));
> > 
> > That would save any intermediate int/float value, and is easier to read IMHO.
> > What do you think?
> 
> I have to see when I get home. I am not sure you know, but when doing bug fixes is it ok/desired to optimize the code in the change? I guess it does make sense when the change is in the same code section, just wondering whether anything solid is written about that.
It's definately okay - the underlying issue is that we're actually using intermediate values, where not necessary. You could of course split up in two patches, the first switching int to float and the next to optimize the code, but I think it's pointless if it just affects a few lines of code.

Cheers,
Niko

[Rob Buis]

Created attachment 57264 [details]
Patch

[Rob Buis]

(In reply to comment #8)
> Created an attachment (id=57264) [details]
> Patch

Here is the new patch with the code change. I left the testcase + results unchanged. I noticed
svg/css/arrow-with-shadow.svg and svg/css/stars-with-shadow.html have slightly different
text results because of the patch(because of rounding changes) but same pixel results, let me know if that should be part of the patch or could be done seperately.
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 57264 [details]
Patch

Hey Rob,

the ChangeLog needs an update, you're not using intermediate values anymore :-)
Almost r+, but you've said that there are results that have changed, and the pixel tests stayed the same? Sounds awkward to me, did you use "run-webkit-tests --tolerance 0 -p svg" ?
If there are changes, please update a full patch with all changes.
Sorry for the new round...

Cheers,
Niko

[Rob Buis]

Created attachment 57415 [details]
Patch

[Rob Buis]

(In reply to comment #11)
> Created an attachment (id=57415) [details]
> Patch

This patch causes no regressions in the svg test results, so I didnt upload any changed results.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=57415) [details] [details]
> > Patch
> 
> This patch causes no regressions in the svg test results, so I didnt upload any changed results.
> Cheers,

Hey Rob,

this patch still puzzles me. I don't know why we actually have to clamp to intmax??
If we just get rid of any intermediate integers, we can use floatmax no?

Just a quick test:
float floatMin = std::numeric_limits<float>::min();
float floatMax = std::numeric_limits<float>::max();
std::cout << "FLOAT min=" << floatMin << " max=" << floatMax << std::endl;

int intMin = std::numeric_limits<int>::min();
int intMax = std::numeric_limits<int>::max();
std::cout << "  INT min=" << intMin << " max=" << intMax << std::endl;

Result:
FLOAT min=1.17549e-38 max=3.40282e+38
  INT min=-2147483648 max=2147483647


If you just store shadowLeft/shadowRight and shadowBottom/shadowTop in float values (changing getSVGShadowExtent to operator on floats, not integers), then following code should just work, no?

repaintRect.move(shadowLeft, shadowRight);
repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));

That would remove the need for the intermediate overflow* values, and we wouldn't need to clamp to just integer range, which sounds wrong to me.

Cheers,
Niko

P.S. Not setting r-, to discuss this a bit further.

[Rob Buis]

Hello Niko,

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created an attachment (id=57415) [details] [details] [details]
> > > Patch
> > 
> > This patch causes no regressions in the svg test results, so I didnt upload any changed results.
> > Cheers,
> 
> Hey Rob,
> 
> this patch still puzzles me. I don't know why we actually have to clamp to intmax??
> If we just get rid of any intermediate integers, we can use floatmax no?
> 
> Just a quick test:
> float floatMin = std::numeric_limits<float>::min();
> float floatMax = std::numeric_limits<float>::max();
> std::cout << "FLOAT min=" << floatMin << " max=" << floatMax << std::endl;
> 
> int intMin = std::numeric_limits<int>::min();
> int intMax = std::numeric_limits<int>::max();
> std::cout << "  INT min=" << intMin << " max=" << intMax << std::endl;
> 
> Result:
> FLOAT min=1.17549e-38 max=3.40282e+38
>   INT min=-2147483648 max=2147483647
> 
> 
> If you just store shadowLeft/shadowRight and shadowBottom/shadowTop in float values (changing getSVGShadowExtent to operator on floats, not integers), then following code should just work, no?
> 
> repaintRect.move(shadowLeft, shadowRight);

You mean repaintRect.move(shadowLeft, shadowTop) here :)

> repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));
> 
> That would remove the need for the intermediate overflow* values, and we wouldn't need to clamp to just integer range, which sounds wrong to me.

All the trickery in the patch was only needed to prevent any changed results. Using all floats works as well, but causes 6/7 changed results. All seem to be small, with topmost 2 pixels changed per result. I can make a patch for that, and in fact was almost done, but got some message about pixel hash changed even though pixel result was correct, which I did not really understand. Let me know what you think.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #14)
> > repaintRect.move(shadowLeft, shadowRight);
> 
> You mean repaintRect.move(shadowLeft, shadowTop) here :)
Right, sorry for that :-)

> 
> > repaintRect.setSize(repaintRect.size() + FloatSize(shadowRight - shadowLeft, shadowBottom - shadowTop));
> > 
> > That would remove the need for the intermediate overflow* values, and we wouldn't need to clamp to just integer range, which sounds wrong to me.
> 
> All the trickery in the patch was only needed to prevent any changed results. Using all floats works as well, but causes 6/7 changed results. All seem to be small, with topmost 2 pixels changed per result. I can make a patch for that, and in fact was almost done, but got some message about pixel hash changed even though pixel result was correct, which I did not really understand. Let me know what you think.

That's normal, sometimes it results in subtle alpha diffs - just update the changed hashs. It's fine to change the pixel tests results, happily waiting for the new patch :-)

Cheers,
Niko

[Rob Buis]

Created attachment 57476 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 57476 [details]
Patch

Looks great, r=me. Please fix a minor issue before landing:

LayoutTests/ChangeLog:8
 +          Add a new test and update changed testresults.
Please use a more descriptive ChangeLog. Something like "Add new test covering that large shadow rects get renderered now. Update test results that show slight differences, after fixing the repaintRect calulcations."

[WebKit Review Bot]

http://trac.webkit.org/changeset/60503 might have broken Qt Linux Release

[James Robinson]

fast/repaint/moving-shadow-on-path-pretty needs a text rebaseline after this patch, see http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r60503%20(15454)/fast/repaint/moving-shadow-on-path-pretty-diff.html.

[James Robinson]

Committed r60516: <http://trac.webkit.org/changeset/60516>