Bug 150212 – Null dereference loading Blink layout test svg/filters/display-none-filter-primitive.html

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jon Honeycutt]

Created attachment 263229 [details]
crashing test

Null dereference loading Blink layout test svg/filters/display-none-filter-primitive.html.

Stack trace:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGABRT)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000040

VM Regions Near 0x40:
--> 
    __TEXT                 0000000102043000-00000001020dd000 [  616K] r-x/rwx SM=COW  /Users/USER/*

Application Specific Information:
CRASHING TEST: blink-tests-that-are-unknown/svg/filters/display-none-filter-primitive.html
================================================================
==23322==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x00010779b8cd bp 0x7fff5dbb37f0 sp 0x7fff5dbb37f0 T0)
    #0 0x10779b8cc in WTF::Ref<WebCore::RenderStyle>::get() const Ref.h:120
    #1 0x1095c1625 in WebCore::RenderSVGResourceFilter::buildPrimitives(WebCore::SVGFilter&) const RenderSVGResourceFilter.cpp:93
    #2 0x1095c20a9 in WebCore::RenderSVGResourceFilter::applyResource(WebCore::RenderElement&, WebCore::RenderStyle const&, WebCore::GraphicsContext*&, unsigned short) RenderSVGResourceFilter.cpp:137
    #3 0x1095acb58 in WebCore::SVGRenderingContext::prepareToRenderSVGContent(WebCore::RenderElement&, WebCore::PaintInfo&, WebCore::SVGRenderingContext::NeedsGraphicsContextSave) SVGRenderingContext.cpp:191
    #4 0x1095d223e in WebCore::RenderSVGShape::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderSVGShape.cpp:307
    #5 0x1095cea03 in WebCore::RenderSVGRoot::paintReplaced(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderSVGRoot.cpp:284
    #6 0x1095795ff in WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderReplaced.cpp:189
    #7 0x1093e756a in WebCore::RenderElement::paintAsInlineBlock(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderElement.cpp:1213
    #8 0x1085c024d in WebCore::InlineElementBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) InlineElementBox.cpp:91
    #9 0x1085c91b0 in WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) InlineFlowBox.cpp:1213
    #10 0x10971a8ab in WebCore::RootInlineBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) RootInlineBox.cpp:186
    #11 0x1094f51a6 in WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, WebCore::LayoutPoint const&) const RenderLineBoxList.cpp:271
    #12 0x1092e8e57 in WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1435
    #13 0x1092ea21c in WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1584
    #14 0x1092e8b57 in WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1415
    #15 0x1092e95c3 in WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool, WebCore::RenderBlock::PaintBlockType) RenderBlock.cpp:1488
    #16 0x1092e905e in WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool) RenderBlock.cpp:1455
    #17 0x1092e8f4a in WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1448
    #18 0x1092ea21c in WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1584
    #19 0x1092e8b57 in WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1415
    #20 0x1092e95c3 in WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool, WebCore::RenderBlock::PaintBlockType) RenderBlock.cpp:1488
    #21 0x1092e905e in WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool) RenderBlock.cpp:1455
    #22 0x1092e8f4a in WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1448
    #23 0x1092ea21c in WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1584
    #24 0x1092e8b57 in WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) RenderBlock.cpp:1415
    #25 0x10949cba1 in WebCore::RenderLayer::paintForegroundForFragmentsWithPhase(WebCore::PaintPhase, WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*) RenderLayer.cpp:4738
    #26 0x109498ca7 in WebCore::RenderLayer::paintForegroundForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::GraphicsContext&, WebCore::GraphicsContext&, WebCore::LayoutRect const&, bool, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*, bool) RenderLayer.cpp:4703
    #27 0x10949361f in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) RenderLayer.cpp:4330
    #28 0x109490d94 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) RenderLayer.cpp:3969
    #29 0x10949895f in WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow, 16ul>*, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) RenderLayer.cpp:4436
    #30 0x1094936df in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) RenderLayer.cpp:4342
    #31 0x109490d94 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) RenderLayer.cpp:3969
    #32 0x1094908fa in WebCore::RenderLayer::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&, WebCore::LayoutSize const&, unsigned int, WebCore::RenderObject*, unsigned int) RenderLayer.cpp:3774
    #33 0x108260bfe in WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&) FrameView.cpp:4051
    #34 0x10eb2f675 in -[WebFrame(WebInternal) _drawRect:contentsOnly:] WebFrame.mm:647
    #35 0x10eb8ed7a in -[WebHTMLView drawSingleRect:] WebHTMLView.mm:3502
    #36 0x10eb8f461 in -[WebHTMLView drawRect:] WebHTMLView.mm:3575
    #37 0x7fff8d648bb9 in -[NSView _drawRect:clip:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x1abbb9)
    #38 0x7fff8d6a1378 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204378)
    #39 0x10eb82a44 in -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:] WebHTMLView.mm:1467
    #40 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #41 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #42 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #43 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #44 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #45 0x7fff8d6a1755 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x204755)
    #46 0x7fff8d6462e2 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x1a92e2)
    #47 0x7fff8d6a0b0b in -[NSNextStepFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x203b0b)
    #48 0x7fff8d643ed2 in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x1a6ed2)
    #49 0x7fff8d63f2e0 in -[NSView displayIfNeeded] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x1a22e0)
    #50 0x1020675d5 in dump() DumpRenderTree.mm:1697
    #51 0x1020a2292 in TestRunner::notifyDone() TestRunnerMac.mm:273
    #52 0x7fff96fa2c83 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x92c83)
    #53 0x7fff96fa2912 in __CFRunLoopDoTimer (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x92912)
    #54 0x7fff96fa2469 in __CFRunLoopDoTimers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x92469)
    #55 0x7fff96f99960 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x89960)
    #56 0x7fff96f98fc7 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x88fc7)
    #57 0x10206598d in runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:2030
    #58 0x102064f39 in runTestingServerLoop() DumpRenderTree.mm:1180
    #59 0x102064267 in dumpRenderTree(int, char const**) DumpRenderTree.mm:1288
    #60 0x1020662b1 in DumpRenderTreeMain(int, char const**) DumpRenderTree.mm:1418
    #61 0x7fff931e95ac in start (/usr/lib/system/libdyld.dylib+0x35ac)
    #62 0x1  (<unknown module>)

[Radar WebKit Bug Importer]

<rdar://problem/23137376>

[Dean Jackson]

Created attachment 263725 [details]
Patch

[Brent Fulgham]

Comment on attachment 263725 [details]
Patch

r=me!

[Dean Jackson]

Committed r191403: <http://trac.webkit.org/changeset/191403>

[Said Abou-Hallawa]

Comment on attachment 263725 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:94
> +            effect->setOperatingColorSpace(element.renderer()->style().svgStyle().colorInterpolationFilters() == CI_LINEARRGB ? ColorSpaceLinearRGB : ColorSpaceDeviceRGB);

Why do we have to handle every reference to the renderer and check if it's null or not? If we have a filter like that <feMerge style="display: none">, I think the whole filter has to be ignored as if it did not exist. Why do not we change the loop in RenderSVGResourceFilter::buildPrimitives() to be like this:

    for (auto& element : childrenOfType<SVGFilterPrimitiveStandardAttributes>(filterElement())) {
        RefPtr<FilterEffect> effect = element.build(builder.get(), filter);
        if (!effect) {
            builder->clearEffects();
            return nullptr;
        }
        if (!element.renderer())
            continue;
        builder->appendEffectToEffectReferences(effect.copyRef(), element.renderer());
        element.setStandardAttributes(effect.get());
        effect->setEffectBoundaries(SVGLengthContext::resolveRectangle<SVGFilterPrimitiveStandardAttributes>(&element, filterElement().primitiveUnits(), targetBoundingBox));
        effect->setOperatingColorSpace(element.renderer()->style().svgStyle().colorInterpolationFilters() == CI_LINEARRGB ? ColorSpaceLinearRGB : ColorSpaceDeviceRGB);
        builder->add(element.result(), WTF::move(effect));
    }

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:69
> +    ASSERT(!object || !m_effectRenderer.contains(object));

This new assertion looks weird to me. Why do we even come to this function if the renderer is null?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:80
> +    // work (the LayoutObject -> FilterEffect mapping will not be defined).

I think this comment is misleading. Reading "...isn't attached for some reason" made me think there is a bug somewhere else and we could not figure it out. But only when I looked at the test case in this patch I understood why the renderer was not created. Also I think we should rename object => renderer.

[Said Abou-Hallawa]

Comment on attachment 263725 [details]
Patch

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

>> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:94
>> +            effect->setOperatingColorSpace(element.renderer()->style().svgStyle().colorInterpolationFilters() == CI_LINEARRGB ? ColorSpaceLinearRGB : ColorSpaceDeviceRGB);
> 
> Why do we have to handle every reference to the renderer and check if it's null or not? If we have a filter like that <feMerge style="display: none">, I think the whole filter has to be ignored as if it did not exist. Why do not we change the loop in RenderSVGResourceFilter::buildPrimitives() to be like this:
> 
>     for (auto& element : childrenOfType<SVGFilterPrimitiveStandardAttributes>(filterElement())) {
>         RefPtr<FilterEffect> effect = element.build(builder.get(), filter);
>         if (!effect) {
>             builder->clearEffects();
>             return nullptr;
>         }
>         if (!element.renderer())
>             continue;
>         builder->appendEffectToEffectReferences(effect.copyRef(), element.renderer());
>         element.setStandardAttributes(effect.get());
>         effect->setEffectBoundaries(SVGLengthContext::resolveRectangle<SVGFilterPrimitiveStandardAttributes>(&element, filterElement().primitiveUnits(), targetBoundingBox));
>         effect->setOperatingColorSpace(element.renderer()->style().svgStyle().colorInterpolationFilters() == CI_LINEARRGB ? ColorSpaceLinearRGB : ColorSpaceDeviceRGB);
>         builder->add(element.result(), WTF::move(effect));
>     }

Sorry I meant:

If we have a filterEffect like that <feMerge style="display: none">, I think the whole filterEffect has to be ignored as if it did not exist.