1422983 - cycle collect ClientSource owner reference to nsGlobalWindowInner and nsDocShell

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

We are seeing some leaks on nightly lately.  It occurs to me that the ClientSource code I integrated with nsGlobalWindowInner and nsDocShell could be contributing to the problem.  There is a ref-cycle in play but we don't include it in the cycle collection because ClientSource is not ref-counted at the moment.

Comment 1

[Andrew McCreight [:mccr8]]

This could cause bad leaks and should be easy to fix.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77744b88435fc0be682e4ffecabd636f2cf9a9b

Comment 3

[Andrew McCreight [:mccr8]]

Make sure to traverse/unlink this in every CCed thing that holds one of these. It looks like at least nsDocShell and nsGlobalWindowInner have one. LoadInfo has one but it isn't CCed. WorkerPrivate has one, and maybe it is CCed?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934316 [details] [diff] [review]
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

Since this is a small patch I'm going to flag for review now even though try has not completed.

Note, we only need to CC the ClientSource when its owned by the nsGlobalWindowInner and nsDocShell.  These are the only cases where we have a strong ref out from ClientSource to a CC'able object.

When the ClientSource is owned by a WorkerPrivate or LoadInfo object we do not have a strong reference out to the owner.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8934316 [details] [diff] [review]
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

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

::: dom/clients/manager/ClientSource.cpp
@@ +270,5 @@
> +{
> +  if (mOwner.is<RefPtr<nsPIDOMWindowInner>>()) {
> +    ImplCycleCollectionTraverse(aCallback,
> +                                mOwner.as<RefPtr<nsPIDOMWindowInner>>(),
> +                                "mOwner", aFlags);

You should use aName here instead of "mOwner", and in the other case. aName will be the name of the ClientSource field (for instance "mClientSource"). This is what the name of the field is going to show up as in the cycle collector log, for the window or docshell. If there was more than one thing here we wanted to traverse then it would get complicated but for now this will work. If you think that would be confusing, then maybe a field name like ClientSource::mOwner would work.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46463dab3129
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

Comment 8

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

Hmm, how good is the cycle collection stuff in nsDocShell. Didn't mystor make the globalwindow split in a bit different way because merging outer to nsDocShell was leaky.
(I wonder why mInitialClientSource is in docshell and not in outer. Perhaps doesn't matter much.)

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/46463dab3129