Bug 44533 - XMLDocumentParser needs to implement DocumentParser::detach()
: XMLDocumentParser needs to implement DocumentParser::detach()
Status: RESOLVED FIXED
: WebKit
XML DOM
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Critical
Assigned To:
: http://weblab.ab-c.nl/streetview
: InRadar, NeedsReduction
:
:
  Show dependency treegraph
 
Reported: 2010-08-24 10:15 PST by
Modified: 2010-09-16 17:05 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-24 10:15:07 PST
Visiting the page in the latest version of WebKit (r65825, Safari 5.0.1) causes a crash.
------- Comment #1 From 2010-08-24 12:01:24 PST -------
#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?
------- Comment #2 From 2010-08-24 13:46:18 PST -------
Investigating.
------- Comment #3 From 2010-08-24 14:14:34 PST -------
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 From 2010-08-24 14:20:27 PST -------
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ā„¢.
------- Comment #5 From 2010-08-24 14:29:40 PST -------
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 From 2010-08-24 16:44:10 PST -------
Created an attachment (id=65343) [details]
Patch
------- Comment #7 From 2010-08-24 16:49:19 PST -------
(From update of attachment 65343 [details])
This looks good.  Don't we want a test case for the original crash though?
------- Comment #8 From 2010-08-24 16:54:44 PST -------
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 From 2010-08-24 16:57:55 PST -------
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 From 2010-08-24 16:58:25 PST -------
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 From 2010-08-24 17:04:48 PST -------
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.
------- Comment #12 From 2010-08-24 17:29:57 PST -------
Added one.  Will commit.
------- Comment #13 From 2010-08-24 17:30:58 PST -------
Folks releasing from a branch may want this change.  Should be easy to pick up.
------- Comment #14 From 2010-08-24 17:40:29 PST -------
Committed r65958: <http://trac.webkit.org/changeset/65958>
------- Comment #15 From 2010-08-24 21:34:09 PST -------
Committed r65976: <http://trac.webkit.org/changeset/65976>
------- Comment #16 From 2010-09-16 17:04:59 PST -------
<rdar://problem/8442098>