Closed Bug 293347 Opened 20 years ago Closed 16 years ago

XSLT with document.write crashes [@ nsScrollPortView::~nsScrollPortView]

Categories

(Core :: XSLT, defect, P2)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2
Tracking Flags:
Tracking Status
status1.9.2 --- beta3-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: joseangel, Assigned: sicking)

References

(
URL
)

Details

(5 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(6 files, 4 obsolete files)

jesse's stack trace
4.35 KB, text/plain
Details
testcase from URL
1.01 KB, text/xml
Details
testcase stylesheet
3.09 KB, application/xml
Details
Now with tests
8.49 KB, patch
peterv
: review+
Details | Diff | Splinter Review
191 branch patch
9.55 KB, patch
dveditz
: approval1.9.1.6+
Details | Diff | Splinter Review
Another 1.9.0 patch
8.56 KB, text/plain
samuel.sidler+old
: approval1.9.0.16+
Details
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.7.7) Gecko/20050414 Firefox/1.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.7.7) Gecko/20050414 Firefox/1.0.3 Openins this site with my firefox, it crashes automatically. Reproducible: Always
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050503 Firefox/1.0+ I appear to get a hang rather than a crash. ###!!! ASSERTION: mCurrentNode is NULL: 'mCurrentNode', file ../../../../../../src/extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp, line 386 ###!!! ASSERTION: mCurrentNode is NULL: 'mCurrentNode', file ../../../../../../src/extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp, line 465 ###!!! ASSERTION: mCurrentNode is NULL: 'mCurrentNode', file ../../../../../../src/extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp, line 231 ###!!! ASSERTION: initial containing block already created: 'nsnull == mInitialContainingBlock', file ../../../../src/layout/base/nsCSSFrameConstructor.cpp, line 8964 ###!!! ASSERTION: Unexpected child of document element containing block: 'mDocElementContainingBlock->GetFirstChild(nsnull) == nsnull', file ../../../../src/layout/base/nsCSSFrameConstructor.cpp, line 8992 ###!!! ASSERTION: already have a child frame: 'mFrames.IsEmpty()', file ../../../../src/layout/generic/nsHTMLFrame.cpp, line 274 ###!!! ASSERTION: unexpected next reflow command frame: '*iter == mFrames.FirstChild()', file ../../../../src/layout/generic/nsHTMLFrame.cpp, line 481 You might want to review Bug 226425 "xml, xslt crash using apply-templates and document"
Reproduced on two WindowsXP and one Linux machine. I found another URL, that crashes Firefox (recent version 1.0.5): http://de.selfhtml.org/xml/darstellung/anzeige/xsltext.xml This example is quite simpler than the other one: === xmltext.xml === <?xml version="1.0" encoding="ISO-8859-1"?> <?xml-stylesheet type="text/xsl" href="xsltext.xsl" ?> <test> <behauptung>Die Welt ist ein Dorf</behauptung> </test> === xsltext.xsl === <?xml version="1.0" encoding="iso-8859-1"?> <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <html> <head> </head> <body> <xsl:apply-templates /> </body> </html> </xsl:template> <xsl:template match="behauptung"> <p><xsl:value-of select="." /></p> <script type="text/javascript"> <xsl:text> &lt;!-- document.write(&quot;&lt;p&gt;&quot; + document.lastModified + &quot;&lt;/p&gt;&quot;); //--&gt; </xsl:text> </script> </xsl:template> </xsl:stylesheet> After a quick look at both transformation sheets, i recognize that both are using inner Javascript - especially "document.write()" - which might crashes Firefox immediately.
Got another example, I think I know how this works now. I can produce two behaviours with slightly different XSL documents: ----Data.XML---- <?xml version="1.0"?> <?xml-stylesheet type="text/xsl" href="crash.xsl"?> <blank>3</blank> ---------------- #1: This causes FF to simply not load the page, instead it just sits there and attempts to load it forever (does not produce crash) ----crash.xsl---- <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <html><script language="javascript">document.write(1)</script></html> </xsl:template> </xsl:stylesheet> --------------- #2: This is the problematic one. This crashes FF 1.0.6 on both Windows and Mac OS X, every time. ----crash.xsl---- <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <html><body><script language="javascript">document.write(1)</script></body></html> </xsl:template> </xsl:stylesheet> --------------- The only difference between the two is the second contains a <body> tag around the script; the first does not. Expected results: A single "1" is rendered Actual results: FF crashes. Crash only occurs when document.write is used, not in any other circumstance. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 These are the two User-Agents that I have used to test, both with the same reults as described above.
Are any of you seeing these crashes using Firefox 2.0.0.3 or later with a new profile?
Keywords: crash, hang
Both url's seem to keep loading without finishing, but they don't hang or crash firefox. Tested with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090812 Minefield/3.6a2pre ID:20090812045356
I get a crash on Mac when I load http://www.digi21.net/digi3d/internaxml/1/1.inn.xml and then press the back button. ###!!! ASSERTION: root element frame already created: 'nsnull == mRootElementFrame', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 6559 ###!!! ASSERTION: Shouldn't have a doc element containing block here: '!mDocElementContainingBlock', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 2448 ###!!! ASSERTION: unexpected second call to SetInitialChildList: 'Not Reached', file /Users/jruderman/central/layout/generic/nsContainerFrame.cpp, line 111 ###!!! ASSERTION: Already have an undisplayed context entry for aContent: '!GetUndisplayedContent(aContent)', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 593 ###!!! ASSERTION: unexpected mRootElementFrame: 'processChildren ? !mRootElementFrame : mRootElementFrame == contentFrame', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 2636
Group: core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Summary: Firefox crashes when open xml with associated xsl → Firefox crashes when open xml with associated xsl [@ nsScrollPortView::~nsScrollPortView]
Component: General → XSLT
Product: Firefox → Core
QA Contact: general → xslt
Summary: Firefox crashes when open xml with associated xsl [@ nsScrollPortView::~nsScrollPortView] → XSLT with document.write crashes [@ nsScrollPortView::~nsScrollPortView]
Version: unspecified → Trunk
Whiteboard: [sg:critical?]
Attached file jesse's stack trace —
See also bug 202765.
Depends on: 202765
Jonas: please find an appropriate owner for this one
Assignee: nobody → jonas
blocking1.9.1: --- → ?
status1.9.1: --- → wanted
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
See also bug 202765 comment 39, could "fix" these by disabling document.write in XSLT rather than patching the crash itself.
blocking1.9.1: ? → needed
Attached file testcase from URL —
Preserving original testcase.
Jonas, is this something that you need to look into, or should this go over to layout?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
I don't know off the top of my head what's going on here. I suspect the world is different enough from 2005 when this bug was filed that we're now seeing different bugs. I don't actually think making document.write a no-op fixes any actual security problems as you can basically replicate the same behavior by removing all nodes from the document. What we used to have a problem with was layout starting while still displaying the source XML document. Layout (and potentially other) code would then fail when the XSLT code was done running and we tried to swap in the XSLT result instead of the old source XML document. Something about the DocumentViewerImpl::SetDOMDocument not being completely able to swap away a document where layout had already started. Unfortunately I don't really know the code involved off the top of my head. But I suspect the safest fix would be to change DocumentViewerImpl::SetDOMDocument to bail if layout has already started on the current document.
So who should own this bug? Jonas, if you're not the right guy to attempt the fix you suggested in comment 14, can we identify that person?
blocking1.9.1: needed → .5+
Jonas? Any progress on the fix you proposed?
Working on another bug, but hopefully should have something end of this week or early next. Simply blocking the call to SetDOMDocument when layout has started should not be too hard.
Keywords: testcase
Attached patch Patch to fix (obsolete) — — Splinter Review
So the problem is this: If script in the stylesheet calls document.write, the document.write implementation will actually do a whole lot more than just nuke the current document. It will set up a new parser and new content sink and then stream data to these. The new content sink will start layout on the now-empty document. (all of this happens because the result document of an xslt transformation doesn't have a parser, which it shouldn't). This is just scary in and of itself since it means that we have two content sinks messing around with the result document. One that was used to load the original XML file, and one that is used for the document.write. And sure enough, things fail once we're done transforming, since then the XML content sink tries to start layout on the result document. However the document.write content sink has already done so. That is what produces all the layout assertions. Fix is to simply disable document.write for documents created as a result of an XSLT transformation.
Attachment #410705 - Flags: superreview?(jst)
Attachment #410705 - Flags: review?(jst)
Err.. and just imagine that I actually changed the nsIHTMLDocument IID :)
Comment on attachment 410705 [details] [diff] [review] Patch to fix r+sr=jst with the IID actually changed! :)
Attachment #410705 - Flags: superreview?(jst)
Attachment #410705 - Flags: superreview+
Attachment #410705 - Flags: review?(jst)
Attachment #410705 - Flags: review+
Comment on attachment 410705 [details] [diff] [review] Patch to fix Peter, could you have a look at this before I land this on branch as well?
Attachment #410705 - Flags: review?(peterv)
Attached patch Now with tests — — Splinter Review
Attachment #410705 - Attachment is obsolete: true
Attachment #410719 - Flags: review?(peterv)
Attachment #410705 - Flags: review?(peterv)
Landed this on trunk. Would still be great to get review from peterv before landing on branch. http://hg.mozilla.org/mozilla-central/rev/a8365a42e185
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?][needs review peterv]
Attachment #410719 - Flags: review?(peterv) → review+
Checked in to 1.9.2 branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8ecead83349e
status1.9.2: --- → final-fixed
Whiteboard: [sg:critical?][needs review peterv] → [sg:critical?]
Both urls load and work fine now. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091109 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091109043812
Comment on attachment 410719 [details] [diff] [review] Now with tests >+++ b/content/html/document/src/nsIHTMLDocument.h >-// 5a959364-a2f4-4cac-9a2c-957055dc3569 >+// 56ff0e81-191c-421c-b75c-1727e13091c0 > #define NS_IHTMLDOCUMENT_IID \ Can we change the nsIHTMLDocument interface on branches? Does having it in a .h file make it "internal" enough to not worry about?
Ah, right, I'll have to do a nsIHTMLDocument_1_9_1_BRANCH interface. Want me to write up a patch for that before giving approval?
Attachment #410719 - Flags: approval1.9.1.6?
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.1 patch]
Attached patch 191 branch patch (obsolete) — — Splinter Review
Attachment #411621 - Flags: approval1.9.1.6?
Attached patch 191 branch patch (obsolete) — — Splinter Review
This one actually builds
Attachment #411621 - Attachment is obsolete: true
Attachment #411622 - Flags: approval1.9.1.6?
Attachment #411621 - Flags: approval1.9.1.6?
Attached patch 191 branch patch — — Splinter Review
Ok, really really builds now
Attachment #411622 - Attachment is obsolete: true
Attachment #411623 - Flags: approval1.9.1.6?
Attachment #411622 - Flags: approval1.9.1.6?
Comment on attachment 411623 [details] [diff] [review] 191 branch patch Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #411623 - Flags: approval1.9.1.6? → approval1.9.1.6+
Whiteboard: [sg:critical?][needs 1.9.1 patch] → [sg:critical?]
Had to land a followup fix for the 1.9.1 branch. HTML elements are in a different namespace there compared to later branches. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5405e8bb0e82
Flags: blocking1.9.0.17?
Flags: blocking1.9.0.16?
Jonas: now we need a 1.9.0 version of this fix since we're still supporting that and now the tree has a testcase for this bug. Hopefully not too different from the 1.9.1 version.
Flags: blocking1.9.0.17?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16+
Attached patch 1.9.0 branch patch (obsolete) — — Splinter Review
This *should* work, but I can't test locally. Running a tryserver build right now.
Jonas: How did your tryserver build go?
Attached file Another 1.9.0 patch —
I think this'll work better
Attachment #412316 - Attachment is obsolete: true
Attachment #412752 - Attachment mime type: application/octet-stream → text/plain
Got a verbal a=dveditz Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.796; previous revision: 3.795 done Checking in content/html/document/src/nsHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v <-- nsHTMLDocument.h new revision: 3.233; previous revision: 3.232 done Checking in content/html/document/src/nsIHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsIHTMLDocument.h,v <-- nsIHTMLDocument.h new revision: 3.75; previous revision: 3.74 done Checking in content/xml/document/src/nsXMLContentSink.cpp; /cvsroot/mozilla/content/xml/document/src/nsXMLContentSink.cpp,v <-- nsXMLContentSink.cpp new revision: 1.451; previous revision: 1.450 done Checking in content/xml/document/test/Makefile.in; /cvsroot/mozilla/content/xml/document/test/Makefile.in,v <-- Makefile.in new revision: 1.7; previous revision: 1.6 done RCS file: /cvsroot/mozilla/content/xml/document/test/file_bug293347.xml,v done Checking in content/xml/document/test/file_bug293347.xml; /cvsroot/mozilla/content/xml/document/test/file_bug293347.xml,v <-- file_bug293347.xml initial revision: 1.1 done RCS file: /cvsroot/mozilla/content/xml/document/test/file_bug293347xslt.xml,v done Checking in content/xml/document/test/file_bug293347xslt.xml; /cvsroot/mozilla/content/xml/document/test/file_bug293347xslt.xml,v <-- file_bug293347xslt.xml initial revision: 1.1 done RCS file: /cvsroot/mozilla/content/xml/document/test/test_bug293347.html,v done Checking in content/xml/document/test/test_bug293347.html; /cvsroot/mozilla/content/xml/document/test/test_bug293347.html,v <-- test_bug293347.html initial revision: 1.1
Attachment #412752 - Flags: approval1.9.0.16+
Verified for 1.9.0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729) using Brandon's testcase (and passing unit test). Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) using Brandon's testcase (and mochitest).
Keywords: fixed1.9.0.16 → verified1.9.0.16, verified1.9.1
Flags: wanted1.8.1.x+
Group: core-security
Flags: in-testsuite+
Crash Signature: [@ nsScrollPortView::~nsScrollPortView]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: