Bug 44533 – XMLDocumentParser needs to implement DocumentParser::detach()

• Component: DOM
• Status: RESOLVED FIXED

[Chasen Le Hara]

Visiting the page in the latest version of WebKit (r65825, Safari 5.0.1) causes a crash.

[Alexey Proskuryakov]

#0	0x1029e0421 in WebCore::TreeShared<WebCore::Node>::deref at TreeShared.h:71
#1	0x10367b6cf in WebCore::XMLDocumentParser::clearCurrentNodeStack at XMLDocumentParser.cpp:107
#2	0x10367d0ed in WebCore::XMLDocumentParser::~XMLDocumentParser at XMLDocumentParserLibxml2.cpp:619
#3	0x102c3598d in WTF::RefCounted<WebCore::DocumentParser>::deref at RefCounted.h:139
#4	0x102c4b317 in WTF::derefIfNotNull<WebCore::DocumentParser> at PassRefPtr.h:58
#5	0x102c4b39d in WTF::RefPtr<WebCore::DocumentParser>::clear at RefPtr.h:104
#6	0x102c1c77d in WebCore::Document::detachParser at Document.cpp:1830
#7	0x102c2b20c in WebCore::Document::~Document at Document.cpp:541
#8	0x1035386cd in WebCore::SVGDocument::~SVGDocument at SVGDocument.cpp:45
#9	0x102c2b87a in WebCore::Document::removedLastRef at Document.cpp:514
#10	0x1029e04ff in WebCore::TreeShared<WebCore::Node>::deref at TreeShared.h:78
#11	0x102c9c469 in WTF::derefIfNotNull<WebCore::SVGDocument> at PassRefPtr.h:58
#12	0x102aa2839 in WTF::RefPtr<WebCore::SVGDocument>::clear at RefPtr.h:104
#13	0x102aa1f3a in WebCore::CachedFont::ensureSVGFontData at CachedFont.cpp:146
#14	0x102b24abf in WebCore::CSSFontFaceSource::getFontData at CSSFontFaceSource.cpp:130

    if (m_currentNode && m_currentNode != document())
        m_currentNode->deref();

m_currentNode is a Document, but DocumentParser::m_document is null, so we're wrongly trying to deref the document.

Could it be an HTML5 parser related change that DocumentParser::m_document is null here?

[Eric Seidel (no email)]

Investigating.

[Eric Seidel (no email)]

The ASSERT/crash is that the Document is already being deleted and the XMLDocumentParser still has a reference to it on its node stack.

This may have been caused by changes to fragment parsing.  Seems we should have cleared the node stack earlier.

I'm not sure how this worked before.

[Eric Seidel (no email)]

I don't think it's "safe" in this case, to blast away the Document before telling the XMLDocumentParser we're done.

I believe the wrong code is bool CachedFont::ensureSVGFontData().

bool CachedFont::ensureSVGFontData()
{
    ASSERT(m_isSVGFont);
    if (!m_externalSVGDocument && !errorOccurred() && !isLoading() && m_data) {
        m_externalSVGDocument = SVGDocument::create(0, KURL());
        m_externalSVGDocument->open();

        RefPtr<TextResourceDecoder> decoder = TextResourceDecoder::create("application/xml");
        m_externalSVGDocument->write(decoder->decode(m_data->data(), m_data->size()));
        m_externalSVGDocument->write(decoder->flush());
        if (decoder->sawError()) {
            m_externalSVGDocument.clear();
            return 0;
        }

        m_externalSVGDocument->finishParsing();
        m_externalSVGDocument->close();
    }

We should move the decoding up to before opening the document, or we should let the parse finish and only abort on decoding error after the parse is done.

I'm not sure what the old parser was doing here, but it's possible it was doing Bad Things™.

[Eric Seidel (no email)]

On second thought, I think the root cause here is that XMLDocumentParser needs a detach() implementation, which should do things like clearing the node stack at that time.

[Eric Seidel (no email)]

Created attachment 65343 [details]
Patch

[Adam Barth]

Comment on attachment 65343 [details]
Patch

This looks good.  Don't we want a test case for the original crash though?

[Eric Seidel (no email)]

I'm not averse to adding one.  The case encountered by the orignal crash is now transformed to be covered by a whole bunch of Layout Tests.  I'll see if I can whip up an SVG font with invalid decoding however.

[Eric Seidel (no email)]

I believe http://weblab.ab-c.nl/css/font/abc.aspx is the font in question.  It's a truetype file, but maybe somehow it's heading down the SVG font path?

[Eric Seidel (no email)]

curl -I "http://weblab.ab-c.nl/css/font/abc.aspx"
HTTP/1.1 200 OK
Date: Tue, 24 Aug 2010 23:58:09 GMT
Server: Microsoft-IIS/6.0
X-UA-Compatible: IE=EmulateIE7
X-Powered-By: ASP.NET
X-AspNet-Version: 2.0.50727
Cache-Control: private
Content-Length: 0

[Eric Seidel (no email)]

The page is explicitly telling WebKit it's an SVG font when it isn't:

http://weblab.ab-c.nl/css/weblab.css:

@font-face {font-family: 'abc'; src: url('font/abc.aspx#webfont6R4Fh5cD') format('svg'), url('font/abc.aspx')}

So a test case should be easy to create.

[Eric Seidel (no email)]

Added one.  Will commit.

[Eric Seidel (no email)]

Folks releasing from a branch may want this change.  Should be easy to pick up.

[Eric Seidel (no email)]

Committed r65958: <http://trac.webkit.org/changeset/65958>

[Eric Seidel (no email)]

Committed r65976: <http://trac.webkit.org/changeset/65976>

[David Kilzer (:ddkilzer)]

<rdar://problem/8442098>

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.