240592 - crash when script element inserted into DOM that removes its parent [@ nsGenericElement::InsertChildAt ]

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Mazen Turkmani]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322)
Build Identifier: 

Each time I try and load the page http://www.elitefx.com/firefox_bugs/order.xml 
it crashes. Works fine in IE. Says there is a memory access violation each time 
is loaded in Firefox on Windows 2000 Pro.

Reproducible: Always
Steps to Reproduce:
1. Open the page through Firefox in Windows 2000 Pro
2.
3.

Actual Results:  
The page crashed and could only be closed or debugged.

Expected Results:  
The software should of generated an xml product list that displays images using 
javascript from an xsl file.

Application error - Memory cannot be "read"

=================

about:buildconfig

Build platform
target
i586-pc-msvc

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	12.00.8804 for 80x86 	-TC -nologo -W3 -nologo -Gy -
Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	12.00.8804 for 80x86 	-TP -nologo -W3 -nologo -Gy -
Fd$(PDBFILE)

Configure arguments
--disable-tests --disable-debug --enable-optimize --disable-shared --enable-
static --enable-crypto --disable-composer --disable-ldap --disable-mailnews --
disable-profilesharing --enable-extensions=cookie,xml-
rpc,xmlextras,p3p,pref,transformiix,universalchardet,typeaheadfind,webservices,i
nspector --enable-static --disable-shared

Comment 1

[Mazen Turkmani]

Same thing happens in Mozilla 1.6, so I guess it will probably happen in all
Mozilla based products.

Comment 2

[Xanthor Sccas]

Same think with my linux build
So OS -> All ?

Comment 3

[Mazen Turkmani]

(In reply to comment #2)
> Same think with my linux build
> So OS -> All ?

I also noticed that if the javascript function is reference from a file rather
than having in all in the xsl file, the page loads up blank. I guess the whole
area of using javascript with xsl is not been fully implemented just yet.

Comment 4

[Adam Hauner]

Not Firefox specific, confirming using 2004041408/trunk/W2K -> TB19696M, TB19695W
-> NEW, Browser

Comment 5

[Boris Zbarsky [:bzbarsky]]

Adam, please do NOT confirm layout bugs in browser-general and without minimal
testcases!  That's the best way to make sure no one ever looks at the bug again.

When I load that page in a debug build, I get:

WARNING: NS_ENSURE_TRUE(aContainer) failed, file
/home/bzbarsky/mozilla/xlib/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 2115
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/xlib/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 1995
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/xlib/mozilla/content/html/document/src/nsHTMLDocument.cpp,
line 2115
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/xlib/mozilla/content/html/document/src/nsHTMLDocument.cpp,
line 2170
JavaScript error: , line 0: uncaught exception: [Exception... "Component
returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER)
[nsIDOMNSHTMLDocument.writeln]"  nsresult: "0x80004003
(NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
http://www.elitefx.com/firefox_bugs/order.xml :: show_image :: line 18"  data: no]

Then I crash:

#0  0x413c2856 in nsGenericElement::InsertChildAt(nsIContent*, unsigned, int, int) (
    this=0x8897a60, aKid=0x8897b80, aIndex=0, aNotify=1, aDeepSetDocument=1)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/base/src/nsGenericElement.cpp:2490
#1  0x413c3702 in nsGenericElement::doInsertBefore(nsIContent*, nsIDOMNode*,
nsIDOMNode*, nsIDOMNode**) (aElement=0x8897a60, aNewChild=0x8897ba0, aRefChild=0x0, 
    aReturn=0xbfffe630)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/base/src/nsGenericElement.cpp:2817
#2  0x4139ad23 in nsGenericElement::AppendChild(nsIDOMNode*, nsIDOMNode**) (
    this=0x8897a60, aNewChild=0x8897ba0, aReturn=0xbfffe630) at
nsGenericElement.h:498
#3  0x414e3ab9 in nsHTMLTableCellElement::AppendChild(nsIDOMNode*, nsIDOMNode**) (
    this=0x8897a60, aNewChild=0x8897ba0, aReturn=0xbfffe630)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/html/content/src/nsHTMLTableCellElement.cpp:64
#4  0x429985de in txMozillaXMLOutput::endElement(nsAString const&, int)
(this=0x8396088, 
    aName=@0xbfffe720, aNsID=0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp:298
#5  0x42966a70 in txEndElement::execute(txExecutionState&) (this=0x86276f0, 
    aEs=@0xbfffe850)
    at
/home/bzbarsky/mozilla/xlib/mozilla/extensions/transformiix/source/xslt/txInstructions.cpp:484
#6  0x4298ba72 in txXSLTProcessor::execute(txExecutionState&) (aEs=@0xbfffe850)
    at
/home/bzbarsky/mozilla/xlib/mozilla/extensions/transformiix/source/xslt/txXSLTProcessor.cpp:98
#7  0x4299fd51 in txMozillaXSLTProcessor::DoTransform() (this=0x883c7b0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp:375
#8  0x429a0e23 in txMozillaXSLTProcessor::setStylesheet(txStylesheet*)
(this=0x883c7b0, 
    aStylesheet=0x883c920)
    at
/home/bzbarsky/mozilla/xlib/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp:667
#9  0x4298f0d2 in txCompileObserver::onDoneCompiling(txStylesheetCompiler*,
unsigned, unsigned short const*, unsigned short const*) (this=0x883af60,
aCompiler=0x883c818, 
    aResult=0, aErrorText=0x0, aParam=0x0)


(gdb) frame 0
#0  0x413c2856 in nsGenericElement::InsertChildAt(nsIContent*, unsigned, int, int) (
    this=0x8897a60, aKid=0x8897b80, aIndex=0, aNotify=1, aDeepSetDocument=1)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/base/src/nsGenericElement.cpp:2490
2490            mDocument->ContentAppended(this, aIndex);
(gdb) p mDocument
$1 = (class nsIDocument *) 0x0

The relevant code is:

2486   if (mDocument) {
2487     aKid->SetDocument(mDocument, aDeepSetDocument, PR_TRUE);
2488     if (aNotify) {
2489       if (isAppend) {
2490         mDocument->ContentAppended(this, aIndex);
2491       } else {
2492         mDocument->ContentInserted(this, aKid, aIndex);
2493       }

The problem is that the script executes during that SetDocument() call onthe
child.  This is not a problem in AppendChildTo when coming from the sink,
because there the sink will set the kid's mDocument up front and when we
SetParent the script will execute.

In any case, I assume that the script removes this node from the tree, so
mDocument becomes null.  We probably need to re-null-check mDocument after the
SetDocument() call here.... (and I'd say similarly in AppendChildTo).

That said, I'm not sure what those errors up front are about....

Comment 6

[Adam Hauner]

Attachment: XSL file

Comment 7

[Adam Hauner]

Attachment: minimal XSL file

Comment 8

[Adam Hauner]

Attachment: testcase

Comment 9

[Adam Hauner]

Boris: Could you test, if minimal tescase is still crashing same way?
BTW 1.7b is crashing on this too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, that's cute.  That explains all the warnings, then.  The problem is that
document.writeln is called, but the document is not loading as far as it knows
(no parser, since it's generated by XSLT!) so that calls document.open() and
wipes out the entire document (as it should, if this were being called in an
already-loaded document).  Then the crash ensues, of course.

So in addition to doing the bullet-proofing in nsGenericElement, we should
consider ways of not having this document clobber itself in this circumstance. 
Or just not support document.write/document.writeln in XSLT-generated documents.

sicking, thoughts?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

document.write is currently not supported for XSLT documents. Most of the time
there are alternatives though, such as using .innerHTML.

*** This bug has been marked as a duplicate of 202765 ***

Comment 12

[Boris Zbarsky [:bzbarsky]]

Reopening.  You can reproduce this crash without document.write and XSLT, if
desired; see what I said in comment 5.  The XSLT issue is totally incidental. 
This is why I left the bug in the DOM component.

Nice quirks-mode HTML testcase with no hint of XML or XSLT coming up.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Crash testcase, no XSLT

Comment 14

[Boris Zbarsky [:bzbarsky]]

So bug 202765 can focus on not clobbering the document, I guess, but this bug
still needs to be fixed in the DOM code as I outlined in comment 5.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Along these lines

I'm not quite happy with this.	See the XXX comments.

Comment 16

[Boris Zbarsky [:bzbarsky]]

In my mind, the only really correct way to handle this is to make sure the
script does not execute till after the ContentAppended/ContentInserted
notifications (and probably the DOM events) have fired.  Otherwise document
observers will just be confused (content being removed that was never in the DOM
as far as they're concerned, etc).

Perhaps one way to do that is to move the ContentAppended/ContentInserted firing
inside BindToTree() (which we then need).  That way we could ensure that we're
properly inserted before firing and all.

Comment 17

[Adam Hauner]

(no entry for both incidents, removing them)

Comment 18

[timeless]

*** Bug 257650 has been marked as a duplicate of this bug. ***

Comment 19

[Daniel de Wildt]

*** Bug 266888 has been marked as a duplicate of this bug. ***

Comment 20

[Frank Wein [:mcsmurf]]

Might it that this bug has been resolved? None of the testcases crashes for me
anymore with a current trunk build.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The patch to bug 27382 made it so that we get the document pointer up front in
this function instead of using mDocument throughout.  That prevents this exact
crash, but it still delivers a ContentAppended notification for a node that has
already been removed.  I bet I can write a more complicated testcase involving a
content list that will crash (because it will be holding a weak ptr to a node
that has already been destroyed).

The underlying problem is still here, as described in comment 5 and comment 16.

Comment 22

[Olivier Cahagne]

*** Bug 279612 has been marked as a duplicate of this bug. ***

Comment 23

[timeless]

*** Bug 293633 has been marked as a duplicate of this bug. ***

Comment 24

[Ronald Tilby]

Testcases WFM using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060601 Minefield/3.0a1 ID:2006060105 [cairo]

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ronald, see comment 21.

Comment 26

[Boris Zbarsky [:bzbarsky]]

This might just be worksforme now, if scripts can no longer run sync from under BindToTree, right?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I can't think of any case anymore where scripts can run under BindToTree