Bug 44533

Summary: XMLDocumentParser needs to implement DocumentParser::detach()
Product: WebKit Reporter: Chasen Le Hara <rendezvouscp>
Component: DOMAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, ap, cdumez, commit-queue, eric
Priority: P1 Keywords: InRadar, NeedsReduction
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
URL: http://weblab.ab-c.nl/streetview
Attachments:
Description Flags
Patch abarth: review+, abarth: commit-queue-

Chasen Le Hara
Reported 2010-08-24 10:15:07 PDT
Visiting the page in the latest version of WebKit (r65825, Safari 5.0.1) causes a crash.
Attachments
Patch (17.47 KB, patch)
2010-08-24 16:44 PDT, Eric Seidel (no email)
abarth: review+
abarth: commit-queue-
Alexey Proskuryakov
Comment 1 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->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)
Comment 2 2010-08-24 13:46:18 PDT
Investigating.
Eric Seidel (no email)
Comment 3 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.
Eric Seidel (no email)
Comment 4 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() { 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)
Comment 5 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.
Eric Seidel (no email)
Comment 6 2010-08-24 16:44:10 PDT
Adam Barth
Comment 7 2010-08-24 16:49:19 PDT
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)
Comment 8 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.
Eric Seidel (no email)
Comment 9 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?
Eric Seidel (no email)
Comment 10 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
Eric Seidel (no email)
Comment 11 2010-08-24 17:04:48 PDT
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)
Comment 12 2010-08-24 17:29:57 PDT
Added one. Will commit.
Eric Seidel (no email)
Comment 13 2010-08-24 17:30:58 PDT
Folks releasing from a branch may want this change. Should be easy to pick up.
Eric Seidel (no email)
Comment 14 2010-08-24 17:40:29 PDT
Eric Seidel (no email)
Comment 15 2010-08-24 21:34:09 PDT
David Kilzer (:ddkilzer)
Comment 16 2010-09-16 17:04:59 PDT
Lucas Forschler
Comment 17 2019-02-06 09:03:38 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note You need to log in before you can comment on or make changes to this bug.