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.
I'm confused as to what you mean by "tileRect is not empty with Skia". Why is this needed?
(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.
OK, so why does BitmapPlatformDevice::create() do that? :)
(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.
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 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.
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 on attachment 43968 [details] LayoutTest for SVG pattern crash The test looks good.
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.
style-queue ran check-webkit-style on attachment 40344 [details] without any errors.
(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 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.
Created attachment 44376 [details] Proposed patch (rev.2) Thank you very much for reviewing this again. I have updated the ChangeLog as requested.
style-queue ran check-webkit-style on attachment 44376 [details] without any errors.
Comment on attachment 44376 [details] Proposed patch (rev.2) LGTM
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.
I have added myself to the list :-/. I'll try it later again :-)
Due to bug 30084. I"ll restart the bot shortly.
I've restarted the bot. It should not respect your r+. Sorry for the trouble.
Comment on attachment 44376 [details] Proposed patch (rev.2) Thanks Eric.
Comment on attachment 44376 [details] Proposed patch (rev.2) I think I'll land it manually now :-)
landed in r51789. Closing bug now.
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.
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?
(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.
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.
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 on attachment 43968 [details] LayoutTest for SVG pattern crash Marking r- since this was rolled out.
Comment on attachment 44376 [details] Proposed patch (rev.2) Marking r- since this was rolled out.
Fixed in trunk. Closing bug.