cycle collect ClientSource owner reference to nsGlobalWindowInner and nsDocShell

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox58 unaffected, firefox59? fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a month ago
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.
(Assignee)

Updated

a month ago
Summary: verify ClientSource does not break cycle collection for nsGlobalWindowInner and nsDocShell → cycle collect ClientSource owner reference to nsGlobalWindowInner and nsDocShell
Blocks: 1417172
status-firefox58: --- → unaffected
status-firefox59: --- → affected
status-firefox-esr52: --- → unaffected
status-thunderbird_esr52: --- → unaffected
This could cause bad leaks and should be easy to fix.
tracking-firefox59: --- → ?
(Assignee)

Comment 2

a month ago
Created attachment 8934316 [details] [diff] [review]
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77744b88435fc0be682e4ffecabd636f2cf9a9b
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?
(Assignee)

Comment 4

a month ago
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.
Attachment #8934316 - Flags: review?(continuation)
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.
Attachment #8934316 - Flags: review?(continuation) → review+
(Assignee)

Comment 6

a month ago
Created attachment 8934384 [details] [diff] [review]
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8
Attachment #8934316 - Attachment is obsolete: true
Attachment #8934384 - Flags: review+

Comment 7

a month ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46463dab3129
Cycle collect the ClientSource object when owned by an nsGlobalWindowInner or nsDocShell object. r=mccr8
(Assignee)

Updated

a month ago
Blocks: 1293277
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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46463dab3129
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a month ago
See Also: → bug 1422912
(Assignee)

Updated

a month ago
See Also: bug 1422912
You need to log in before you can comment on or make changes to this bug.