| Summary: | Some SVGs with empty <g> elements crash Chromium on Linux | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Cosmin Truta <ctruta> | ||||||||||||||||||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, brettw, commit-queue, ctruta, dglazkov, eric, krit, ojan, rjkroege, simon.fraser, webkit.review.bot, zimmermann | ||||||||||||||||||||
| Priority: | P3 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Cosmin Truta
2010-06-24 14:19:00 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. Created attachment 59703 [details]
SVG test file: filter applied to empty <g>
Created attachment 59912 [details]
This is the half-fix for the chromium.org issue 37986
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 on attachment 59912 [details]
This is the half-fix for the chromium.org issue 37986
Please include the layout test in the patch.
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 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. Created attachment 60990 [details]
This is the half-fix (take 2) for the chromium.org issue 37986
Changed component to SVG, so it shows up in my all-svg-bugs search. 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?
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. 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. 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); 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...] 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 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.
Created attachment 61769 [details]
Revised fix
Comment on attachment 61769 [details]
Revised fix
Thank you.
Comment on attachment 61769 [details] Revised fix Clearing flags on attachment: 61769 Committed r63530: <http://trac.webkit.org/changeset/63530> All reviewed patches have been landed. Closing bug. (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? (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. > 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. This needs to get resolved. 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. 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. 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. 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] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
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 on attachment 62638 [details]
New fix: avoid a regression introduced by the old solution
Hmm... the stylebot is right.
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.
Oops!... forgot to re-run the style checker. Will resubmit shortly. (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. Created attachment 62640 [details]
New fix, style fixed
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> (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 (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. Created attachment 62701 [details]
Expected output for svg/batik/filters/filterRegions.svg
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 on attachment 62640 [details]
New fix, style fixed
OK.
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 Sorry for the extreme delay with the commit-queue (which I'm sure contributed to this failing to apply). 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 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 on attachment 63542 [details] Style fixed, minus new fix Clearing flags on attachment: 63542 Committed r64813: <http://trac.webkit.org/changeset/64813> All reviewed patches have been landed. Closing bug. |