Bug 87964 – Lonely stop crashes

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

Created attachment 145071 [details]
Repro case

The following will crash:
<svg xmlns="http://www.w3.org/2000/svg">
  <stop/>
</svg>

Stacktrace:
Program received signal SIGSEGV, Segmentation fault.
0x00007fa9362a101a in WebCore::RenderObject::nodeAtFloatPoint (this=0x7fa9313e0cd8) at ../../third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2966
2966	    ASSERT_NOT_REACHED();
(gdb) bt
#0  0x00007fa9362a101a in WebCore::RenderObject::nodeAtFloatPoint (this=0x7fa9313e0cd8) at ../../third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2966
#1  0x00007fa9348799cc in WebCore::RenderSVGRoot::nodeAtPoint (this=0x7fa92463c198, request=..., result=..., pointInContainer=..., accumulatedOffset=..., hitTestAction=
    WebCore::HitTestForeground) at ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGRoot.cpp:429
#2  0x00007fa93629f471 in WebCore::RenderObject::hitTest (this=0x7fa92463c198, request=..., result=..., pointInContainer=..., accumulatedOffset=..., hitTestFilter=
    WebCore::HitTestDescendants) at ../../third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2465
#3  0x00007fa9362526a8 in WebCore::RenderLayer::hitTestContents (this=0x7fa92462f018, request=..., result=..., layerBounds=..., hitTestPoint=..., hitTestFilter=
    WebCore::HitTestDescendants) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3596
#4  0x00007fa93625225c in WebCore::RenderLayer::hitTestLayer (this=0x7fa92462f018, rootLayer=0x7fa92462fb58, containerLayer=0x7fa92462fb58, request=..., result=..., 
    hitTestRect=..., hitTestPoint=..., appliedTransform=false, transformState=0x0, zOffset=0x0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3551
#5  0x00007fa93625297d in WebCore::RenderLayer::hitTestList (this=0x7fa92462fb58, list=0x7fa92463ede0, rootLayer=0x7fa92462fb58, request=..., result=..., hitTestRect=..., 
    hitTestPoint=..., transformState=0x0, zOffsetForDescendants=0x0, zOffset=0x0, unflattenedTransformState=0x0, depthSortDescendants=false)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3639
#6  0x00007fa9362520bd in WebCore::RenderLayer::hitTestLayer (this=0x7fa92462fb58, rootLayer=0x7fa92462fb58, containerLayer=0x0, request=..., result=..., hitTestRect=..., 
    hitTestPoint=..., appliedTransform=false, transformState=0x0, zOffset=0x0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3531
#7  0x00007fa936251382 in WebCore::RenderLayer::hitTest (this=0x7fa92462fb58, request=..., result=...)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3313
#8  0x00007fa9362fc397 in WebCore::RenderView::hitTest (this=0x7fa931329498, request=..., result=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderView.cpp:83
#9  0x00007fa9349e12e1 in WebCore::Document::prepareMouseEvent (this=0x7fa924611000, request=..., documentPoint=..., event=...)
    at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:3081
#10 0x00007fa9345e77ac in WebCore::EventHandler::prepareMouseEvent (this=0x7fa92457ea88, request=..., mev=...)
    at ../../third_party/WebKit/Source/WebCore/page/EventHandler.cpp:2090
#11 0x00007fa9345e5f29 in WebCore::EventHandler::handleMouseMoveEvent (this=0x7fa92457ea88, mouseEvent=..., hoveredNode=0x7fff3f091cf0, onlyUpdateScrollbars=false)
    at ../../third_party/WebKit/Source/WebCore/page/EventHandler.cpp:1769
#12 0x00007fa9345e5a8c in WebCore::EventHandler::mouseMoved (this=0x7fa92457ea88, event=...) at ../../third_party/WebKit/Source/WebCore/page/EventHandler.cpp:1691
#13 0x00007fa93368c465 in WebKit::PageWidgetEventHandler::handleMouseMove (this=0x7fa93130ee38, mainFrame=..., event=...)
    at ../../third_party/WebKit/Source/WebKit/chromium/src/PageWidgetDelegate.cpp:193
#14 0x00007fa93368c1cd in WebKit::PageWidgetDelegate::handleInputEvent (page=0x7fa931397200, handler=..., event=...)
    at ../../third_party/WebKit/Source/WebKit/chromium/src/PageWidgetDelegate.cpp:116
#15 0x00007fa933643c85 in WebKit::WebViewImpl::handleInputEvent (this=0x7fa93130ee00, inputEvent=...)

[Rob Buis]

Hi Philip,

Have you tried with Niko's change to not create a renderer for stops?

[Philip Rogers]

(In reply to comment #1)
> Hi Philip,
> 
> Have you tried with Niko's change to not create a renderer for stops?

I have now and it does fix it. I'll mark this as depending on wkbug.com/86941

[Stephen Chenney]

This is also a top crasher in Chrome, on pages containing the Meebo bar, that has the following stack. The only real potential cause is a null renderer or null style. I will post a fix in an effort to get this crash addressed, while waiting for Bug 86941 and 87373 to be fixed.

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]

Argh, this is an entirely different issue to the Chrome crasher. So forget I ever mentioned it.

It's worth pointing out that a <stop/> inside an SVG like this is illegal according to the SVG content model.

[Rob Buis]

Created attachment 209678 [details]
Patch

[Philip Rogers]

(In reply to comment #5)
> Created an attachment (id=209678) [details]
> Patch

I think this fix is too local and may hide real security bugs in the future. RenderSVGGradientStop is one of the few SVG renderers to inherit from RenderObject instead of RenderBlock or RenderSVGBoxModelObject, and it's missing the overrides we have in those "normal" base classes. As an example, we don't appear to do the proper SVGResourcesCache::clientDestroyed call in willBeDestroyed().

Niko tried and failed to remove RenderSVGGradientStop. Reviving that fix is one solution; making RenderSVGGradientStop inherit from RenderSVGBoxModelObject is another.

[Darin Adler]

Comment on attachment 209678 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGGradientStop.h:50
> +    virtual bool nodeAtFloatPoint(const HitTestRequest&, HitTestResult&, const FloatPoint&, HitTestAction) OVERRIDE FINAL { return false; }

No need for FINAL on the function when it’s in a FINAL class.

[Darin Adler]

Seems fine to revert this and do other testing to find the “real fix” too, but seems no harm to doing this.

[Rob Buis]

(In reply to comment #8)
> Seems fine to revert this and do other testing to find the “real fix” too, but seems no harm to doing this.

I talked with Philip on IRC. As was noted this fixes the problem, though long-term the current situation is not ideal. Removing RenderSVGGradientStop sounds like the ideal solution, but it is AFAIK still blocked by problems with interaction with CSS transitions. We'll probably have to decide between removal of RenderSVGGradientStop or inheriting from RenderSVGBoxModelObject (adds some bloat though) soon, for now I'll commit this and close the bug so whatever we choose this problem is already fixed and we have the regression test to check that it remains fixed.

[Rob Buis]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=209678) [details] [details]
> > Patch
> 
> I think this fix is too local and may hide real security bugs in the future. RenderSVGGradientStop is one of the few SVG renderers to inherit from RenderObject instead of RenderBlock or RenderSVGBoxModelObject, and it's missing the overrides we have in those "normal" base classes. As an example, we don't appear to do the proper SVGResourcesCache::clientDestroyed call in willBeDestroyed().

That may be the wrong example ; AFAIK stops are not added to SVGResourcesCache, so there is no need to call SVGResourcesCache::clientDestroyed for them.

[Rob Buis]

Committed r154644: <http://trac.webkit.org/changeset/154644>