The default bug view has changed. See this FAQ.

JSEventListener could have weak mTarget to reduce CC overhead

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla10
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
Created attachment 574081 [details] [diff] [review]
patch
Attachment #574081 - Flags: feedback?(continuation)
https://tbpl.mozilla.org/?tree=Try&rev=8111e9aff59e
Comment on attachment 574081 [details] [diff] [review]
patch

Looks good to me, from the perspective of removing things from the CC graph.  Thanks!
Attachment #574081 - Flags: feedback?(continuation) → feedback+
Comment on attachment 574081 [details] [diff] [review]
patch

nsIJSEventListener ctor change makes sure we get the right nsISupports*.
That is, AFAIK, needed only because nsGlobalWindow's nsISupports isn't
nsIDOMEventTarget.

We could perhaps remove the whole QI, but I'd like to investigate that in
a different bug.
Attachment #574081 - Flags: review?(peterv)
Created attachment 574095 [details]
nsJSEventListener after patch

The patch nicely gets rid of a bunch of nodes.  The attached diagram is the equivalent of the "bird of prey" I attached to bug 702032, and it now lacks that extra outer ring.  That's a reduction of 200 nodes from just this one part.
Review ping
Comment on attachment 574081 [details] [diff] [review]
patch

Boris, you've looked at this code lately...
Attachment #574081 - Flags: review?(peterv) → review?(bzbarsky)
Comment on attachment 574081 [details] [diff] [review]
patch

Add some comments about mTarget being a weak reference and explicit Disconnect() calls being needed if it goes away, and r=me.

We're pretty sure that the ELM can't outlive its target?
Attachment #574081 - Flags: review?(bzbarsky) → review+
ELM's mTarget and event listeners are cleared when ELM's Disconnect() is called.
And there is an assertion if Disconnect() isn't called.
Created attachment 577522 [details] [diff] [review]
patch

https://hg.mozilla.org/mozilla-central/rev/e3c8a14cd60b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment on attachment 574081 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): N/A
 
User impact if declined: Maybe longer CC pauses

Testing completed (on m-c, etc.):
Landed m-c 2011-11-29 so this is already in Aurora. Haven't heard of
any regressions. Comment 5 nicely visualizes what kind of impact this has.

Risk to taking this patch (and alternatives if risky):
Some binary addon could in theory use nsIJSEventListener and rely on
it to keep mTarget alive.
This is still, IMO, on the safe side of possible cycle collection changes for 
FF10 (which is why I'm asking for approval).
Attachment #574081 - Flags: approval-mozilla-beta?
If the patch gets approved, comment
+// Note, mTarget is a raw pointer and the owner of the nsIJSEventListener object
+// is expected to call Disconnect()!
needs to be copied from the patch for trunk.

Comment 13

5 years ago
Comment on attachment 574081 [details] [diff] [review]
patch

[triage comment]
Approved for beta. We're hoping this helps with the CC pause issues we've been seeing.
Attachment #574081 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Created attachment 584870 [details] [diff] [review]
beta patch with comment change
https://hg.mozilla.org/releases/mozilla-beta/rev/09bc6f7ed00e
Target Milestone: mozilla11 → mozilla10
You need to log in before you can comment on or make changes to this bug.