Bug 29912

Summary: Avoid zero division during SVGPaintServerPattern::setup()
Product: WebKit Reporter: Shiki Okasaka <shiki>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, krit, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
URL: http://crbug.com/14521
Attachments:
Description Flags
Proposed fix for a Chromium SVG crash
krit: review-
LayoutTest for SVG pattern crash
eric: review-, krit: commit-queue-
Proposed patch (rev.2) eric: review-, krit: commit-queue-

Description Shiki Okasaka 2009-09-29 22:34:33 PDT
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.
Comment 1 Eric Seidel (no email) 2009-10-02 12:42:49 PDT
I'm confused as to what you mean by "tileRect is not empty with Skia".  Why is this needed?
Comment 2 Shiki Okasaka 2009-10-04 21:43:38 PDT
(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.
Comment 3 Eric Seidel (no email) 2009-10-04 22:29:25 PDT
OK, so why does BitmapPlatformDevice::create() do that? :)
Comment 4 Shiki Okasaka 2009-10-04 22:59:53 PDT
(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.
Comment 5 Dirk Schulze 2009-10-07 08:07:53 PDT
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.
Comment 6 Adam Barth 2009-10-18 22:41:03 PDT
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.
Comment 7 Dirk Schulze 2009-11-27 13:30:28 PST
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.
Comment 8 Adam Barth 2009-11-27 14:11:41 PST
Comment on attachment 43968 [details]
LayoutTest for SVG pattern crash

The test looks good.
Comment 9 Adam Barth 2009-11-27 14:16:15 PST
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.
Comment 10 Adam Barth 2009-11-30 12:47:22 PST
style-queue ran check-webkit-style on attachment 40344 [details] without any errors.
Comment 11 Dirk Schulze 2009-12-01 01:14:31 PST
(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.
Comment 12 Dirk Schulze 2009-12-04 11:13:54 PST
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.
Comment 13 Shiki Okasaka 2009-12-06 21:05:42 PST
Created attachment 44376 [details]
Proposed patch (rev.2)

Thank you very much for reviewing this again. I have updated the ChangeLog as requested.
Comment 14 WebKit Review Bot 2009-12-06 21:10:36 PST
style-queue ran check-webkit-style on attachment 44376 [details] without any errors.
Comment 15 Dirk Schulze 2009-12-07 03:52:12 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

LGTM
Comment 16 WebKit Commit Bot 2009-12-07 03:55:26 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

krit@webkit.org 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.
Comment 17 WebKit Commit Bot 2009-12-07 04:05:38 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

krit@webkit.org 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.
Comment 18 Dirk Schulze 2009-12-07 04:07:37 PST
I have added myself to the list :-/. I'll try it later again :-)
Comment 19 Eric Seidel (no email) 2009-12-07 10:37:51 PST
Due to bug 30084.  I"ll restart the bot shortly.
Comment 20 Eric Seidel (no email) 2009-12-07 11:32:08 PST
I've restarted the bot.  It should not respect your r+.  Sorry for the trouble.
Comment 21 Dirk Schulze 2009-12-07 12:21:46 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

Thanks Eric.
Comment 22 WebKit Commit Bot 2009-12-07 12:22:42 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

Rejecting patch 44376 from review queue.

krit@webkit.org 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.
Comment 23 Dirk Schulze 2009-12-07 12:23:51 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

I think I'll land it manually now :-)
Comment 24 Dirk Schulze 2009-12-07 12:33:04 PST
landed in r51789. Closing bug now.
Comment 25 Eric Seidel (no email) 2009-12-07 16:12:16 PST
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.
Comment 26 Nikolas Zimmermann 2009-12-07 18:30:21 PST
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?
Comment 27 Dirk Schulze 2009-12-08 04:04:36 PST
(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.
Comment 28 Eric Seidel (no email) 2009-12-28 22:15:17 PST
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.
Comment 29 Eric Seidel (no email) 2009-12-29 09:51:23 PST
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.
Comment 30 Eric Seidel (no email) 2009-12-29 09:52:42 PST
Comment on attachment 43968 [details]
LayoutTest for SVG pattern crash

Marking r- since this was rolled out.
Comment 31 Eric Seidel (no email) 2009-12-29 09:53:14 PST
Comment on attachment 44376 [details]
Proposed patch (rev.2)

Marking r- since this was rolled out.
Comment 32 Dirk Schulze 2010-07-01 09:17:28 PDT
Fixed in trunk. Closing bug.