323807 - event listener wrapper preservation can leave dangling nsIDOMGCParticipant in preserved wrapper table [@ ClassifyWrapper]

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think I only half-addressed bug 241518 comment 39:  I solved the problem of dangling wrappers, but not the problem of dangling nsIDOMGCParticipants.  At least I don't think I did.  This would cause a crash in ClassifyWrapper if the nsIDOMGCParticipant has been destroyed.

For the other cases (where the wrapper wraps the participant), we use the ReleaseWrapper call in nsEventReceiverSH::Finalize.

This is probably the cause of the crash in bug 323371 -- ClassifyWrapper shows up on the topcrash list.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm.  Now that I think about it some more, this would only actually crash if we leak the event listener manager, since otherwise we will be guaranteed to have made the appropriate ReleaseWrapper call in nsMarkedJSFunctionHolder_base::~nsMarkedJSFunctionHolder_base.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's also worth noting that the nsEventReceieverSH::Finalize (or nsDOMGCParticipantSH::Finalize after bug 323534) is the wrong place to fix this, since if the node's wrapper isn't preserved (but only the event listener), then the node's wrapper can be finalized while the node is still reachable.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This is still compiling; not tested yet, and may not compile.

I came to the conclusion that there's no general way to fix this, so I just need to document that GC participant destructors (or, more generally, the caller to PreserveWrapper) has to ensure that ReleaseWrapper happens before the participant goes away.

I did that by removing all event listeners before letting go of the event listener manager.  I also found a bunch of places that should have been calling SetListenerTarget(nsnull), but weren't, so I combined the two into Disconnect.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 208801 [details] [diff] [review]
patch

Seems reasonable to me.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 208801 [details] [diff] [review]
patch

sr=jst

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I had to land a very simple bustage fix to nsEventListenerManager.cpp (add "return" before the last line in Disconnect).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.