Bug 23586

Summary: crash on SVGUseElement::expandSymbolElementsInShadowTree(
Product: WebKit Reporter: Ariya Hidayat <ariya.hidayat>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: a.neumann, eric, gdelfino, scarybeasts, zimmermann
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
URL: http://www.croczilla.com/svg/samples/invaders/invaders.svg
Bug Depends on: 25092    
Bug Blocks: 22856    
Attachments:
Description Flags
the backtrace upon crash
none
workaround to prevent the crash
darin: review-
test case which causes a related ASSERT (not necessarily the same bug)
none
backtrace for the related ASSERT (not necessarily the same bug) none

Description Ariya Hidayat 2009-01-28 08:07:33 PST
Using QtWebKit and playing SVG Space Invaders (http://www.croczilla.com/svg/samples/invaders/invaders.svg) ends up with a crash.
Comment 1 Ariya Hidayat 2009-01-28 08:34:37 PST
Created attachment 27106 [details]
the backtrace upon crash
Comment 2 Ariya Hidayat 2009-01-28 08:37:42 PST
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.
Comment 3 Darin Adler 2009-01-28 09:00:34 PST
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-.
Comment 4 Mark Rowe (bdash) 2009-01-28 14:01:38 PST
*** Bug 23578 has been marked as a duplicate of this bug. ***
Comment 5 Eric Seidel (no email) 2009-04-07 05:16:55 PDT
*** Bug 25074 has been marked as a duplicate of this bug. ***
Comment 6 Eric Seidel (no email) 2009-04-07 05:18:54 PDT
As noted in dupe bug 25074, this is a regression (and a reproducible crasher).  Thus a P1.
Comment 7 Eric Seidel (no email) 2009-04-07 06:56:45 PDT
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. :)
Comment 8 Eric Seidel (no email) 2009-04-07 06:57:25 PDT
Created attachment 29310 [details]
test case which causes a related ASSERT (not necessarily the same bug)
Comment 9 Eric Seidel (no email) 2009-04-07 06:58:21 PDT
Created attachment 29311 [details]
backtrace for the related ASSERT (not necessarily the same bug)
Comment 10 Eric Seidel (no email) 2009-04-07 07:00:03 PDT
(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.
Comment 11 Eric Seidel (no email) 2009-04-08 05:59:30 PDT
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.
Comment 12 Eric Seidel (no email) 2009-04-08 06:01:52 PDT
Sigh.  Removing the ASSERT(instance->shadowTreeElement()) causes the test to "pass", and does not trip the bug we were actually trying to test here. :(
Comment 13 Nikolas Zimmermann 2010-01-17 17:23:57 PST
*** Bug 22856 has been marked as a duplicate of this bug. ***
Comment 14 Nikolas Zimmermann 2010-01-17 17:24:05 PST
*** Bug 21590 has been marked as a duplicate of this bug. ***
Comment 15 Nikolas Zimmermann 2010-01-18 19:59:12 PST
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.