420835 - Crash [@ nsNodeUtils::LastRelease] notifying a deleted nsXBLContentSink

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Steps to reproduce:
1. Load the testcase as a local text/html file.
2. Let it reload itself a few times.
3. Quit Firefox.

Result: About 30% of the time, nsNodeUtils::LastRelease notifies a deleted nsIMutationListener, usually a former nsXBLContentSink.  This happens during the Unroot part of cycle collection.

In a debug build, this bug shows up as

###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../../dist/include/xpcom/nsCOMPtr.h, line 594

where mRawPtr is the deleted nsXBLContentSink, and query_result is null because the object's vtable pointer changed to that of nsContentSink during destruction.

If you're running with MallocScribble enabled, Firefox crashes trying to dereference 0x55555559 instead of asserting.

I think the problem is that the cycle collection Unlink method for nsContentSink nulls out the mDocument member, depriving nsXMLContentSink's destructor of a chance to call mDocument->removeObserver(this).

nsContentSink was added to cycle collection as part of bug 386769.

One possible solution is to make it the responsibility of the subclass to have mDocument unlinked, so that subclasses that want to do things like mDocument->removeObserver(this) can do so.  Another possibility is to change NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED to call the superclass's unlink last instead of first.

Comment 1

[Jesse Ruderman]

Attachment: testcase (can cause Firefox to crashing during cycle collection)

Comment 2

[Jesse Ruderman]

Attachment: crash stack trace

Comment 3

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

Doh! Looked for "unlink" in nsContentSink.cpp, but it's implemented using a more complete macro..

We should move more of the mutationobserver stuff into nsContentSink, including making the nsContentSink unlink method call RemoveObserver. And make nsContentSink::Init call AddObserver, it's stupid that the subclasses do that when it's nsContentSink that actually is the one implementing BeginUpdate etc.

Though, it seems like calling the super classes unlink method first seems like a bad idea. It's the reverse of how dtors work.

Jesse, you want to write a patch for this or should I?

Comment 4

[Jesse Ruderman]

Attachment: patch v1

This patch moves the RemoveObserver call to the base class destructor and makes Unlink call RemoveObserver as well.  Not all nsContentSinks call AddObserver, so I didn't move the AddObserver calls to the base class.  (Having AddObserver in one class and RemoveObserver in another class doesn't seem ideal, but hopefully it's ok.)

This patch fixes the crash.  Note that you'll need to turn on MallocScribble to test with the patch, since the part of the patch that moves the QI stuff to the base class silences the "QueryInterface needed" assertion (when the memory is not reused after being freed).

Comment 5

[Jesse Ruderman]

Patch checked in.

Comment 6

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032615 Firefox/3.0b5pre - no crash on the testcase from jesse 

--> Verified fixed