Bug 29912 – Avoid zero division during SVGPaintServerPattern::setup()

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Shiki Okasaka]

Created attachment 40344 [details]
Proposed fix for a Chromium SVG crash

Fix a Chromium SVG crash when visiting
  http://upload.wikimedia.org/wikipedia/commons/c/c2/World_map_pol_2005_v02.svg
as reported by
  http://crbug.com/14521

The issue is tileRect is not empty with Skia when tileWidth or tileHeight becomes
zero, and the calculation of numY or numX causes a zero division.

[Eric Seidel (no email)]

I'm confused as to what you mean by "tileRect is not empty with Skia".  Why is this needed?

[Shiki Okasaka]

(In reply to comment #1)
> I'm confused as to what you mean by "tileRect is not empty with Skia".  Why is
> this needed?

It's due to Skia's BitmapPlatformDevice::create() implementation for Windows; it creates a 1x1 bitmap when either the requested width or height is zero.

http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/bitmap_platform_device_win.cc

The revised numY and numX calculations also seem to be more consistent with the following tile image drawing logic.

[Eric Seidel (no email)]

OK, so why does BitmapPlatformDevice::create() do that? :)

[Shiki Okasaka]

(In reply to comment #3)
> OK, so why does BitmapPlatformDevice::create() do that? :)

It says,

  // CreateDIBSection appears to get unhappy if we create an empty bitmap, so
  // just create a minimal bitmap

If we fix this, it looks we also need to fix SkBitmap::copyTo(), etc., which cannot handle empty bitmaps.

[Dirk Schulze]

It is a general bug. I see it on WebKitGtk too. Get Floating point exception.

But this patch will definitely break a pixel-test, at least stroked-pattern-expected.svg. Even when we don't have a pixel-test bot :-(

I wonder if we have to draw a pattern at all, if tileWidth or tileHeight are 0. Since we don't have an image to draw.

tileHeight and tileWidth are the rounded results of patternBoundaries().width(), patternBoundaries().height(). In the case that one of the both boundaries are not 0 and < .5, we might set tileHeight or tileWidth to 1.f. And make an early return if one of them is 0.

[Adam Barth]

Comment on attachment 40344 [details]
Proposed fix for a Chromium SVG crash

Please add a test.  I don't know whether the code change is correct.

[Dirk Schulze]

Created attachment 43968 [details]
LayoutTest for SVG pattern crash

This patch includes a test case for the bug.

Luckily I was wrong and the previous patch did not break the mentioned test (stroked-pattern.svg) or the one of batik.

This crash is still a general issue and happens if the pattern size is smaller than the tile of the pattern and the attribute overview is set to visible. Also the pattern size must be smaller than 0.5 in any dimension.

I would suggest to review "Proposed fix for a Chromium SVG crash" again with this patch as Layouttest.

[Adam Barth]

Comment on attachment 43968 [details]
LayoutTest for SVG pattern crash

The test looks good.

[Adam Barth]

Comment on attachment 40344 [details]
Proposed fix for a Chromium SVG crash

> I would suggest to review "Proposed fix for a Chromium SVG crash" again with
> this patch as Layouttest.

Dirk, the patch seems fine with me with the test, but I'm not an expert on SVG, so I'll defer to your review.

[Adam Barth]

style-queue ran check-webkit-style on attachment 40344 [details] without any errors.

[Dirk Schulze]

(In reply to comment #9)
> (From update of attachment 40344 [details])
> > I would suggest to review "Proposed fix for a Chromium SVG crash" again with
> > this patch as Layouttest.
> 
> Dirk, the patch seems fine with me with the test, but I'm not an expert on SVG,
> so I'll defer to your review.

I'm not a reviewer. But the patch of Shiki looks good to me. Just the changelog is a bit confusing. I don't see a refernce to Skia here. It's just a zero division bug in SVGPatternElement.

[Dirk Schulze]

Comment on attachment 40344 [details]
Proposed fix for a Chromium SVG crash

The patch looks great.

> Index: WebCore/ChangeLog
...
> +
> +        Fix a Chromium SVG crash when visiting
> +        http://upload.wikimedia.org/wikipedia/commons/c/c2/World_map_pol_2005_v02.svg
> +        as reported by
> +        http://crbug.com/14521
> +
> +        The issue is tileRect is not empty with Skia when tileWidth or tileHeight becomes
> +        zero, and the calculation of numY or numX causes a zero division.
> +
> +        * svg/graphics/SVGPaintServerPattern.cpp:
> +        (WebCore::SVGPaintServerPattern::setup):

The discription doesn't adress the main problem: the zero division in SVGPaintServerPattern. I don't see a connection between the fix, the problem and the comment about Skia. Please add the topic of the bug and the link to the bug report at the beginning of the Changelog.

r- for the Changelog.

[Shiki Okasaka]

Created attachment 44376 [details]
Proposed patch (rev.2)

Thank you very much for reviewing this again. I have updated the ChangeLog as requested.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44376 [details] without any errors.

[Dirk Schulze]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

LGTM

[WebKit Commit Bot]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed) and then set the reviewer flag again.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

[WebKit Commit Bot]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed) and then set the reviewer flag again.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

[Dirk Schulze]

I have added myself to the list :-/. I'll try it later again :-)

[Eric Seidel (no email)]

Due to bug 30084.  I"ll restart the bot shortly.

[Eric Seidel (no email)]

I've restarted the bot.  It should not respect your r+.  Sorry for the trouble.

[Dirk Schulze]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

Thanks Eric.

[WebKit Commit Bot]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed) and then set the reviewer flag again.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

[Dirk Schulze]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

I think I'll land it manually now :-)

[Dirk Schulze]

landed in r51789. Closing bug now.

[Eric Seidel (no email)]

Sorry for the trouble.  Turns out I was running an old copy of the commit-queue even after I thought I had updated it.  Fixed now.

[Nikolas Zimmermann]

Reopen bug. Had to revert the patch, as it caused crashes on all win build slaves. Reverted in revision 51823. Dirk, can you please have a look?

[Dirk Schulze]

(In reply to comment #26)
> Reopen bug. Had to revert the patch, as it caused crashes on all win build
> slaves. Reverted in revision 51823. Dirk, can you please have a look?
Sorry, I checked the buildbots, but didn't noticed the crash on windows.

I don't know why the test fails on windows. I doubt, that 1/0.1 is a zero division on windows. I looked at the svg pattern code again. We create a ImageBuffer with the size of 0x0 pixel, this is contraproductiv and maybe cause the crash on either ImageBuffer or platform pattern. 
We need a backtrace from windows webkit with this patch.

[Eric Seidel (no email)]

I'm confused as to the status of this patch.  Should the patches be r-'d?  It sounds like since they were reverted they should be.  Right now this is r+'d (and thus in the pending-commit list), but seems unactionable.

[Eric Seidel (no email)]

Looks like http://trac.webkit.org/changeset/51823 reverted both of these patches.  I'm going to mark them both r- as a result.  To resolve this bug we'll need to have a fixed (combined) patch posted.

[Eric Seidel (no email)]

Comment on attachment 43968 [details]
LayoutTest for SVG pattern crash

Marking r- since this was rolled out.

[Eric Seidel (no email)]

Comment on attachment 44376 [details]
Proposed patch (rev.2)

Marking r- since this was rolled out.

[Dirk Schulze]

Fixed in trunk. Closing bug.