| Summary: | Null dereference loading Blink layout test svg/filters/display-none-filter-primitive.html | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Honeycutt <jhoneycutt> | ||||||
| Component: | SVG | Assignee: | Dean Jackson <dino> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dino, webkit-bug-importer, zimmermann | ||||||
| Priority: | P2 | Keywords: | HasReduction, InRadar | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 263725 [details]
Patch
Comment on attachment 263725 [details]
Patch
r=me!
Committed r191403: <http://trac.webkit.org/changeset/191403> 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. 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. |
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>)