Last Comment Bug 293347 - XSLT with document.write crashes [@ nsScrollPortView::~nsScrollPortView]
: XSLT with document.write crashes [@ nsScrollPortView::~nsScrollPortView]
Status: RESOLVED FIXED
: [sg:critical?]
: crash, hang, testcase, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: XSLT
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla1.9.2
Assigned To: Jonas Sicking (:sicking)
: xslt
: http://www.digi21.net/digi3d/internax...
: 202765
:
  Show dependency treegraph
 
Reported: 2005-05-08 04:56 PDT by Jose Angel
Modified: 2010-06-27 22:54 PDT (History)
14 users (show)
jst: blocking1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsScrollPortView::~nsScrollPortView]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta3-fixed
  .6+
  .6-fixed


Attachments
jesse's stack trace (4.35 KB, text/plain)
2009-09-13 12:16 PDT, Jesse Ruderman
no flags Details
testcase from URL (1.01 KB, text/xml)
2009-09-16 15:31 PDT, Brandon Sterne (:bsterne)
no flags Details
testcase stylesheet (3.09 KB, application/xml)
2009-09-16 15:33 PDT, Brandon Sterne (:bsterne)
no flags Details
Patch to fix (5.05 KB, patch)
2009-11-05 18:48 PST, Jonas Sicking (:sicking)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Now with tests (8.49 KB, patch)
2009-11-05 20:58 PST, Jonas Sicking (:sicking)
peterv: review+
Details | Diff | Splinter Review
191 branch patch (9.53 KB, patch)
2009-11-10 21:55 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Splinter Review
191 branch patch (9.54 KB, patch)
2009-11-10 21:59 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Splinter Review
191 branch patch (9.55 KB, patch)
2009-11-10 22:10 PST, Jonas Sicking (:sicking)
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
1.9.0 branch patch (10.89 KB, patch)
2009-11-13 15:18 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Splinter Review
Another 1.9.0 patch (8.56 KB, text/plain)
2009-11-16 18:51 PST, Jonas Sicking (:sicking)
samuel.sidler+old: approval1.9.0.16+
Details

Summon comment box

Description Jose Angel 2005-05-08 04:56:39 PDT
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
Comment 1 Ben Fowler 2005-05-08 07:09:18 PDT
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"
Comment 2 Sven 2005-07-18 16:11:55 PDT
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.
Comment 3 spam_hole 2005-08-07 03:59:16 PDT
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.
Comment 4 Samuel Sidler (old account; do not CC) 2007-04-30 15:20:06 PDT
Are any of you seeing these crashes using Firefox 2.0.0.3 or later with a new profile?
Comment 5 Ezra 2009-08-12 15:49:17 PDT
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
Comment 6 Jesse Ruderman 2009-09-13 12:10:14 PDT
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
Comment 7 Jesse Ruderman 2009-09-13 12:16:51 PDT
Created attachment 400376 [details]
jesse's stack trace
Comment 8 Jesse Ruderman 2009-09-13 12:17:23 PDT
See also bug 202765.
Comment 9 Daniel Veditz 2009-09-13 14:12:06 PDT
Jonas: please find an appropriate owner for this one
Comment 10 Daniel Veditz 2009-09-13 14:15:39 PDT
See also bug 202765 comment 39, could "fix" these by disabling document.write in XSLT rather than patching the crash itself.
Comment 11 Brandon Sterne (:bsterne) 2009-09-16 15:31:05 PDT
Created attachment 401110 [details]
testcase from URL

Preserving original testcase.
Comment 12 Brandon Sterne (:bsterne) 2009-09-16 15:33:55 PDT
Created attachment 401111 [details]
testcase stylesheet
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2009-09-22 18:12:24 PDT
Jonas, is this something that you need to look into, or should this go over to layout?
Comment 14 Jonas Sicking (:sicking) 2009-09-22 22:55:10 PDT
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.
Comment 15 Brandon Sterne (:bsterne) 2009-10-05 09:31:05 PDT
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?
Comment 16 Damon Sicore (:damons) 2009-11-03 14:03:49 PST
Jonas?  Any progress on the fix you proposed?
Comment 17 Jonas Sicking (:sicking) 2009-11-04 13:22:34 PST
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.
Comment 18 Jonas Sicking (:sicking) 2009-11-05 18:48:50 PST
Created attachment 410705 [details] [review]
Patch to fix

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.
Comment 19 Jonas Sicking (:sicking) 2009-11-05 18:49:19 PST
Err.. and just imagine that I actually changed the nsIHTMLDocument IID :)
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-05 18:56:53 PST
Comment on attachment 410705 [details] [review]
Patch to fix

r+sr=jst with the IID actually changed! :)
Comment 21 Jonas Sicking (:sicking) 2009-11-05 19:01:07 PST
Comment on attachment 410705 [details] [review]
Patch to fix

Peter, could you have a look at this before I land this on branch as well?
Comment 22 Jonas Sicking (:sicking) 2009-11-05 20:58:10 PST
Created attachment 410719 [details] [review]
Now with tests
Comment 23 Jonas Sicking (:sicking) 2009-11-05 20:59:27 PST
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
Comment 24 Jonas Sicking (:sicking) 2009-11-09 12:20:05 PST
Checked in to 1.9.2 branch:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8ecead83349e
Comment 25 Ezra 2009-11-09 12:26:57 PST
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 26 Daniel Veditz 2009-11-09 14:56:16 PST
Comment on attachment 410719 [details] [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?
Comment 27 Jonas Sicking (:sicking) 2009-11-09 14:59:20 PST
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?
Comment 28 Samuel Sidler (old account; do not CC) 2009-11-09 16:33:08 PST
Yes, please.
Comment 29 Jonas Sicking (:sicking) 2009-11-10 21:55:37 PST
Created attachment 411621 [details] [review]
191 branch patch
Comment 30 Jonas Sicking (:sicking) 2009-11-10 21:59:46 PST
Created attachment 411622 [details] [review]
191 branch patch

This one actually builds
Comment 31 Jonas Sicking (:sicking) 2009-11-10 22:10:07 PST
Created attachment 411623 [details] [review]
191 branch patch

Ok, really really builds now
Comment 32 Daniel Veditz 2009-11-10 22:11:54 PST
Comment on attachment 411623 [details] [review]
191 branch patch

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 33 Jonas Sicking (:sicking) 2009-11-10 22:22:30 PST
Checked in to 1.9.1 branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/335158a486fe
Comment 34 Jonas Sicking (:sicking) 2009-11-11 00:30:58 PST
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
Comment 35 Daniel Veditz 2009-11-13 11:12:10 PST
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.
Comment 36 Jonas Sicking (:sicking) 2009-11-13 15:18:44 PST
Created attachment 412316 [details] [review]
1.9.0 branch patch

This *should* work, but I can't test locally. Running a tryserver build right now.
Comment 37 Samuel Sidler (old account; do not CC) 2009-11-16 09:09:46 PST
Jonas: How did your tryserver build go?
Comment 38 Jonas Sicking (:sicking) 2009-11-16 18:51:05 PST
Created attachment 412752 [details]
Another 1.9.0 patch

I think this'll work better
Comment 39 Jonas Sicking (:sicking) 2009-11-16 19:04:51 PST
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
Comment 40 Al Billings [:abillings] 2009-11-20 16:32:15 PST
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).

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