Bug 18862

Summary: Crash while handling SVG font in the wrong namespace imported with @font-face
Product: WebKit Reporter: John Daggett <jdaggett>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Major CC: darin, jchaffraix, zimmermann
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
svg font file made using FontForge
none
testcase SVG font
none
testcase
none
Proposed fix: make sure that we cast real a SVGFontElement darin: review+, jchaffraix: commit-queue-

Description John Daggett 2008-05-02 19:11:28 PDT
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");
}
Comment 1 John Daggett 2008-05-02 19:14:42 PDT
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
Comment 2 John Daggett 2008-05-02 19:29:11 PDT
Created attachment 20940 [details]
testcase SVG font

same font without testpage or any other modifications after creation in FontForge
Comment 3 John Daggett 2008-05-02 19:30:52 PDT
Created attachment 20941 [details]
testcase

Steps to reproduce:

1. Download both testcase files
2. Load testcase

Result: crash during font load
Comment 4 John Daggett 2008-05-02 19:34:38 PDT
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)
Comment 5 John Daggett 2008-05-02 19:39:59 PDT
Changing <svg> to <svg xmlns="http://www.w3.org/2000/svg"> in the font file seems to fix the problem, go figure...
Comment 6 Julien Chaffraix 2010-04-06 21:12:55 PDT
Confirmed on ToT. Promoting to P1 as it is a crasher.

Assigning this bug to me as I have a patch for it.
Comment 7 Julien Chaffraix 2010-04-06 21:23:28 PDT
(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.
Comment 8 Julien Chaffraix 2010-04-06 21:41:03 PDT
Created attachment 52702 [details]
Proposed fix: make sure that we cast real a SVGFontElement
Comment 9 Darin Adler 2010-04-06 22:04:54 PDT
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.
Comment 10 Julien Chaffraix 2010-04-06 22:48:05 PDT
> > -        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?
Comment 11 Darin Adler 2010-04-07 09:42:06 PDT
(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.
Comment 12 Julien Chaffraix 2010-04-16 06:54:59 PDT
> > 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.
Comment 13 Julien Chaffraix 2010-04-17 07:36:31 PDT
Landed the core in r57779.