Closed Bug 792385 Opened 7 years ago Closed 7 years ago

nsJSEventListener as roots in CC graph

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file, 1 obsolete file)

Something has regressed lately, I think.
I see plenty of nsJSEventListener objects in the CC graph.
There is one 5k cycle kept alive by various nsJSEventListener objects.
No documents leaked.
Assignee: nobody → bugs
Depends on: 793517
Attached patch WIP (obsolete) — Splinter Review
Attachment #663849 - Flags: review?(continuation)
Can you explain a little what is going on here? Is the idea that mTarget is something that we could do cleanup on, but we're not actually seeing it in the purple buffer? What are some examples of the types mTarget could have?
Comment on attachment 663849 [details] [diff] [review]
WIP

Review of attachment 663849 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/events/nsJSEventListener.cpp
@@ +91,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsJSEventListener)
> +  if (tmp->IsBlackForCC()) {
> +    return true;
> +  } else if (tmp->mTarget) {

This is an early return, so there's no need for an else.

@@ +92,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsJSEventListener)
> +  if (tmp->IsBlackForCC()) {
> +    return true;
> +  } else if (tmp->mTarget) {
> +    nsXPCOMCycleCollectionParticipant* cp = nullptr;

This section is unusual, so it would be nice to have a short description of what you are trying to do (eg you are trying to clean up mTarget if possible?).

@@ +96,5 @@
> +    nsXPCOMCycleCollectionParticipant* cp = nullptr;
> +    CallQueryInterface(tmp->mTarget, &cp);
> +    nsISupports* canonical = nullptr;
> +    tmp->mTarget->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
> +                                 reinterpret_cast<void**>(&canonical));

We should really bundle up the CC QI magic in a function in a header that CC participants inherit. Maybe not in this bug, but it seems to be creeping into more and more places outside the CC.

@@ +97,5 @@
> +    CallQueryInterface(tmp->mTarget, &cp);
> +    nsISupports* canonical = nullptr;
> +    tmp->mTarget->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
> +                                 reinterpret_cast<void**>(&canonical));
> +    if (cp && canonical && cp->CanSkip(canonical, true)) {

Probably this CanSkip's second argument should be the second argument of the nsJSEventListener's CanSkip?
(just some early comments, I'd still like to here what exactly this is doing before finishing my review...)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Can you explain a little what is going on here? Is the idea that mTarget is
> something that we could do cleanup on, but we're not actually seeing it in
> the purple buffer?
mTarget keeps event listener manager alive, and ELM keeps nsJSEventListener alive.
(well, XBL uses also nsJSEventListeners, but only as temporary variables.)

What are some examples of the types mTarget could have? DOM Element, Document, and nowadays
more often also nsDOMEventTargetHelper, like nsXMLHttpRequest.

> 
> This is an early return, so there's no need for an else.
Oops :)

> >  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsJSEventListener)
> > +  if (tmp->IsBlackForCC()) {
> > +    return true;
> > +  } else if (tmp->mTarget) {
> > +    nsXPCOMCycleCollectionParticipant* cp = nullptr;
> 
> This section is unusual, so it would be nice to have a short description of
> what you are trying to do (eg you are trying to clean up mTarget if
> possible?).
I'm trying to check if mTarget is certainly alive

> 
> We should really bundle up the CC QI magic in a function in a header that CC
> participants inherit.
totally agree. I guess I could add it in this bug.

> Probably this CanSkip's second argument should be the second argument of the
> nsJSEventListener's CanSkip?
Hmm, not sure why.
Attached patch v2Splinter Review
Couldn't figure out a nice way to do the QI and CanSkip stuff. Followup bug for that.

https://tbpl.mozilla.org/?tree=Try&rev=dbcb51df9338
Attachment #663849 - Attachment is obsolete: true
Attachment #663849 - Flags: review?(continuation)
Attachment #664016 - Flags: review?(continuation)
> Hmm, not sure why.
mTarget could be elsewhere in the purple buffer, so if aRemovingAllowed is false, then this CanSkip could end up removing it in nsPurpleBuffer::RemoveSkippable. Is that a problem?  I don't remember the precise reasons for it.
It is ok to remove other than currently handled object.
(In reply to Olli Pettay [:smaug] from comment #9)
> It is ok to remove other than currently handled object.
Can the target somehow indirectly end up unpurpling the current object?
nsJSEventListener doesn't have any way to unpurple it (outside itself). mRefCnt is not public.

(I know this setup is a bit fragile.)
Comment on attachment 664016 [details] [diff] [review]
v2

Review of attachment 664016 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding the comments.
Attachment #664016 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/6f7dd6f66a00
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.