Summary: | Crash while handling SVG font in the wrong namespace imported with @font-face | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Daggett <jdaggett> | ||||||||||
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Major | CC: | darin, jchaffraix, zimmermann | ||||||||||
Priority: | P1 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
John Daggett
2008-05-02 19:11:28 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
Created attachment 20940 [details]
testcase SVG font
same font without testpage or any other modifications after creation in FontForge
Created attachment 20941 [details]
testcase
Steps to reproduce:
1. Download both testcase files
2. Load testcase
Result: crash during font load
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) Changing <svg> to <svg xmlns="http://www.w3.org/2000/svg"> in the font file seems to fix the problem, go figure... Confirmed on ToT. Promoting to P1 as it is a crasher. Assigning this bug to me as I have a patch for it. (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. Created attachment 52702 [details]
Proposed fix: make sure that we cast real a SVGFontElement
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. > > - 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? (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. > > 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.
Landed the core in r57779. |