792385 - nsJSEventListener as roots in CC graph

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: WIP

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=bb2690ce7451

Comment 3

[Andrew McCreight [:mccr8]]

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 4

[Andrew McCreight [:mccr8]]

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?

Comment 5

[Andrew McCreight [:mccr8]]

(just some early comments, I'd still like to here what exactly this is doing before finishing my review...)

Comment 6

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

(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.

Comment 7

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

Attachment: v2

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

Comment 8

[Andrew McCreight [:mccr8]]

> 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.

Comment 9

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

It is ok to remove other than currently handled object.

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

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

nsJSEventListener doesn't have any way to unpurple it (outside itself). mRefCnt is not public.

(I know this setup is a bit fragile.)

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 664016 [details] [diff] [review]
v2

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

Thanks for adding the comments.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/6f7dd6f66a00