Closed Bug 447764 Opened 12 years ago Closed 12 years ago
Investigate if event listeners aren't kept alive long enough
Bug 333078 changed event listener manager a bit and removed one nsRefPtr, which looks pretty important to me. http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/events/src&command=DIFF_FRAMESET&file=nsEventListenerManager.cpp&rev2=1.253&rev1=1.252 If event is dispatched via DispatchToInterface, the listener is kept alive because of the addrefing QueryInterface. However to usual case is to use HandleEventSubType and there addrefing doesn't happen. Any idea why bug 333078 removed: nsRefPtr<nsIDOMEventListener> listener = ls->mListener.Get(); ?
The name of the variable should make it clear why it is there. (And note, with event listeners nsRefPtr is needed, not nsCOMPtr. That is, IIRC, because of the interface dispatch thingie, see DispatchToInterface) I'm hoping someone proves this a false alarm... And thanks to roc asking whether listeners are kept alive. I knew there was the nsRefPtr when ELM was cleaned-up.
Comment on attachment 331078 [details] [diff] [review] keep listeners alive Looks ok, but can you write a mochitest for this? Just removing the listener while in dispatch and then triggering gc should lead to issues, right?
I think this needs that the listener is implemented in C++. JS engine should keep functions (== event listeners) alive.
Even if they're anonymous functions and even if you gc?
I'm not sure if the testcase should be pushed before the fix has landed to 1.9.0 branch too. But anyway, the testcase does show that we do have a problem and that the patch does fix it.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Olli, if this patch is good for the 1.9 branch can you please request approval on it? We'll need to get it approved and landed in the next 30 hours if you want it (and I think you do because I do too! ;) ). fwiw, the test can land after we've fixed this in 1.9.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Comment on attachment 331078 [details] [diff] [review] keep listeners alive Approved for 220.127.116.11. Please land in CVS. a=ss
Attachment #331078 - Flags: approval18.104.22.168? → approval22.214.171.124+
Probably need an sg marker here too.
Flags: blocking126.96.36.199? → blocking188.8.131.52+
I checked in the test, but OSX/qm-moz2mini01 didn't like it for some reason. It did compile and run fine on some other tboxes (also OSX). I don't have a mac, so hard to find the reason for the problem.
Yeah, running it over here works fine too. Is the box in question even on the Firefox tinderbox? I don't see it there... (That is, could the issue be with the code the box is building, not with your code or the test itself?)
It is there, or at least was there. There was also something strange happening with qm-win2k3-moz2-01, first it managed to compile and run the test but next time it couldn't compile it anymore.
You need to log in before you can comment on or make changes to this bug.