[webkit-reviews] review denied: [Bug 23586] crash on SVGUseElement::expandSymbolElementsInShadowTree( : [Attachment 27107] workaround to prevent the crash

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 28 09:00:34 PST 2009

Darin Adler <darin at apple.com> has denied Ariya Hidayat
<ariya.hidayat at trolltech.com>'s request for review:
Bug 23586: crash on SVGUseElement::expandSymbolElementsInShadowTree(

Attachment 27107: workaround to prevent the crash

------- Additional Comments from Darin Adler <darin at apple.com>
> 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,
> +	   // 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-.

More information about the webkit-reviews mailing list