Tell the cycle collector about more fields of nsGlobalWindow

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
It looks like this should have happened back in bug 808612.
(Assignee)

Comment 1

3 years ago
I just looked at all the fields, and there are some more that should be added to the traverse/unlink methods.
Summary: Tell the cycle collector about nsGlobalWindow::mLocation → Tell the cycle collector about more fields of nsGlobalWindow
(Assignee)

Comment 2

3 years ago
Kyle, I noticed that nsGlobalWindow::mIndexedDB is not traversed or unlinked. Should I add it? There's a bunch of IDB classes that traverse but don't unlink IDBFactory for some reason, so maybe I could do that here, too?
Flags: needinfo?(khuey)
(Assignee)

Comment 3

3 years ago
I also filed bug 1194322 for a couple of wake lock fields that aren't being reported to the CC.
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Kyle, I noticed that nsGlobalWindow::mIndexedDB is not traversed or
> unlinked. Should I add it? There's a bunch of IDB classes that traverse but
> don't unlink IDBFactory for some reason, so maybe I could do that here, too?

Hmm ... I guess it doesn't leak because FreeInnerObjects clears mIndexedDB?  Not sure if we need to bother CCing it or not in that case.
Flags: needinfo?(khuey)
The IDB classes traversing but not unlinking stuff is expected.  IDB unlink methods are designed to create an acyclic graph, not a completely disconnected graph.  That allows us to use certain pointers during cleanup without having to null check them constantly.
(Assignee)

Comment 6

3 years ago
Created attachment 8652465 [details] [diff] [review]
Tell the cycle collector about more fields of nsGlobalWindow.

try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d47fa561ec04

Based on your comment, it sounds like it is okay to null out the IDB field, because presumably IDB won't be going through the window.

Also, remove what looks like an obsolete pre-cycle-collector comment about cyclical ownership.
Attachment #8652465 - Flags: review?(khuey)
Comment on attachment 8652465 [details] [diff] [review]
Tell the cycle collector about more fields of nsGlobalWindow.

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

::: dom/base/nsGlobalWindow.h
@@ -1634,5 @@
> -  // through JavaScript.  If there is any chance that a member variable
> -  // could own objects that are implemented in JavaScript, then those
> -  // objects will keep the global object (this object) alive.  To prevent
> -  // these cycles, ownership of such members must be released in
> -  // |CleanUp| and |DetachFromDocShell|.

lol
Attachment #8652465 - Flags: review?(khuey) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/831fb361c4bc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.