WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
41175
Some SVGs with empty <g> elements crash Chromium on Linux
https://bugs.webkit.org/show_bug.cgi?id=41175
Summary
Some SVGs with empty <g> elements crash Chromium on Linux
Cosmin Truta
Reported
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.
Attachments
SVG test file: filter applied to empty <g>
(466 bytes, image/svg+xml)
2010-06-24 14:33 PDT
,
Cosmin Truta
no flags
Details
This is the half-fix for the chromium.org issue 37986
(1.51 KB, patch)
2010-06-28 11:22 PDT
,
Cosmin Truta
eric
: review-
Details
Formatted Diff
Diff
This is the half-fix (take 2) for the chromium.org issue 37986
(5.14 KB, patch)
2010-07-08 18:00 PDT
,
Cosmin Truta
eric
: commit-queue-
Details
Formatted Diff
Diff
Revised fix
(5.23 KB, patch)
2010-07-15 23:50 PDT
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
New fix: avoid a regression introduced by the old solution
(3.78 KB, patch)
2010-07-26 20:52 PDT
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
New fix, style fixed
(3.77 KB, patch)
2010-07-26 21:22 PDT
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Expected output for svg/batik/filters/filterRegions.svg
(16.64 KB, image/png)
2010-07-27 09:53 PDT
,
Cosmin Truta
no flags
Details
Actual output for svg/batik/filters/filterRegions.svg
(16.65 KB, image/png)
2010-07-27 09:56 PDT
,
Cosmin Truta
no flags
Details
Style fixed, minus new fix
(1.20 KB, patch)
2010-08-04 21:24 PDT
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Cosmin Truta
Comment 1
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.
Cosmin Truta
Comment 2
2010-06-24 14:33:34 PDT
Created
attachment 59703
[details]
SVG test file: filter applied to empty <g>
Cosmin Truta
Comment 3
2010-06-28 11:22:57 PDT
Created
attachment 59912
[details]
This is the half-fix for the chromium.org issue 37986
Eric Seidel (no email)
Comment 4
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. :)
Eric Seidel (no email)
Comment 5
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.
Cosmin Truta
Comment 6
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
Eric Seidel (no email)
Comment 7
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.
Cosmin Truta
Comment 8
2010-07-08 18:00:35 PDT
Created
attachment 60990
[details]
This is the half-fix (take 2) for the chromium.org issue 37986
Nikolas Zimmermann
Comment 9
2010-07-09 07:29:11 PDT
Changed component to SVG, so it shows up in my all-svg-bugs search.
Brett Wilson (Google)
Comment 10
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?
Cosmin Truta
Comment 11
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.
Brett Wilson (Google)
Comment 12
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.
Brett Wilson (Google)
Comment 13
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);
Cosmin Truta
Comment 14
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...]
Brett Wilson (Google)
Comment 15
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.
Eric Seidel (no email)
Comment 16
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.
Cosmin Truta
Comment 17
2010-07-15 23:50:16 PDT
Created
attachment 61769
[details]
Revised fix
Eric Seidel (no email)
Comment 18
2010-07-16 00:04:20 PDT
Comment on
attachment 61769
[details]
Revised fix Thank you.
WebKit Commit Bot
Comment 19
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
>
WebKit Commit Bot
Comment 20
2010-07-16 01:15:58 PDT
All reviewed patches have been landed. Closing bug.
Nikolas Zimmermann
Comment 21
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?
Cosmin Truta
Comment 22
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.
Adam Barth
Comment 23
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.
Simon Fraser (smfr)
Comment 24
2010-07-21 10:44:27 PDT
This needs to get resolved.
Ojan Vafai
Comment 25
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.
Cosmin Truta
Comment 26
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.
Cosmin Truta
Comment 27
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.
WebKit Review Bot
Comment 28
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] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Eric Seidel (no email)
Comment 29
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. :)
Eric Seidel (no email)
Comment 30
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.
Eric Seidel (no email)
Comment 31
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.
Cosmin Truta
Comment 32
2010-07-26 21:01:33 PDT
Oops!... forgot to re-run the style checker. Will resubmit shortly.
Cosmin Truta
Comment 33
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.
Cosmin Truta
Comment 34
2010-07-26 21:22:40 PDT
Created
attachment 62640
[details]
New fix, style fixed
WebKit Commit Bot
Comment 35
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
>
Dirk Schulze
Comment 36
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
Cosmin Truta
Comment 37
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.
Cosmin Truta
Comment 38
2010-07-27 09:53:38 PDT
Created
attachment 62701
[details]
Expected output for svg/batik/filters/filterRegions.svg
Cosmin Truta
Comment 39
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.
Eric Seidel (no email)
Comment 40
2010-08-03 15:00:13 PDT
Comment on
attachment 62640
[details]
New fix, style fixed OK.
WebKit Commit Bot
Comment 41
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
Eric Seidel (no email)
Comment 42
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).
Cosmin Truta
Comment 43
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.
Eric Seidel (no email)
Comment 44
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
.
WebKit Commit Bot
Comment 45
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
>
WebKit Commit Bot
Comment 46
2010-08-05 20:48:14 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug