Closed Bug 586321 Opened 10 years ago Closed 9 years ago

Crash in TabChildGlobal due to mListenerManager not Disconnected()

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: azakai, Assigned: azakai)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
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)
Blocks: 550936
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+
Keywords: checkin-needed
Assignee: nobody → azakai
No longer blocks: 550936
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.
Keywords: checkin-needed
Attachment #464854 - Flags: approval2.0? → approval2.0+
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+
http://hg.mozilla.org/mozilla-central/rev/390f32bd1b8a
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.