Bug 18862 – Crash while handling SVG font in the wrong namespace imported with @font-face

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[John Daggett]

Webkit crashes when loading fonts using this @font-face definition:

@font-face {
    font-family: My DejaVu Sans Mono;
    src: url(MyDejaVuSansMono.svg#MyDejaVuSansMono) format("svg"), local("DejaVu Sans Mono");
}

[John Daggett]

Created attachment 20939 [details]
svg font file made using FontForge

Created using FontForge.

1. Import Deja Vu Sans Mono
2. Change name info to My Deja Vu Sans Mono
3. Generate font in SVG format
4. Edit to include test lines

[John Daggett]

Created attachment 20940 [details]
testcase SVG font

same font without testpage or any other modifications after creation in FontForge

[John Daggett]

Created attachment 20941 [details]
testcase

Steps to reproduce:

1. Download both testcase files
2. Load testcase

Result: crash during font load

[John Daggett]

console output on crash:

ASSERTION FAILED: node->hasTagName(SVGNames::fontTag)
(/builds/webkit/trunk/WebKit/WebCore/loader/CachedFont.cpp:167 WebCore::SVGFontElement* WebCore::CachedFont::getSVGFontById(const WebCore::String&) const)

[John Daggett]

Changing <svg> to <svg xmlns="http://www.w3.org/2000/svg"> in the font file seems to fix the problem, go figure...

[Julien Chaffraix]

Confirmed on ToT. Promoting to P1 as it is a crasher.

Assigning this bug to me as I have a patch for it.

[Julien Chaffraix]

(In reply to comment #5)
> Changing <svg> to <svg xmlns="http://www.w3.org/2000/svg"> in the font file
> seems to fix the problem, go figure...

FYI, this is the core of the issue. We use both local name and namespaceURI to determine which element to create. The crash is due to us not checking the namespaceURI which made us doing a static_cast from an element of the wrong type.

[Julien Chaffraix]

Created attachment 52702 [details]
Proposed fix: make sure that we cast real a SVGFontElement

[Darin Adler]

Comment on attachment 52702 [details]
Proposed fix: make sure that we cast real a SVGFontElement

> -    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)->idAttributeName()) != 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.

[Julien Chaffraix]

> > -        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)->idAttributeName()) != 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*.

I don't get it: Element::isElementNode() will return the same result before or after the cast (thought it may be better to avoid shooting ourselves in the foot to do it before in case somebody change this behaviour).

> 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.

This part was supposed to be harmless that's why I squashed it with this change as we usually do for such simple refactorings.

I am not convinced the ASSERT is wrong in itself: we are downcasting a Node to an Element in the old code too. The change just made visible that the items of our NodeList should be Element's.

I am confused as what you want to do with this part: refactoring (ASSERT before cast), plain removal or migration to another change?

[Darin Adler]

(In reply to comment #10)
> I don't get it: Element::isElementNode() will return the same result before or
> after the cast (thought it may be better to avoid shooting ourselves in the
> foot to do it before in case somebody change this behaviour).

It's not legal to call isElementNode on an Element*. Although it compiles right now, it's not guaranteed to work in the future. You need to call isElementNode on a Node* 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.
> 
> This part was supposed to be harmless that's why I squashed it with this change
> as we usually do for such simple refactorings.

I wish you hadn't.

> I am not convinced the ASSERT is wrong in itself: we are downcasting a Node to
> an Element in the old code too. The change just made visible that the items of
> our NodeList should be Element's.

It's the mechanics of the assertion that is wrong.

> I am confused as what you want to do with this part: refactoring (ASSERT before
> cast), plain removal or migration to another change?

I'd prefer that you leave this part of the change out entirely. It has no relationship with the other half of the patch and should be judged on its own merits.

Alternatively you could correct the assertion as follows:

    ASSERT(list->item(i));
    ASSERT(list->item(i)->isElementNode());
    Element* element = static_cast<Element*>(list->item(i));

or:

    Node* node = list->item(i);
    ASSERT(node);
    ASSERT(node->isElementNode());
    Element* element = static_cast<Element*>(node);

If I was writing the function myself I would write this:

    Node* node = list->item(i);
    ASSERT(node);
    ASSERT(node->hasTagName(SVGNames::fontTag));
    return static_cast<SVGFontElement*>(node);

There's no need to cast to Element* since Node* already has the hasTagName function which combines the element check with the tag name check.

[Julien Chaffraix]

> > I am not convinced the ASSERT is wrong in itself: we are downcasting a Node to
> > an Element in the old code too. The change just made visible that the items of
> > our NodeList should be Element's.
> 
> It's the mechanics of the assertion that is wrong.
> 
> > I am confused as what you want to do with this part: refactoring (ASSERT before
> > cast), plain removal or migration to another change?
> 
> I'd prefer that you leave this part of the change out entirely. It has no
> relationship with the other half of the patch and should be judged on its own
> merits.


Ok, thanks for explaining why the logic was wrong. I can see the issue and will just remove this part from the landed patch.

[Julien Chaffraix]

Landed the core in r57779.