420429 - Crash [@ nsContentList::PopulateWith] with mutation event listener on window, cloned XML PI

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers a crash.  It appears exploitable.

The crash only happens if the mutation event listener is on |window|.  If it's on |document|, there is no crash.  The mutation event listener does not appear to be called, but it has to be installed in order to trigger the bug.

Sometimes, this assertion shows up before the crash:

###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObject())', file /Users/jruderman/trunk/mozilla/js/src/xpconnect/src/xpcinlines.h, line 662

Comment 1

[Jesse Ruderman]

Attachment: testcase

Oops, attached the wrong thing in comment 0.

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Mats Palmgren (inactive)]

Attachment: More stacks

The nsXMLProcessingInstruction constructor calls SetData():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xml/content/src/nsXMLProcessingInstruction.cpp&rev=1.77&root=/cvsroot&mark=84#78
which calls SetTextInternal() with aNotify=PR_TRUE but since we
do this from the constructor there is no strong ref yet so when
the nsMutationEvent object is destroyed when we leave the scope here
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsGenericDOMDataNode.cpp&rev=3.250&root=/cvsroot&mark=486#440
'this' is deleted.  The first stack shows the call to free(), the
second the crash stack.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Don't notify when setting the value from within the ctor. (and mute a warning)

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Alternative patch that still uses SetData() that notifies, but doing it
in a separate step after we have NS_ADDREF'ed the object.

Comment 6

[Daniel Veditz [:dveditz]]

On the 1.8 branch I don't crash, I just get

WARNING: waaah!, file c:/dev/ff2/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp, line 862

Comment 7

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

I sort of want to do both patch versions. I see no reason to notify as there can't be anything yet that listens to the notification. But it's in general a nice code cleanup to not have the "clonetext" argument.

Though now isn't really the time for cleanups, so maybe we should just take the rev 1 patch for now and land the other once we've branched for FF3.

Comment 8

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

This patch is ready to be landed, right?

Comment 9

[Mats Palmgren (inactive)]

Patch rev.1 + crashtest landed:

mozilla/content/base/src/nsCommentNode.cpp 	3.89
mozilla/content/xml/content/src/nsXMLProcessingInstruction.cpp 	1.78
mozilla/content/xml/content/crashtest/420429.xul 	1.1
mozilla/content/xml/content/crashtest/crashtests.list 	1.1
mozilla/testing/crashtest/crashtests.list 	1.32 

Filed bug 421034 on the parameter cleanup.

-> FIXED

Comment 10

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre as well as the Win XP nighty. No crash using the testcase.