586321 - Crash in TabChildGlobal due to mListenerManager not Disconnected()

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

Attachment: Patch

mListenerManager must have Disconnect() called on manually, if it is not part of a cycle (somewhat ironically).

So: If you do not call addEventListener, there is no need for Disconnect - so no crash. If you *do* call addEventListener, there will be a cycle, and again no crash. If you also call removeEventListener like a good citizen, the cycle is broken and manual Disconnect is needed, so you will crash with

###!!! ASSERTION: didn't call Disconnect: '!mTarget', file /scratchbox/users/alon/home/alon/mozilla-central/content/events/src/nsEventListenerManager.cpp, line 297

Comment 1

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

Comment on attachment 464854 [details] [diff] [review]
Patch

Could you call it in nsInProcessTabChildGlobalnsInProcessTabChildGlobal:Disconnect()

and perhaps when TabChild disconnects itself from TabChildGlobal.

Comment 2

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

Comment on attachment 464854 [details] [diff] [review]
Patch

Well, ok, let's do this way, and change *if* there are some
memory leaks related to this.
Though, cycle collector really should take of all the leaks.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 464854 [details] [diff] [review]
Patch

This shouldn't be checkin-needed yet -- it needs approval (or blocking) status before it can be checked in.

Requesting approval to get that ball rolling... (feel free to request blocking instead if that's more appropriate)

Comment 4

[Alon Zakai (:azakai)]

Thanks, I didn't know that stuff.

As it turns out, this isn't crucial anymore (I thought it was blocking a beta blocker - but found a way around that). This patch will only be important once we start doing removeEventListeners in frame scripts. So this can wait for now, no real need for approval or blocking.

Comment 5

[Alon Zakai (:azakai)]

We should push this after 4.0 I think. Is there a whiteboard value for such things?

Comment 6

[Doug Turner (:dougt)]

why?  what risk is there?

Comment 7

[Alon Zakai (:azakai)]

I dunno, but >0 I think ;) while the benefit is 0 (for now).

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 464854 [details] [diff] [review]
Patch

clearing approval-2.0 based on feedback.  benjamin, if you feel otherwise, please reset.

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/390f32bd1b8a