Closed Bug 586321 Opened 10 years ago Closed 9 years ago
Crash in Tab
Child Global due to m Listener Manager not Disconnected()
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
Attachment #464854 - Flags: review?(Olli.Pettay)
Comment on attachment 464854 [details] [diff] [review] Patch Could you call it in nsInProcessTabChildGlobalnsInProcessTabChildGlobal:Disconnect() and perhaps when TabChild disconnects itself from TabChildGlobal.
Attachment #464854 - Flags: review?(Olli.Pettay) → review-
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.
Attachment #464854 - Flags: review- → review+
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)
Attachment #464854 - Flags: approval2.0?
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.
We should push this after 4.0 I think. Is there a whiteboard value for such things?
why? what risk is there?
I dunno, but >0 I think ;) while the benefit is 0 (for now).
Comment on attachment 464854 [details] [diff] [review] Patch clearing approval-2.0 based on feedback. benjamin, if you feel otherwise, please reset.
Attachment #464854 - Flags: approval2.0+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.