[webkit-reviews] review granted: [Bug 18862] Crash while handling SVG font in the wrong namespace imported with @font-face : [Attachment 52702] Proposed fix: make sure that we cast real a SVGFontElement

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 18862: Crash while handling SVG font in the wrong namespace imported with
@font-face
https://bugs.webkit.org/show_bug.cgi?id=18862

Attachment 52702: Proposed fix: make sure that we cast real a SVGFontElement
https://bugs.webkit.org/attachment.cgi?id=52702&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    RefPtr<NodeList> list =
m_externalSVGDocument->getElementsByTagName(SVGNames::fontTag.localName());
> +    RefPtr<NodeList> list =
m_externalSVGDocument->getElementsByTagNameNS(SVGNames::fontTag.namespaceURI(),
SVGNames::fontTag.localName());

Good fix.

> -	   Node* node = list->item(i);
> -	   ASSERT(node);
> +	   Element* element = static_cast<Element*>(list->item(i));
> +	   ASSERT(element);
> +	   ASSERT(element->isElementNode());
>  
> -	   if
(static_cast<Element*>(node)->getAttribute(static_cast<Element*>(node)->idAttri
buteName()) != fontName)
> +	   if (element->getAttribute(element->idAttributeName()) != fontName)
>	       continue;
>  
> -	   ASSERT(node->hasTagName(SVGNames::fontTag));
> -	   return static_cast<SVGFontElement*>(node);
> +	   ASSERT(element->hasTagName(SVGNames::fontTag));
> +	   return static_cast<SVGFontElement*>(element);

It's not right to ASSERT(element->isElementNode()). You should do that
assertion before casting to an Element*.

In general, this second part seems like sort of a gratuitous change. While it
reads slightly nicer with fewer type casts, the existing code was already
correct, and the element->isElementNode assertion part is a mistake that the
old code did not have.