Bug 90814 - Crash in SVGStopElement::stopColorIncludingOpacity
Summary: Crash in SVGStopElement::stopColorIncludingOpacity
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen Chenney
Depends on:
Reported: 2012-07-09 12:44 PDT by Stephen Chenney
Modified: 2012-07-17 12:19 PDT (History)
1 user (show)

See Also:

Patch (1.76 KB, patch)
2012-07-09 13:00 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2012-07-13 10:46 PDT, Stephen Chenney
krit: review+
krit: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Chenney 2012-07-09 12:44:51 PDT
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.


0x7b6bc17c	 [Google Chrome Framework]	 - ../../WTF/wtf/RefPtr.h:58]	
0x7b6831ac	 [Google Chrome Framework]	 - SVGGradientElement.cpp:150	
0x7b68cedd	 [Google Chrome Framework]	 - SVGLinearGradientElement.cpp:150	
0x7b6066c6	 [Google Chrome Framework]	 - RenderSVGResourceLinearGradient.cpp:45	
0x7b6059ad	 [Google Chrome Framework]	 - RenderSVGResourceGradient.cpp:131	
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	
0x7b60e4a0	 [Google Chrome Framework]	 - RenderSVGShape.cpp:291	
0x7b60e857	 [Google Chrome Framework]	 - RenderSVGShape.cpp:339	
Comment 1 Stephen Chenney 2012-07-09 13:00:04 PDT
Created attachment 151304 [details]
Comment 2 Build Bot 2012-07-09 13:07:00 PDT
Comment on attachment 151304 [details]

Attachment 151304 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13172203
Comment 3 Build Bot 2012-07-09 13:15:03 PDT
Comment on attachment 151304 [details]

Attachment 151304 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13164353
Comment 4 Nikolas Zimmermann 2012-07-10 05:37:39 PDT
Comment on attachment 151304 [details]

*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.
Comment 5 Stephen Chenney 2012-07-10 10:18:57 PDT
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.
Comment 6 Stephen Chenney 2012-07-13 10:46:11 PDT
Created attachment 152299 [details]
Comment 7 Stephen Chenney 2012-07-13 10:51:07 PDT
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.
Comment 8 Dirk Schulze 2012-07-13 16:04:12 PDT
Comment on attachment 152299 [details]

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.
Comment 9 Stephen Chenney 2012-07-17 12:17:44 PDT
Committed r122867: <http://trac.webkit.org/changeset/122867>
Comment 10 Stephen Chenney 2012-07-17 12:19:22 PDT
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.