Bug 90814 – Crash in SVGStopElement::stopColorIncludingOpacity

• Component: SVG
• Status: RESOLVED FIXED

[Stephen Chenney]

A top crasher in Chrome, on pages containing the Meebo bar, has the following stack. The only real potential cause is a null renderer or style. In an effort to get this crash addressed, while waiting for Bug 86941 and 87373 to be fixed, I'll be putting up a patch to catch null pointers in this code.

Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000004 )

0x7b6bc17c	 [Google Chrome Framework]	 - ../../WTF/wtf/RefPtr.h:58]	
WebCore::SVGStopElement::stopColorIncludingOpacity
0x7b6831ac	 [Google Chrome Framework]	 - SVGGradientElement.cpp:150	
WebCore::SVGGradientElement::buildStops
0x7b68cedd	 [Google Chrome Framework]	 - SVGLinearGradientElement.cpp:150	
WebCore::SVGLinearGradientElement::collectGradientAttributes
0x7b6066c6	 [Google Chrome Framework]	 - RenderSVGResourceLinearGradient.cpp:45	
WebCore::RenderSVGResourceLinearGradient::collectGradientAttributes
0x7b6059ad	 [Google Chrome Framework]	 - RenderSVGResourceGradient.cpp:131	
WebCore::RenderSVGResourceGradient::applyResource
0x7b605db0	 [Google Chrome Framework]	 + 0x01e78db0]	
non-virtual thunk to WebCore::RenderSVGResourceGradient::applyResource(WebCore::RenderObject*, WebCore::RenderStyle*, WebCore::GraphicsContext*&, unsigned short)
0x7b60e27b	 [Google Chrome Framework]	 - RenderSVGShape.cpp:254	
WebCore::RenderSVGShape::fillShape
0x7b60e4a0	 [Google Chrome Framework]	 - RenderSVGShape.cpp:291	
WebCore::RenderSVGShape::fillAndStrokePath
0x7b60e857	 [Google Chrome Framework]	 - RenderSVGShape.cpp:339	
WebCore::RenderSVGShape::paint

[Stephen Chenney]

Created attachment 151304 [details]
Patch

[Build Bot]

Comment on attachment 151304 [details]
Patch

Attachment 151304 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13172203

[Build Bot]

Comment on attachment 151304 [details]
Patch

Attachment 151304 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13164353

[Nikolas Zimmermann]

Comment on attachment 151304 [details]
Patch

*grml* this is weird. SVGStopElement::rendererIsNeeded() returns true.
Are there any other cases when a renderer might not be created? Thinking about it, there might be chances that there is code which forbids renderer creation for a whole Document.
Is it possible we're hitting that? I wish this had a testcase, with one I'd immediately r+ it.

[Stephen Chenney]

This is some kind of race condition. It's reliably flakey, generating several crash reports per day for several months, going back several Chrome versions. It's more prominent now because of the usage of Chrome and the prevalence of the Meebo bar at the bottom of high traffic sites. It seems likely that the stop element doesn't have its renderer about 0.001% of the time, but we would rather not have crashes on high profile sites. At the same time, coming up with a test to hit what is clearly a very rare occurrence is something that hardly seems the effort, or maybe not even possible.

I'll poke around a bit more to try to figure out how the stop element can not have a renderer yet be in it's parent's tree, but I'm not real hopeful.

[Stephen Chenney]

Created attachment 152299 [details]
Patch

[Stephen Chenney]

I have looked into the renderer creation code. In normal operation, if the stop element's parent gradient node has a renderer, the stop element should also have a renderer. And the crashing code is only invoked if the parent has a renderer.

I presume the problem is arising with JS creation of the stop element in such a way that checks in NodeRendererFactory::createRendererIfNeeded() prevent the renderer from being created. Or maybe it's a transient situation on element removal. Given the challenges in reproducing this, I think the best approach is to add the null check.

We're planning to remove this code anyway, as soon as CSS animations on non-renderer elements gets through.

[Dirk Schulze]

Comment on attachment 152299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152299&action=review

Since it is still a hack, you should add a FIXME that this is because of crashers and needs to be fixed with a bug number. I would still prefer a real fix. And a test case would be extremely helpful too. Since it is a random crasher at the moment, I still give r+. Please do the changes on the code before landing.

> Source/WebCore/svg/SVGStopElement.cpp:116
> +    RenderStyle* styleToUse = renderer() ? renderer()->style() : 0;

IIRC we usually just call it style, not styleToUse.

> Source/WebCore/svg/SVGStopElement.cpp:118
> +        return Color(0x00000000, true); // Transparent black.

Color(Color::transparent, true) looks better.

[Stephen Chenney]

Committed r122867: <http://trac.webkit.org/changeset/122867>

[Stephen Chenney]

I fixed the code as suggested and committed. In the FIXME I referenced the bug to remove RenderSVGGradientStop, at which point this code will be removed.