Bug 149613 - A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
Summary: A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.10
: P1 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-28 22:55 PDT by Neil
Modified: 2016-03-29 21:45 PDT (History)
12 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (11.57 KB, patch)
2016-01-15 20:39 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (19.40 KB, patch)
2016-01-20 15:30 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (21.57 KB, patch)
2016-01-20 18:08 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Neil 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.
Comment 1 Alexey Proskuryakov 2015-09-28 23:36:25 PDT
For Apple employees: see also rdar://problem/14485328 (but the stack trace is different).
Comment 2 Radar WebKit Bug Importer 2015-09-28 23:36:39 PDT
<rdar://problem/22892801>
Comment 3 Said Abou-Hallawa 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.
Comment 4 Said Abou-Hallawa 2016-01-13 17:50:32 PST
Created attachment 268926 [details]
Patch
Comment 5 Said Abou-Hallawa 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.
Comment 6 Said Abou-Hallawa 2016-01-15 13:16:24 PST
This looks a regression from http://trac.webkit.org/changeset/103539.
Comment 7 Said Abou-Hallawa 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.
Comment 8 Said Abou-Hallawa 2016-01-15 20:39:19 PST
Created attachment 269145 [details]
Patch
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Said Abou-Hallawa 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.
Comment 16 Darin Adler 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.
Comment 17 Darin Adler 2016-01-18 18:52:28 PST
I understand that you are also contemplating a different fix, inspired by the change in Blink.
Comment 18 zalan 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?
Comment 19 Said Abou-Hallawa 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.
Comment 20 Said Abou-Hallawa 2016-01-20 13:11:25 PST
Created attachment 269377 [details]
Test case not working in Blink
Comment 21 Said Abou-Hallawa 2016-01-20 13:14:20 PST
Created attachment 269378 [details]
Another test case not working in Blink
Comment 22 Said Abou-Hallawa 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.
Comment 23 Said Abou-Hallawa 2016-01-20 15:05:18 PST
Created attachment 269392 [details]
Patch
Comment 24 Said Abou-Hallawa 2016-01-20 15:30:32 PST
Created attachment 269396 [details]
Patch
Comment 25 Darin Adler 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.
Comment 26 Said Abou-Hallawa 2016-01-20 18:08:46 PST
Created attachment 269410 [details]
Patch
Comment 27 Said Abou-Hallawa 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.
Comment 28 Said Abou-Hallawa 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.
Comment 29 WebKit Commit Bot 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>
Comment 30 WebKit Commit Bot 2016-01-21 09:50:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Bryan Housel 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
Comment 32 Said Abou-Hallawa 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.
Comment 33 Jon Lee 2016-03-29 21:45:00 PDT
Bryan, if you can find reproducible steps, please file a new bug.