Closed Bug 447764 Opened 15 years ago Closed 15 years ago

Investigate if event listeners aren't kept alive long enough


(Core :: DOM: Events, defect)

Not set





(Reporter: smaug, Assigned: smaug)



(Keywords: fixed1.9.0.2, regression, Whiteboard: [sg:critical?] maybe gc issue)


(2 files)

Bug 333078 changed event listener manager a bit and removed one nsRefPtr, which
looks pretty important to me.

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.
Assignee: nobody → Olli.Pettay
Attachment #331078 - Flags: superreview?(bzbarsky)
Attachment #331078 - Flags: review?(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?
Attachment #331078 - Flags: superreview?(bzbarsky)
Attachment #331078 - Flags: superreview+
Attachment #331078 - Flags: review?(bzbarsky)
Attachment #331078 - Flags: review+
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?
Attached patch unittestSplinter Review
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.
Closed: 15 years ago
Flags: wanted1.9.0.x?
Flags: in-testsuite?
Flags: blocking1.9.0.2?
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+
Attachment #331078 - Flags: approval1.9.0.2?
Comment on attachment 331078 [details] [diff] [review]
keep listeners alive

Approved for Please land in CVS. a=ss
Attachment #331078 - Flags: approval1.9.0.2? → approval1.9.0.2+
Probably need an sg marker here too.
Flags: blocking1.9.0.2? → blocking1.9.0.2+
Keywords: fixed1.9.0.2
Whiteboard: [sg:critical?] maybe gc issue
Group: core-security
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.
Flags: wanted1.8.1.x-
Keywords: regression
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.