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

VERIFIED FIXED

Status

()

P2
critical
VERIFIED FIXED
11 years ago
4 years ago

People

(Reporter: jruderman, Assigned: jruderman)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments)

(Assignee)

Description

11 years ago
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.
Flags: blocking1.9?
(Assignee)

Comment 1

11 years ago
Created attachment 307180 [details]
testcase (can cause Firefox to crashing during cycle collection)
(Assignee)

Comment 2

11 years ago
Created attachment 307181 [details]
crash stack trace
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?
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 4

11 years ago
Created attachment 307329 [details] [diff] [review]
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).
Assignee: nobody → jruderman
Status: NEW → ASSIGNED
Attachment #307329 - Flags: review?(jonas)
Attachment #307329 - Flags: superreview+
Attachment #307329 - Flags: review?(jonas)
Attachment #307329 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Comment 5

11 years ago
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsNodeUtils::LastRelease]
Group: core-security
You need to log in before you can comment on or make changes to this bug.