Bug 23586 – crash on SVGUseElement::expandSymbolElementsInShadowTree(

• Component: SVG
• Status: RESOLVED FIXED

[Ariya Hidayat]

Using QtWebKit and playing SVG Space Invaders (http://www.croczilla.com/svg/samples/invaders/invaders.svg) ends up with a crash.

[Ariya Hidayat]

Created attachment 27106 [details]
the backtrace upon crash

[Ariya Hidayat]

Created attachment 27107 [details]
workaround to prevent the crash

Avoid NULL parentNode() is a stop-gap measure / work-around. The actual cause for the bug needs more investigation.

[Darin Adler]

Comment on attachment 27107 [details]
workaround to prevent the crash

> diff --git a/WebCore/svg/SVGUseElement.cpp b/WebCore/svg/SVGUseElement.cpp
> index 83ad559..b5beae9 100644
> --- a/WebCore/svg/SVGUseElement.cpp
> +++ b/WebCore/svg/SVGUseElement.cpp
> @@ -719,8 +719,9 @@ void SVGUseElement::expandSymbolElementsInShadowTree(Node* element)
>              removeDisallowedElementsFromSubtree(svgElement.get());
>  
>          // Replace <symbol> with <svg>.
> -        ASSERT(element->parentNode()); 
> -        element->parentNode()->replaceChild(svgElement.release(), element, ec);
> +        // ASSERT(element->parentNode());
> +        if (element->parentNode())
> +            element->parentNode()->replaceChild(svgElement.release(), element, ec);
>          ASSERT(ec == 0);

Keep in mind that when the DOM, JavaScript, and mutation events are involved, it's not possible to make any assumptions about things like this, so it's not a huge surprise that sometimes the element might not have a parent here. Even if you do find what's causing this in one particular case, we have to make sure that our DOM manipulation code does not assume too much. If you make a call to modify the DOM, by the time you return everything might have changed. All the nodes might be in an entirely new configuration. And our code has to cope with that without crashing.

The change seems fine, but you need a ChangeLog and a regression test for this, so review-.

[Mark Rowe (bdash)]

*** Bug 23578 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 25074 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

As noted in dupe bug 25074, this is a regression (and a reproducible crasher).  Thus a P1.

[Eric Seidel (no email)]

Ok, I've created a test case which might cause this crash.  Right now it hits another ASSERT first:
ASSERTION FAILED: instance->shadowTreeElement()
(/Users/eseidel/Projects/WebKit/WebCore/svg/SVGUseElement.cpp:838 WebCore::SVGElementInstance* WebCore::SVGUseElement::instanceForShadowTreeElement(WebCore::Node*, WebCore::SVGElementInstance*) const)

The testcase I've created is unlikely to be the root cause of this bug.  Since again, invaders.svg used to work fine.  I'm not sure if they're doing something which silently fails (but doesn't crash) in FF, or if we have some bug in our <use> code which they're tripping with totally innocent SVG code.  By working around this crash with an if, we might just be papering over some other bug.  Then again, if we're able to repro this crash in other ways, then it needs to be fixed with an null-check anyway. :)

[Eric Seidel (no email)]

Created attachment 29310 [details]
test case which causes a related ASSERT (not necessarily the same bug)

[Eric Seidel (no email)]

Created attachment 29311 [details]
backtrace for the related ASSERT (not necessarily the same bug)

[Eric Seidel (no email)]

(In reply to comment #8)
> Created an attachment (id=29310) [review]
> test case which causes a related ASSERT (not necessarily the same bug)
> 

It might be possible to modify this test case to only prevent the 3rd insertion or similar, and thus trip the exact ASSERT seen by invaders.svg.

[Eric Seidel (no email)]

The ASSERT which the new test case hits, is because we're trying to dispatch an event to the corresponding node for a just-inserted shadow tree node.  The new shadow tree node has not yet had a shadowTreeElement() associated with it, because
void SVGUseElement::associateInstancesWithShadowTreeElements(Node* target, SVGElementInstance* targetInstance)
has not been called yet.

We could remove the ASSERT to allow this dispatch (and keep searching for the actual bug in this example), or we could change the <use> design to associate shadow tree elements with their originals as they're being added instead of afterwards.

[Eric Seidel (no email)]

Sigh.  Removing the ASSERT(instance->shadowTreeElement()) causes the test to "pass", and does not trip the bug we were actually trying to test here. :(

[Nikolas Zimmermann]

*** Bug 22856 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 21590 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Fixed in ToT, after the <use> rewrite, we're not doing sync tree rebuilding during svgAttributeChanged or sth similar - as Darin mentioned the side-effects are unpredictable. Closing bug.