Bug 41175 – Some SVGs with empty <g> elements crash Chromium on Linux

• Component: SVG
• Status: RESOLVED FIXED

[Cosmin Truta]

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.

[Cosmin Truta]

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]

Created attachment 59703 [details]
SVG test file: filter applied to empty <g>

[Cosmin Truta]

Created attachment 59912 [details]
This is the half-fix for the chromium.org issue 37986

[Eric Seidel (no email)]

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 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]

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)]

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]

Created attachment 60990 [details]
This is the half-fix (take 2) for the chromium.org issue 37986

[Nikolas Zimmermann]

Changed component to SVG, so it shows up in my all-svg-bugs search.

[Brett Wilson (Google)]

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]

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)]

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)]

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]

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)]

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 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]

Created attachment 61769 [details]
Revised fix

[Eric Seidel (no email)]

Comment on attachment 61769 [details]
Revised fix

Thank you.

[WebKit Commit Bot]

Comment on attachment 61769 [details]
Revised fix

Clearing flags on attachment: 61769

Committed r63530: <http://trac.webkit.org/changeset/63530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

(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]

(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]

> 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)]

This needs to get resolved.

[Ojan Vafai]

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]

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]

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]

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 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 on attachment 62638 [details]
New fix: avoid a regression introduced by the old solution

Hmm... the stylebot is right.

[Eric Seidel (no email)]

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]

Oops!... forgot to re-run the style checker. Will resubmit shortly.

[Cosmin Truta]

(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]

Created attachment 62640 [details]
New fix, style fixed

[WebKit Commit Bot]

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]

(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]

(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]

Created attachment 62701 [details]
Expected output for svg/batik/filters/filterRegions.svg

[Cosmin Truta]

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 on attachment 62640 [details]
New fix, style fixed

OK.

[WebKit Commit Bot]

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)]

Sorry for the extreme delay with the commit-queue (which I'm sure contributed to this failing to apply).

[Cosmin Truta]

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 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 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]

All reviewed patches have been landed.  Closing bug.