Investigate if event listeners aren't kept alive long enough

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({fixed1.9.0.2, regression})

Trunk
x86
All
Points:
---
Bug Flags:
blocking1.9.0.2 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] maybe gc issue)

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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(); ?
(Assignee)

Comment 1

11 years ago
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
Status: NEW → ASSIGNED
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+
(Assignee)

Comment 3

11 years ago
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?
(Assignee)

Comment 5

11 years ago
Posted 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.
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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+
(Assignee)

Updated

11 years ago
Attachment #331078 - Flags: approval1.9.0.2?
Comment on attachment 331078 [details] [diff] [review]
keep listeners alive

Approved for 1.9.0.2. 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+
(Assignee)

Updated

11 years ago
Keywords: fixed1.9.0.2
Whiteboard: [sg:critical?] maybe gc issue
Group: core-security
(Assignee)

Comment 9

11 years ago
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?)
(Assignee)

Comment 11

11 years ago
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
(Assignee)

Updated

7 years ago
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.