Bug 44533 - XMLDocumentParser needs to implement DocumentParser::detach()
: XMLDocumentParser needs to implement DocumentParser::detach()
Product: WebKit
Classification: Unclassified
Component: XML DOM
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Critical
Assigned To: Eric Seidel
: InRadar, NeedsReduction
Depends on:
  Show dependency treegraph
Reported: 2010-08-24 10:15 PDT by Chasen Le Hara
Modified: 2010-09-16 17:05 PDT (History)
4 users (show)

See Also:

Patch (17.47 KB, patch)
2010-08-24 16:44 PDT, Eric Seidel
abarth: review+
abarth: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chasen Le Hara 2010-08-24 10:15:07 PDT
Visiting the page in the latest version of WebKit (r65825, Safari 5.0.1) causes a crash.
Comment 1 Alexey Proskuryakov 2010-08-24 12:01:24 PDT
#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 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?
Comment 2 Eric Seidel 2010-08-24 13:46:18 PDT
Comment 3 Eric Seidel 2010-08-24 14:14:34 PDT
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.
Comment 4 Eric Seidel 2010-08-24 14:20:27 PDT
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()
    if (!m_externalSVGDocument && !errorOccurred() && !isLoading() && m_data) {
        m_externalSVGDocument = SVGDocument::create(0, KURL());

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


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™.
Comment 5 Eric Seidel 2010-08-24 14:29:40 PDT
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.
Comment 6 Eric Seidel 2010-08-24 16:44:10 PDT
Created attachment 65343 [details]
Comment 7 Adam Barth 2010-08-24 16:49:19 PDT
Comment on attachment 65343 [details]

This looks good.  Don't we want a test case for the original crash though?
Comment 8 Eric Seidel 2010-08-24 16:54:44 PDT
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.
Comment 9 Eric Seidel 2010-08-24 16:57:55 PDT
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?
Comment 10 Eric Seidel 2010-08-24 16:58:25 PDT
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
Comment 11 Eric Seidel 2010-08-24 17:04:48 PDT
The page is explicitly telling WebKit it's an SVG font when it isn't:


@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.
Comment 12 Eric Seidel 2010-08-24 17:29:57 PDT
Added one.  Will commit.
Comment 13 Eric Seidel 2010-08-24 17:30:58 PDT
Folks releasing from a branch may want this change.  Should be easy to pick up.
Comment 14 Eric Seidel 2010-08-24 17:40:29 PDT
Committed r65958: <http://trac.webkit.org/changeset/65958>
Comment 15 Eric Seidel 2010-08-24 21:34:09 PDT
Committed r65976: <http://trac.webkit.org/changeset/65976>
Comment 16 David Kilzer (:ddkilzer) 2010-09-16 17:04:59 PDT