RESOLVED FIXED 149613
A crash reproducible in Path::isEmpty() under RenderSVGShape::paint() 
https://bugs.webkit.org/show_bug.cgi?id=149613
Summary A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
Neil
Reported 2015-09-28 22:55:51 PDT
Created attachment 262042 [details] Minimal test case showing crash. Open the attached file. Click the green block. Then click the red block. Webkit (nightly) and Safari (8.0.8) crash. Firefox and Chrome do not crash. The attached file is a minimal test case. Which is pretty hard to believe, given how many steps are required to trigger this elusive crash. This bug was spotted in Google's Blockly and isolated from that codebase.
Attachments
Minimal test case showing crash. (3.31 KB, text/html)
2015-09-28 22:55 PDT, Neil 		no flags
Details
Simpler test case (1.09 KB, text/html)
2016-01-13 17:48 PST, Said Abou-Hallawa 		no flags
Details
Patch (1.56 KB, patch)
2016-01-13 17:50 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (11.57 KB, patch)
2016-01-15 20:39 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.01 MB, application/zip)
2016-01-15 21:15 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews102 for mac-yosemite (746.77 KB, application/zip)
2016-01-15 21:28 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews116 for mac-yosemite (782.08 KB, application/zip)
2016-01-15 21:32 PST, Build Bot 		no flags
Details
Test case not working in Blink (549 bytes, text/html)
2016-01-20 13:11 PST, Said Abou-Hallawa 		no flags
Details
Another test case not working in Blink (911 bytes, image/svg+xml)
2016-01-20 13:14 PST, Said Abou-Hallawa 		no flags
Details
Patch (18.92 KB, patch)
2016-01-20 15:05 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (19.40 KB, patch)
2016-01-20 15:30 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (21.57 KB, patch)
2016-01-20 18:08 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2015-09-28 23:36:25 PDT
For Apple employees: see also rdar://problem/14485328 (but the stack trace is different).
Radar WebKit Bug Importer
Comment 2 2015-09-28 23:36:39 PDT
<rdar://problem/22892801>
Said Abou-Hallawa
Comment 3 2016-01-13 17:48:41 PST
Created attachment 268925 [details] Simpler test case This test case shows two red rectangles. After 50 ms, the first rectangle should change to green and after another 50 ms, the second rectangle should change to green. And of course, there should not be any crash.
Said Abou-Hallawa
Comment 4 2016-01-13 17:50:32 PST
Created attachment 268926 [details] Patch
Said Abou-Hallawa
Comment 5 2016-01-15 12:49:32 PST
Consider the following document: <!DOCTYPE html> <html> <head> <style> .dummy { } .hide-second-path > .second-path { display: none; } </style> </head> <body> <svg style="background-color: #fff"> <g id="parent-group" class="hide-second-path"> <path id="id1" d="m 0,0 H 40 V 25 H 0 z" fill="red"/> <path id="id2" d="m 0,0 H 40 V 25 H 0 z" fill="green" class="second-path"/> <g id="child-group" filter="url(#nop-filter)"> <path id="id3" d="m 0,50 H 40 V 25 H 0 z" fill="green"/> <path id="id4" d="m 0,50 H 40 V 25 H 0 z" fill="red" class="second-path"/> </g> </g> </svg> <svg> <filter id="nop-filter"> <feOffset dx="0" dy="0"/> </filter> </svg> <script> setTimeout(function() { document.styleSheets[0].deleteRule(0); setTimeout(function() { document.getElementById('parent-group').removeAttribute('class'); document.getElementById('child-group').setAttribute('class', 'hide-second-path'); }, 50); }, 50); </script> </body> </html> Here is the explanation of this bug: Before running any script, we have the following render tree: RenderBlockFlow1 { RenderSVGRoot1 { RenderSVGTransformableContainer1 { RenderSVGPath1 RenderSVGPath2 RenderSVGTransformableContainer2 { RenderSVGPath3 RenderSVGPath4 } } } RenderSVGRoot2 { RenderSVGResourceFilter } } JS statment-1: - When deleting the first css rule, all above hierarchy is invalidated. - The filter is referenced by RenderSVGTransformableContainer2 - RenderSVGTransformableContainer2.setNeedsLayout() is called by from RenderSVGRoot2.layout() via other calls. - RenderSVGTransformableContainer2.markContainingBlocksForLayout() calls RenderSVGTransformableContainer1.setNormalChildNeedsLayoutBit(true) is called RenderSVGRoot1.setNormalChildNeedsLayoutBit(true) is called It also breaks on ancestor = RenderSVGRoot1 because RenderSVGRoot1.objectIsRelayoutBoundary() is true. - scheduleRelayoutForSubtree(RenderSVGRoot1) calls FrameView::scheduleRelayoutOfSubtree(RenderSVGRoot1) It assigns m_layoutRoot = RenderSVGRoot1 and calls FrameView::convertSubtreeLayoutToFullLayout() - FrameView::convertSubtreeLayoutToFullLayout() calls RenderSVGRoot1.markContainingBlocksForLayout() It returns without calling FrameView::scheduleRelayoutForSubtree() because the ancestor RenderBlockFlow1 has RenderBlockFlow1.normalChildNeedsLayout() true. This is because RenderBlockFlow1 has not finished its layout yet. So end up leaking RenderSVGTransformableContainer1.normalChildNeedsLayoutBit() = true This will prevent the layout for this renderer to happen in the next layout JS statment-2: - The class attribute of RenderSVGTransformableContainer1 is deleted. - Style::TreeResolver creates a new renderer for RenderSVGPath2 - When adding the new renderer to the tree, RenderSVGTransformableContainer1.addChild() is called - Then RenderSVGPath1.setNeedLayout() is called which calls RenderSVGPath1.markContainingBlocksForLayout() - But RenderSVGPath1.markContainingBlocksForLayout() fails to schedule a layout because RenderSVGTransformableContainer1.normalChildNeedsLayoutBit() is true from the previous invalidation.
Said Abou-Hallawa
Comment 6 2016-01-15 13:16:24 PST
This looks a regression from http://trac.webkit.org/changeset/103539.
Said Abou-Hallawa
Comment 7 2016-01-15 15:13:04 PST
This block in RenderSVGRoot::layout() is very strange: if (!m_resourcesNeedingToInvalidateClients.isEmpty()) { // Invalidate resource clients, which may mark some nodes for layout. for (auto& resource : m_resourcesNeedingToInvalidateClients) { resource->removeAllClientsFromCache(); SVGResourcesCache::clientStyleChanged(*resource, StyleDifferenceLayout, resource->style()); } m_isLayoutSizeChanged = false; SVGRenderSupport::layoutChildren(*this, false); } We realize that some resources are referenced by other clients, we want to make sure that the clients are setNeedsLayout(). Then we call SVGRenderSupport::layoutChildren() one more time assuming that the clients of the resources are all contained in the current SVG root. This is not true in the test case. The client of the resource is in a different SVG root.
Said Abou-Hallawa
Comment 8 2016-01-15 20:39:19 PST
Created attachment 269145 [details] Patch
Build Bot
Comment 9 2016-01-15 21:15:31 PST
Comment on attachment 269145 [details] Patch Attachment 269145 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/697030 New failing tests: svg/custom/unicode-in-tspan-multi-svg-crash.html
Build Bot
Comment 10 2016-01-15 21:15:34 PST
Created attachment 269147 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 11 2016-01-15 21:28:18 PST
Comment on attachment 269145 [details] Patch Attachment 269145 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/697078 New failing tests: svg/custom/unicode-in-tspan-multi-svg-crash.html
Build Bot
Comment 12 2016-01-15 21:28:22 PST
Created attachment 269148 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 13 2016-01-15 21:32:06 PST
Comment on attachment 269145 [details] Patch Attachment 269145 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/697062 New failing tests: svg/custom/unicode-in-tspan-multi-svg-crash.html
Build Bot
Comment 14 2016-01-15 21:32:10 PST
Created attachment 269149 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Said Abou-Hallawa
Comment 15 2016-01-18 18:40:43 PST
It seems the failed test svg/custom/unicode-in-tspan-multi-svg-crash.html has the wrong expectation. This test was originally ported from Blink http://trac.webkit.org/changeset/166420/trunk/LayoutTests/ChangeLog. But the expectation of this test was changed in Blink: https://src.chromium.org/viewvc/blink?revision=158480&view=revision. The Blink change seems interesting. It looks like changing the SVG resources layout order should fix of this bug cleanly. I was puzzled when I did not see in Blink the RenderSVGRoot re-layout logic added in http://trac.webkit.org/changeset/111601. But now I understand why it was removed. Although the new results of the failed test is the correct ones. It is worthy investigating the change from Blink.
Darin Adler
Comment 16 2016-01-18 18:51:45 PST
Comment on attachment 269145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269145&action=review > Source/WebCore/rendering/svg/RenderSVGResourceContainer.h:71 > private: > friend class SVGResourcesCache; > + friend class RenderSVGRoot; > void addClient(RenderElement&); > void removeClient(RenderElement&); > + const HashSet<RenderElement*>& clients() const { return m_clients; } > > private: Why two private sections? We don’t need to repeat “private”. Not sure it’s better to have these be private and use friend; probably OK to make the clients function be public. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:458 > + auto& svgRoot = SVGRenderSupport::findTreeRootObject(*resource); > + svgRoot.m_resourcesNeedingToInvalidateClients.add(resource); I think this would read better without the local variable. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:463 > + bool containSelf = true; This function always returns true, because we initialize this to true and then set it to true. How can we construct a test that demonstrates we aren’t making this mistake? I suggest the name containsSelf rather than containSelf. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:465 > + for (auto& resource : m_resourcesNeedingToInvalidateClients) { Two spaces here after the ":" but it should just be one. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:467 > + auto& clients = resource->clients(); > + for (auto& client : clients) { I think this would read better without a local variable. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:468 > + auto& svgRoot = SVGRenderSupport::findTreeRootObject(*client); I suggest naming the local variable “root” rather than “svgRoot”. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:488 > + for (auto* svgRoot : resourcesSVGRoots) > + svgRoot->layout(); I suggest naming the local variable “root” rather than “svgRoot”. > Source/WebCore/rendering/svg/RenderSVGRoot.h:65 > + bool invalidateResourcesClients(HashSet<RenderSVGRoot*>&); This function should be private rather than public. Probably needs a comment explaining the boolean return value. It’s not clear what it means. > Source/WebCore/rendering/svg/RenderSVGRoot.h:66 > + static void updateResourcesClients(const HashSet<RenderSVGRoot*>&); This function should be private rather than public. > Source/WebCore/rendering/svg/SVGRenderSupport.h:94 > // FIXME: These methods do not belong here. Mysterious comment. Might be nice to make this clearer and also not to use the word “methods” for C++ member functions.
Darin Adler
Comment 17 2016-01-18 18:52:28 PST
I understand that you are also contemplating a different fix, inspired by the change in Blink.
zalan
Comment 18 2016-01-18 19:06:17 PST
Comment on attachment 269145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269145&action=review > Source/WebCore/ChangeLog:13 > + do that by scheduling another layout pass. We should mark the clients and their > + ancestors till their RenderSVGRoots to be dirty. Then directly run the layout > + for all of the clients RenderSVGRoots. What's the problem with marking the other roots dirty and having them laid out after scheduling a layout? This patch might collect renderers from _different_ formatting contexts and run layout on them directly. Is that okay to do?
Said Abou-Hallawa
Comment 19 2016-01-19 13:43:13 PST
(In reply to comment #18) > Comment on attachment 269145 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269145&action=review > > > Source/WebCore/ChangeLog:13 > > + do that by scheduling another layout pass. We should mark the clients and their > > + ancestors till their RenderSVGRoots to be dirty. Then directly run the layout > > + for all of the clients RenderSVGRoots. > > What's the problem with marking the other roots dirty and having them laid > out after scheduling a layout? This patch might collect renderers from > _different_ formatting contexts and run layout on them directly. Is that > okay to do? I think scheduling a layout from a layout can lead to infinite layout loop in some cases. For example if we have two SVG's; each has a resource and another element which references the other resource in the other SVG, the layout will not stabilize. The second SVG will mark the first SVG dirty and schedule a new layout. And the first SVG will mark the second SVG dirty and schedule a new layout and so on. I think Blink fix is the right approach. We have to make sure all the layout has been run for all the resources referenced by an element before running its layout. This should guarantee no layout cycles will happen and there will no be dirty renderer left till render time, like the test case of this bug. I am not sure if changing the layout order in this case is problematic or not. But the current layout traversal assumes the elements are arranged in a tree while for SVG it is actually DAG. Trying to traverse it in a tree order seems not converging to a clean solution. If we have a better solution please let me know.
Said Abou-Hallawa
Comment 20 2016-01-20 13:11:25 PST
Created attachment 269377 [details] Test case not working in Blink
Said Abou-Hallawa
Comment 21 2016-01-20 13:14:20 PST
Created attachment 269378 [details] Another test case not working in Blink
Said Abou-Hallawa
Comment 22 2016-01-20 14:18:56 PST
The Blink change https://src.chromium.org/viewvc/blink?revision=158480&view=revision broke the test cases "Test case not working in Blink" and "Another test case not working in Blink" in Chrome 47.0.2526.11. I think assuming the resources can be laid out without the context of their RenderSVGRoot is not always correct. It is not going to work correctly in all cases especially with the relative sizes. So here is what I think should fix this bug. It is not ideal but it's the simplest change I could have come up with. -- We have to keep RenderSVGRoot re-layout logic added in http://trac.webkit.org/changeset/111601 with minor modifications. Once the children of an RenderSVGRoot are laid out, the size of RenderSVGRoot itself might change. So some resources's sizes might change as well. And all the clients of these changed resources "only within this RenderSVGRoot" have to invalidated and laid out one more time. This fixes the issue of resolving the relative sizes within the same RenderSVGRoot (i.e. the resources and their clients both belong to the current RenderSVGRoot). And the restriction "only within this RenderSVGRoot" fixes the crash of this bug. ** It is not guarantee that this two layout passes give the correct sizes at end; we might need to do more than two passes to resolve all the dependencies correctly. But I think it is okay to keep it this way. First we do not want how many passes we actually need. Second it is very rare to need more than two passes and the risk is we may go to infinite layout loop. -- Inspired by the Blink change, we need to run the layout for all the resources which referenced by an SVG renderer but belong to "different RenderSVGRoots" before running the layout of this SVG renderer. -- When the RenderSVGResourceContainer is asked to markAllClientsForInvalidation from the RenderSVGRoot re-layout pass, we have to make sure we are not marking any node outside the current RenderSVGRoot for invalidation. The assumption is the clients of this resource either had already run the layout for the same resource before running the layout for themselves outside the context of the RenderSVGRoot in which case, these clients might get the wrong size. Or they are going to use the correct layout data since their layout will happen later. ** This does not guarantee the correctness of the layout in some cases. But I think also it is okay to have it this way. The reason is these cases are very narrow cases. ++ The layout should have been run before. The problem happens because of layout optimization. ++ A RenderSVGRoot with a resource which has relative size is invalidated such that the size of RenderSVGRoot is changing. ++ A client of this resource under a different RenderSVGRoot is invalidated. ++ The RenderSVGRoot of the resource's client comes before the RenderSVGRoot of resource in the render tree. I think the ideal solution is to find the best order for laying out the RenderSVGRoots. It seems we need kind of topological sort for the all the RenderSVGRoots which should run at the RenderView::layout() level. It is troublesome to try finding the correct order from within the RenderSVGRoot. I think also it might be beneficial if the w3c SVG group addresses this issue and describe a mechanism for figuring out the best layout order for SVG roots.
Said Abou-Hallawa
Comment 23 2016-01-20 15:05:18 PST
Created attachment 269392 [details] Patch
Said Abou-Hallawa
Comment 24 2016-01-20 15:30:32 PST
Created attachment 269396 [details] Patch
Darin Adler
Comment 25 2016-01-20 16:37:15 PST
Comment on attachment 269396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269396&action=review Shouldn’t you also add the two test cases that were the "not working with Blink's solution" ones? > Source/WebCore/rendering/svg/RenderSVGShape.cpp:93 > + ASSERT(hasPath()); > + return !hasPath() || path().isEmpty(); I think this needs a comment. It’s unclear why we need both the assertion and the real check, but a comment could make it clear. > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:235 > + auto* root = SVGRenderSupport::findTreeRootObject(renderer); This should go inside the if statement. No point in doing this work if resources is null. > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:237 > + auto* resources = SVGResourcesCache::cachedResourcesForRenderer(renderer); > + if (resources) Should put the definition inside the if: if (auto* resources = ... Or use early exit.
Said Abou-Hallawa
Comment 26 2016-01-20 18:08:46 PST
Created attachment 269410 [details] Patch
Said Abou-Hallawa
Comment 27 2016-01-20 18:10:52 PST
Comment on attachment 269396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269396&action=review >> Source/WebCore/rendering/svg/RenderSVGShape.cpp:93 >> + return !hasPath() || path().isEmpty(); > > I think this needs a comment. It’s unclear why we need both the assertion and the real check, but a comment could make it clear. Done. >> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:235 >> + auto* root = SVGRenderSupport::findTreeRootObject(renderer); > > This should go inside the if statement. No point in doing this work if resources is null. Done. >> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:237 >> + if (resources) > > Should put the definition inside the if: > > if (auto* resources = ... > > Or use early exit. Done. The assignment was put inside the if.
Said Abou-Hallawa
Comment 28 2016-01-20 18:12:54 PST
I added the "Test case not working in Blink" only to the new patch. The second test already exits in LayoutTests/imported/mozilla/svg/dynamic-pattern-02.svg.
WebKit Commit Bot
Comment 29 2016-01-21 09:50:37 PST
Comment on attachment 269410 [details] Patch Clearing flags on attachment: 269410 Committed r195411: <http://trac.webkit.org/changeset/195411>
WebKit Commit Bot
Comment 30 2016-01-21 09:50:43 PST
All reviewed patches have been landed. Closing bug.
Bryan Housel
Comment 31 2016-03-23 10:53:42 PDT
Hi Webkit team I'm still hitting this bug in 11601 (11601.5.17.1) Build Info: WebKit2-7601005017001000~1 Can we reopen this issue? I'm the project maintainer for the OpenStreetMap iD map editor. We're tracking this issue here: https://github.com/openstreetmap/iD/issues/2913 I'll try to put together a more minimal test case, but for now I'm testing this link: http://sonny.7thposition.com/osm/iD/#map=17.76/18.53595/4.33465 I've found that visiting the link and interacting with the map seems to cause crashes in Safari 9+ pretty quickly. Thanks, Bryan --- Stack trace: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff8c157f34 WebCore::Path::isEmpty() const + 4 1 com.apple.WebCore 0x00007fff8c23b9d0 WebCore::RenderSVGShape::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 80 2 com.apple.WebCore 0x00007fff8c28ff63 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 755 3 com.apple.WebCore 0x00007fff8c28ff63 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 755 4 com.apple.WebCore 0x00007fff8c28ffb1 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 833 5 com.apple.WebCore 0x00007fff8c28ffb1 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 833 6 com.apple.WebCore 0x00007fff8c23b698 WebCore::RenderSVGRoot::paintReplaced(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 1544 7 com.apple.WebCore 0x00007fff8c1cd802 WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 770 8 com.apple.WebCore 0x00007fff8cc78753 WebCore::RenderElement::paintAsInlineBlock(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 131 9 com.apple.WebCore 0x00007fff8c775742 WebCore::InlineElementBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 258 10 com.apple.WebCore 0x00007fff8c158fb2 WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 1762 11 com.apple.WebCore 0x00007fff8c158846 WebCore::RootInlineBox::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 198 12 com.apple.WebCore 0x00007fff8c11aabc WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, WebCore::LayoutPoint const&) const + 1004 13 com.apple.WebCore 0x00007fff8c1174b3 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 547 14 com.apple.WebCore 0x00007fff8c11931e WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 478 15 com.apple.WebCore 0x00007fff8cc9f1bd WebCore::RenderLayer::paintForegroundForFragmentsWithPhase(WebCore::PaintPhase, WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*) + 397 16 com.apple.WebCore 0x00007fff8cc9ce43 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) + 467 17 com.apple.WebCore 0x00007fff8c116a20 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 3056 18 com.apple.WebCore 0x00007fff8c13c31c WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, unsigned int) + 524 19 com.apple.WebCore 0x00007fff8cca9d20 WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, unsigned int, WebCore::FloatRect const&) + 528 20 com.apple.WebCore 0x00007fff8c68f467 WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::FloatRect const&) + 135 21 com.apple.WebCore 0x00007fff8cc07199 WebCore::PlatformCALayer::drawLayerContents(CGContext*, WebCore::PlatformCALayer*, WTF::Vector<WebCore::FloatRect, 5ul, WTF::CrashOnOverflow, 16ul>&) + 345 22 com.apple.WebCore 0x00007fff8c15619d -[WebLayer drawInContext:] + 61 23 com.apple.QuartzCore 0x00007fff920b0d44 CABackingStoreUpdate_ + 4049 24 com.apple.QuartzCore 0x00007fff920afd6d ___ZN2CA5Layer8display_Ev_block_invoke + 59 25 com.apple.QuartzCore 0x00007fff920af759 CA::Layer::display_() + 1565 26 com.apple.WebCore 0x00007fff8cfc0f0b -[WebSimpleLayer display] + 43 27 com.apple.QuartzCore 0x00007fff920a14a5 CA::Layer::display_if_needed(CA::Transaction*) + 603 28 com.apple.QuartzCore 0x00007fff920a0fcd CA::Layer::layout_and_display_if_needed(CA::Transaction*) + 35 29 com.apple.QuartzCore 0x00007fff920a04a1 CA::Context::commit_transaction(CA::Transaction*) + 277 30 com.apple.QuartzCore 0x00007fff920a00ec CA::Transaction::commit() + 508 31 com.apple.QuartzCore 0x00007fff920ab977 CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*) + 71 32 com.apple.CoreFoundation 0x00007fff89498067 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23 33 com.apple.CoreFoundation 0x00007fff89497fd7 __CFRunLoopDoObservers + 391 34 com.apple.CoreFoundation 0x00007fff89476ef8 CFRunLoopRunSpecific + 328 35 com.apple.HIToolbox 0x00007fff81bbd935 RunCurrentEventLoopInMode + 235 36 com.apple.HIToolbox 0x00007fff81bbd76f ReceiveNextEventCommon + 432 37 com.apple.HIToolbox 0x00007fff81bbd5af _BlockUntilNextEventMatchingListInModeWithFilter + 71 38 com.apple.AppKit 0x00007fff8eae0efa _DPSNextEvent + 1067 39 com.apple.AppKit 0x00007fff8eae032a -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454 40 com.apple.AppKit 0x00007fff8ead4e84 -[NSApplication run] + 682 41 com.apple.AppKit 0x00007fff8ea9e46c NSApplicationMain + 1176 42 libxpc.dylib 0x00007fff913f245e _xpc_objc_main + 793 43 libxpc.dylib 0x00007fff913f0e8a xpc_main + 494 44 com.apple.WebKit.WebContent 0x000000010e388b4a 0x10e388000 + 2890 45 libdyld.dylib 0x00007fff889895ad start + 1
Said Abou-Hallawa
Comment 32 2016-03-23 14:44:02 PDT
(In reply to comment #31) I opened the link http://sonny.7thposition.com/osm/iD/#map=17.76/18.53595/4.33465 and I tried to interact with the map for some time but I could not reproduce the crash. If you describe the steps that can reproduce the crash, this will be great. Also you can attach a video showing what you meant by the interaction with the map and showing Safari crashing at the end. There is no direct call to Path::isEmpty() from RenderSVGShape::paint(). This might be because of compiler optimization. If you try the crash from a debug build, where the optimization is disabled, and attach the crashing call stack this will be very helpful. Also the change I did in RenderSVGShape::isEmpty() was to prevent the crash if RenderSVGShape::m_path was null. This function before my change looked like this: bool RenderSVGShape::isEmpty() const { return path().isEmpty(); } And now it looks like this: bool RenderSVGShape::isEmpty() const { // This function should never be called before assigning a new Path to m_path. // But this bug can happen if this renderer was created and its layout was not // done before painting. Assert this did not happen but do not crash. ASSERT(hasPath()); return !hasPath() || path().isEmpty(); } And from the above call stack, the crash happens in Path::isEmpty(), although m_path is not null. This means the crash, if it happens, is because of memory corruption. m_path is not null but it points to an invalid memory block.
Jon Lee
Comment 33 2016-03-29 21:45:00 PDT
Bryan, if you can find reproducible steps, please file a new bug.
Note You need to log in before you can comment on or make changes to this bug.