|Summary:||Some SVGs with empty <g> elements crash Chromium on Linux|
|Product:||WebKit||Reporter:||Cosmin Truta <ctruta>|
|Severity:||Normal||CC:||abarth, brettw, commit-queue, ctruta, dglazkov, eric, krit, ojan, rjkroege, simon.fraser, webkit.review.bot, zimmermann|
|Version:||528+ (Nightly build)|
Description Cosmin Truta 2010-06-24 14:19:00 PDT
This is remotely related to the Chromium issue #37986 http://code.google.com/p/chromium/issues/detail?id=37986 Chromium on Linux (nightly) is crashing when loading an SVG file that contains empty <g> elements to which filters like feGaussianBlur are applied. A sample SVG file is attached.
Comment 1 Cosmin Truta 2010-06-24 14:27:48 PDT
This issue is happening because skia can't process zero-sized images. A part of the fix (which, in fact, is a workaround) is in Chromium, the other one is in WebKit. The workaround that is to be submitted may or may not be ideal. If the latter turns out to be true, a better fix will follow eventually. A layout test consisting of the currently-attached SVG test case will also follow, after both parts of the fix get integrated, respectively, in Chrome and WebKit.
Comment 2 Cosmin Truta 2010-06-24 14:33:34 PDT
Created attachment 59703 [details] SVG test file: filter applied to empty <g>
Comment 3 Cosmin Truta 2010-06-28 11:22:57 PDT
Created attachment 59912 [details] This is the half-fix for the chromium.org issue 37986
Comment 4 Eric Seidel (no email) 2010-07-02 14:27:07 PDT
I don't understand why the layout test would be separate? Seems like a skia bug, which continues to bite us. This is certainly not the first fix related to this quirk that I've seen go by. :)
Comment 5 Eric Seidel (no email) 2010-07-02 14:27:55 PDT
Comment on attachment 59912 [details] This is the half-fix for the chromium.org issue 37986 Please include the layout test in the patch.
Comment 6 Cosmin Truta 2010-07-07 16:13:48 PDT
A new patch, containing the layout test, as well as a test exclusion for chromium-linux, will come shortly. The exclusion will be removed after both this patch and the chromium patch get integrated. In response to comment #4: if a layout test is included without the other half of the chromium fix, it will be reported as a regression in the Chromium testing. This needs to be avoided. The initial idea was to submit the layout test after the Chromium fix. Since that isn't acceptable, adding an exclusion, which is to be removed after everything else is done, is another possible course of action. I opened the WebKit bug 41808. This will be referenced from LayoutTests/platform/chromium/test_expectations.txt See https://bugs.webkit.org/show_bug.cgi?id=41808
Comment 7 Eric Seidel (no email) 2010-07-07 16:15:31 PDT
I don't know what Chromium's two-sided patch process looks like these days. But I agree adding the new test to the skipped list when landing the fix is better than not landing the test at all.
Comment 8 Cosmin Truta 2010-07-08 18:00:35 PDT
Created attachment 60990 [details] This is the half-fix (take 2) for the chromium.org issue 37986
Comment 9 Nikolas Zimmermann 2010-07-09 07:29:11 PDT
Changed component to SVG, so it shows up in my all-svg-bugs search.
Comment 10 Brett Wilson (Google) 2010-07-09 17:32:51 PDT
I don't understand. When I run the test case, I get this stack for an assertion failure: #0 0x0000000000c46dce in WebCore::getImageData<(WebCore::Multiply)0> (rect=..., bitmap=..., size=...) at third_party/WebKit/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:160 #1 0x0000000000c4588d in WebCore::ImageBuffer::getPremultipliedImageData (this=0x7fffecf12200, rect=...) at third_party/WebKit/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:199 #2 0x000000000102453b in WebCore::FEGaussianBlur::apply (this=0x7fffed3c0460, filter=0x7fffed41a6e0) at third_party/WebKit/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:122 This doesn't look like what you're changing at all. I see an assertion failure later when checking the format of the empty bitmap (which is nothing since it's empty). Can you post the stack of the crash you're seeing and/or explain in more detail what the exact problem is?
Comment 11 Cosmin Truta 2010-07-10 23:56:00 PDT
Brett, you're seeing the same failure that I'm seeing. bitmap.config() is kNo_Config (which is zero), instead of the expected kARGB_8888_Config. That's happening because an invalid skia image object (a memzero'ed object, to be more precise) was flying under the radar for a while, up to this point of failure. Specifically, the occurrence of a filtered empty <g> triggers the creation of a zero-sized bitmap. This works well on Mac, apparently. On Windows, the WinAPI doesn't allow degenerate bitmaps, but that's worked around in skia/ext/bitmap_platform_device_win.cc; see BitmapPlatformDevice::create(). Similarly to what's happening on Windows, the skia core bitmap class doesn't allow zero-sized bitmaps. See SkBitmap::setConfig(). And SkCanvas::createDevice() calls setConfig, but doesn't seem to care about the success of the operation, and here is when the invalid bitmap escapes in the wild. All these get called upon the creation of the bitmap, inside skia/ext/bitmap_platform_device_linux.cc, right at the point where I wrote my Chromium patch. While developing my solution, I was essentially inspired by what's done in skia/ext/bitmap_platform_device_win.cc, and I did the same in bitmap_platform_device_linux.cc. I admit that the accompanying WebKit patch is not ideal, but it's necessary. The surrounding context is not ideal either, and I fully agree with the comment "Can we have another way to manage this?" from the surrounding context. I'm currently looking at a better solution for that as well. But right now, resolving a crash that's happening with a valid SVG image is something that I considered more important, so there's my current solution.
Comment 12 Brett Wilson (Google) 2010-07-13 09:04:20 PDT
I don't understand what this has to do with the call to m_data.m_canvas.drawARGB that you're changing. The crash is an assert happening somewhere else. This draw call looks to me like it works fine. Why isn't the solution to fix getImageData. In fact, it looks like the function would work perfectly if we just removed the assert (normally we don't check the pixel format since we never use any other pixel format). From what I can see, Skia is perfectly happy with empty bitmaps, doesn't crash, and handles them in a consistent way. The problem is in our code that reads pixels out. It's possible the best solution to ensure we don't have empty bitmaps since our code might not handle this in too many cases, but it seems like that's a slightly different question than you're trying to solve here. I could be missing a Skia failure, though.
Comment 13 Brett Wilson (Google) 2010-07-13 09:47:20 PDT
To clarify, can you explain what crash (either with an example or a stack trace) you're fixing with this change: - m_data.m_canvas.drawARGB(0, 0, 0, 0, SkXfermode::kClear_Mode); + if (!size.isZero()) + m_data.m_canvas.drawARGB(0, 0, 0, 0, SkXfermode::kClear_Mode);
Comment 14 Cosmin Truta 2010-07-13 14:30:00 PDT
If I just do the chromium fix, there's another crash in Skia, which, remember, doesn't do zero-sized regions. I wouldn't mind seeing Skia augmented with this capability, though. The ImageBufferSkia workaround consists of not drawing zero-sized regions at all. This is in the constructor WebCore::ImageBuffer::ImageBuffer(), which you can see in the stack trace below. More precisely, this is happening while calling drawARGB(), which is exactly the thing that I'm avoiding in my solution. [16945:16945:527293672669:FATAL:third_party/skia/src/core/SkDraw.cpp(265)] third_party/skia/src/core/SkDraw.cpp:265: failed assertion "pixels" Backtrace: StackTrace::StackTrace() [0x8bf2e6] logging::LogMessage::~LogMessage() [0x86354b] SkDebugf_FileLine() [0x7f2dd1] CallBitmapXferProc() [0x7a3d83] SkDraw::drawPaint() [0x7a3f8d] SkDevice::drawPaint() [0x7a3303] SkCanvas::drawPaint() [0x79e262] SkCanvas::drawARGB() [0x79f444] WebCore::ImageBuffer::ImageBuffer() [0x10478dd] WebCore::ImageBuffer::create() [0x10503fe] WebCore::RenderSVGResourceFilter::applyResource() [0x14d5aba] WebCore::SVGRenderSupport::prepareToRenderSVGContent() [0x14f262d] WebCore::RenderSVGContainer::paint() [0x1566708] [...snip...]
Comment 15 Brett Wilson (Google) 2010-07-13 16:32:45 PDT
I see. I wasn't getting that assertion when running the code, but I see how it can occur, so it seems reasonable to do this patch. For the Chrome-side, you should change Mac to do the same as you changed Linux to do. We don't use Skia for rendering pages on Mac, but we do use it for some things and we want it to be consistent.
Comment 16 Eric Seidel (no email) 2010-07-15 13:39:55 PDT
Comment on attachment 60990 [details] This is the half-fix (take 2) for the chromium.org issue 37986 LayoutTests/ChangeLog:3 + Unreviewed. Please leave this line as it was, so the tools can fix it when they land this for you. WebCore/platform/graphics/skia/ImageBufferSkia.cpp:77 + if (!size.isZero()) Does this check need a comment? When should this size check be removed? It seems strange that we check size here and it's not even used as a parameter in that function.
Comment 18 Eric Seidel (no email) 2010-07-16 00:04:20 PDT
Comment on attachment 61769 [details] Revised fix Thank you.
Comment 19 WebKit Commit Bot 2010-07-16 01:15:52 PDT
Comment on attachment 61769 [details] Revised fix Clearing flags on attachment: 61769 Committed r63530: <http://trac.webkit.org/changeset/63530>
Comment 20 WebKit Commit Bot 2010-07-16 01:15:58 PDT
All reviewed patches have been landed. Closing bug.
Comment 21 Nikolas Zimmermann 2010-07-16 04:17:14 PDT
(In reply to comment #20) > All reviewed patches have been landed. Closing bug. Errm, how did you generate the mac baseline for this test? It crashes on Mac as well :( Can you please investigate?
Comment 22 Cosmin Truta 2010-07-16 12:25:54 PDT
(In reply to comment #21) > Errm, how did you generate the mac baseline for this test? It crashes on Mac as well :( > Can you please investigate? What kind of crash are you getting? An assertion failure, or a layout test failure, or something else? I could not reproduce any error on my Mac machine. In fact, this is Linux-specific, and on Mac everything works fine with and without this patch.
Comment 23 Adam Barth 2010-07-19 02:02:42 PDT
> What kind of crash are you getting? An assertion failure, or a layout test failure, or something else? An ASSERT failure: http://build.webkit.org/waterfall?show=Leopard%20Intel%20Debug%20(Tests) http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r63644%20(17576)/svg/filters/filter-empty-g-stderr.txt > I could not reproduce any error on my Mac machine. In fact, this is Linux-specific, and on Mac everything works fine with and without this patch. Did you try a debug build? Perhaps it only reproduces on Leopard.
Comment 24 Simon Fraser (smfr) 2010-07-21 10:44:27 PDT
This needs to get resolved.
Comment 25 Ojan Vafai 2010-07-21 11:00:26 PDT
r63530 only modifies Skia code. So the Leopard assert is not new, it's just a new test. ctruta is looking into a proper fix for the ASSERT, but in the meantime, we should just skip this test on leopard.
Comment 26 Cosmin Truta 2010-07-21 19:47:51 PDT
Indeed, this is not a regression, but a trigger of an old problem by a new layout test. I made arrangements to have access to a 10.5 machine so that I can debug and fix this problem. Meanwhile, I opened the bug 42802, in which I added an exclusion for the problematic layout test.
Comment 27 Cosmin Truta 2010-07-26 20:52:04 PDT
Created attachment 62638 [details] New fix: avoid a regression introduced by the old solution This also resolves the bug 41808, by removing a no longer needed exclusion. The resolution of the bug 42802 and its corresponding exclusion are still TODO.
Comment 28 WebKit Review Bot 2010-07-26 20:54:36 PDT
Attachment 62638 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/skia/ImageBufferSkia.cpp:134: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero]  Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Eric Seidel (no email) 2010-07-26 20:56:10 PDT
Comment on attachment 62638 [details] New fix: avoid a regression introduced by the old solution Woh. This is *way* cleaner. I guess Brett or others should see this go buy, but this looks much better to me. :)
Comment 30 Eric Seidel (no email) 2010-07-26 20:56:50 PDT
Comment on attachment 62638 [details] New fix: avoid a regression introduced by the old solution Hmm... the stylebot is right.
Comment 31 Eric Seidel (no email) 2010-07-26 20:57:37 PDT
Comment on attachment 62638 [details] New fix: avoid a regression introduced by the old solution I'm also slightly surprised that SkBitmap doesn't have some sort if isValid() function which does this too.
Comment 32 Cosmin Truta 2010-07-26 21:01:33 PDT
Oops!... forgot to re-run the style checker. Will resubmit shortly.
Comment 33 Cosmin Truta 2010-07-26 21:04:40 PDT
(In reply to comment #31) Eric, see my comment 21 in Chromium issue 37986: http://code.google.com/p/chromium/issues/detail?id=37986#c21 One of the things I learned during the long days of developing the fix for this bug is that there is no "invalid" SkBitmap.
Comment 34 Cosmin Truta 2010-07-26 21:22:40 PDT
Created attachment 62640 [details] New fix, style fixed
Comment 35 WebKit Commit Bot 2010-07-26 21:34:25 PDT
Comment on attachment 62638 [details] New fix: avoid a regression introduced by the old solution Clearing flags on attachment: 62638 Committed r64103: <http://trac.webkit.org/changeset/64103>
Comment 36 Dirk Schulze 2010-07-27 00:20:01 PDT
(In reply to comment #35) > (From update of attachment 62638 [details]) > Clearing flags on attachment: 62638 > > Committed r64103: <http://trac.webkit.org/changeset/64103> I dislike just fixing it on skia. It should be fixed globaly. It the <g> is empty, it's strokeRect should be empty too and filtering should stop. So a check check should be added if the bufferRect is empty as well and return earlier: http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGResourceFilter.cpp#L205
Comment 37 Cosmin Truta 2010-07-27 09:46:22 PDT
(In reply to comment #36) > I dislike just fixing it on skia. It should be fixed globaly. It the <g> is empty, it's strokeRect should be empty too and filtering should stop. > So a check check should be added if the bufferRect is empty as well and return earlier: http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGResourceFilter.cpp#L205 Dirk, doing that was my first impulse, at the very beginning. But after reading that filtering empty <g> isn't necessarily expected to produce an empty output, I backed down. I was only imagining that, because I haven't actually found a good SVG counterexample. But I did try your suggestion now, and it does indeed work incorrectly. It is regressing svg/batik/filters/filterRegions.svg, even though that test doesn't even have <g> elements. I will attach the expected and actual rendered output images.
Comment 38 Cosmin Truta 2010-07-27 09:53:38 PDT
Created attachment 62701 [details] Expected output for svg/batik/filters/filterRegions.svg
Comment 39 Cosmin Truta 2010-07-27 09:56:38 PDT
Created attachment 62702 [details] Actual output for svg/batik/filters/filterRegions.svg This is the actual (incorrect) output, produced when exiting early (too early...) upon detecting empty <g> and empty-sized image buffers.
Comment 40 Eric Seidel (no email) 2010-08-03 15:00:13 PDT
Comment on attachment 62640 [details] New fix, style fixed OK.
Comment 41 WebKit Commit Bot 2010-08-03 20:35:02 PDT
Comment on attachment 62640 [details] New fix, style fixed Rejecting patch 62640 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Last 500 characters of output: ded at 1 with fuzz 3. patching file LayoutTests/platform/chromium/test_expectations.txt Hunk #1 FAILED at 2681. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/platform/graphics/skia/ImageBufferSkia.cpp Hunk #1 FAILED at 74. Hunk #2 FAILED at 128. 2 out of 2 hunks FAILED -- saving rejects to file WebCore/platform/graphics/skia/ImageBufferSkia.cpp.rej Full output: http://queues.webkit.org/results/3647254
Comment 42 Eric Seidel (no email) 2010-08-03 20:36:09 PDT
Sorry for the extreme delay with the commit-queue (which I'm sure contributed to this failing to apply).
Comment 43 Cosmin Truta 2010-08-04 21:24:59 PDT
Created attachment 63542 [details] Style fixed, minus new fix In reply to comment #42: It turns out that the patch 62640 could not be applied because the patch 61769 is already in, and all that's needed is the actual stylistic change. The changes from the patch 61769 are still in the commit queue.
Comment 44 Eric Seidel (no email) 2010-08-05 08:55:49 PDT
Comment on attachment 62640 [details] New fix, style fixed Cleared Eric Seidel's review+ from obsolete attachment 62640 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 45 WebKit Commit Bot 2010-08-05 20:48:07 PDT
Comment on attachment 63542 [details] Style fixed, minus new fix Clearing flags on attachment: 63542 Committed r64813: <http://trac.webkit.org/changeset/64813>
Comment 46 WebKit Commit Bot 2010-08-05 20:48:14 PDT
All reviewed patches have been landed. Closing bug.