447764 - Investigate if event listeners aren't kept alive long enough

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

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(); ?

Comment 1

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

Attachment: keep listeners alive

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 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

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

I think this needs that the listener is implemented in C++. JS engine should keep
functions (== event listeners) alive.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Even if they're anonymous functions and even if you gc?

Comment 5

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

Attachment: unittest

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.

Comment 6

[Samuel Sidler (old account; do not CC)]

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.

Comment 7

[Samuel Sidler (old account; do not CC)]

Comment on attachment 331078 [details] [diff] [review]
keep listeners alive

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 8

[Samuel Sidler (old account; do not CC)]

Probably need an sg marker here too.

Comment 9

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

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?)

Comment 11

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

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.