Fixes crash - #632

SVGElement guarantees to have that selector. So ... if you are having a crash on this, it's because something is passing-in something that is not an SVGElement instance (in violation of the method signature). Whatever is doing that needs to be fixed, and this bug will disappear.

This is incorrect:

if ( [element respondsToSelector:@selector(cascadedValueForStylableProperty:inherit:)] )

In the debugger, what is the actual type of the object "element" at the time of the crash? And where did that object come from?

I'm guessing, based on quick search, that this is the problem:

    for (SVGElement *child in clipPathElement.childNodes )
    {
        if ([child conformsToProtocol:@protocol(ConverterSVGToCALayer)]) {       
            CALayer *sublayer = [self newLayerWithElement:(SVGElement<ConverterSVGToCALayer> *)child];

... that code in clip-paths is assuming that ALL children are valid SVG nodes (which isn't true, in general). Perhaps breakpoint there and verify if it's accidentally trying to process some non-SVGElement childnodes.

Incidentally ... the ConverterSVGToCALayer appears to be an old hack that needs replacing anyway (I see a few FIXME comments in some source files).

@valeriyvan @adamgit I can reproduce the crash. But this fix does not solve the issue from the scratch.

See my debug screenshot:

It seems this line of code: https://github.com/SVGKit/SVGKit/blob/3.x/Source/Parsers/Parser%20Extensions/SVGKParserDefsAndUse.m#L97

Does not check whether the <link ref> is a SVGElement, or a normal DOM element. This is the main issue. We should only do recusrive parse for SVGElement, and ignore the DOM element.

Ok. @dreampiggy, it seems you know what has to be fixed.

@valeriyvan I found the issue is that SVGKit does not support symbol element. Which means, even I can fix the crash, this demo SVG can never been rendered :)

So, maybe this need to be a long-term feature request. To support <symbol> element. And maybe this fix does not help for production issues...You can re-write your SVG using the raw g or other element.

@dreampiggy, issuing log message on not supported element is much better than crashing somewhere far from real culprit of crash, I think. Having this log message for me would have been big timesaver.

You can re-write your SVG using the raw g or other element.

I can't. SVGs are generated by huge library.

You might be able to create a relatively simple XSLT that would convert the library's SVG into one that uses G everytime the original uses the SYMBOL? But, yeah ... adding support for SYMBOL would be better :)

And then what? Find out that some other strange crash? This library supports some subset of SVG features. That's OK. Unfortunately it isn't documented what is supported and what is not. Library just crashes when not supported features of SVG are used.

The library does NOT crash when it hits unsupported features. Anything unsupported is automatically ignored (it is passed-through as if it were regular XML - SVGKit has to do this, because an SVG can legally include (almost) any XML at all!).

The SYMBOL situation is a one-off: it has some implementation code (which causes the posibility of a crash), but it's a complex feature, rarely used in real-world SVGs, that hasn't affected most people. So we've ended up with an incomplete (or totally non-working?) implementation that almost no-one noticed(!) and no-one cared about enough to finish / update in the 6 years since it was first prototyped.

...to be clear: I'm not saying it's OK that it got missed/ignored/unloved, when I joined the project we had something like 10% support and we're now well over 50% (if you look at the stuff usually found in real-world SVG's, I'd say more like 75-90%) - I pushed hard to get the coverage as high as possible.

But the reality of an open-source, volunteer, project is that the less-common features are in danger of getting missed out :(.

👋 Hey all! Sorry to chime in on an old thread.

I was about to PR this fix myself, until I found this discussion. I would love to see this fix land as part of incremental correctness to this library. Crashing an app on SVGs that the library cannot handle is not great behavior, especially if we want to encourage other folks to use this library. Getting this fix landed to stop the bleeding would be a great temporary solution.

I'd love to start work on supporting <symbol>s, as we've identified a large number of SVGs in READMEs across projects on GitHub:

• mochajs/mocha
  • various SauceLabs badges use <symbol>s, such as: https://saucelabs.com/browser-matrix/mochajs.svg
• curl/curl
  • their logo uses <symbol>s: https://curl.haxx.se/logo/curl-logo.svg

I'll start taking a stab at contributing <symbol> support now, but I'd love to see this PR land to cure some well-understood crashes!

Related to #649, #487

As I understood it originally, this PR wasn't a "fix", rather it hides a bug somewhere else, and pretends it's not there?

Finding + fixing the actual bug would be great (I see I made a guess at which line it might be - but I haven't done native iOS development recently, so I haven't had an opportunity to investigate it myself).

NB: if we're going to hide/mask bugs like this one, the way to do it is put something in that detects globally when we try to access an object as SVGElement but it's actually a plain Node - and Assert on that (i.e. crash in development, do nothing in production). There is no situation in which you should be reading a Node and thinking/pretending that it's an SVGElement, but I had a couple of times when implementing some of the tags where I accidentally set up a case like that (always my own bug).

this PR wasn't a "fix", rather it hides a bug somewhere else, and pretends it's not there?

I'm not sure what you mean here. As I understand it, and from my testing, a patch in the style of this PR prevents crashing, but does not add or remove any functionality already existing in the project.

In the project's current state, support for <symbol>s and other future properties added to the SVG spec will cause this same crash. Landing a PR like this will be leave the project in a more future-proof state, as we will not assume any DOM/XML entity is always an SVG entity.

if we're going to hide/mask bugs like this one, the way to do it is put something in that detects globally when we try to access an object as SVGElement but it's actually a plain Node - and Assert on that (i.e. crash in development, do nothing in production). There is no situation in which you should be reading a Node and thinking/pretending that it's an SVGElement, but I had a couple of times when implementing some of the tags where I accidentally set up a case like that (always my own bug).

Would a PR using an NSAssert to provide the same behavior as this PR be accepted then?

EDIT: I hit enter too soon... working on this comment still 😅