1194270 - Tell the cycle collector about more fields of nsGlobalWindow

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

It looks like this should have happened back in bug 808612.

Comment 1

[Andrew McCreight [:mccr8]]

I just looked at all the fields, and there are some more that should be added to the traverse/unlink methods.

Comment 2

[Andrew McCreight [:mccr8]]

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?

Comment 3

[Andrew McCreight [:mccr8]]

I also filed bug 1194322 for a couple of wake lock fields that aren't being reported to the CC.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/831fb361c4bc

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/831fb361c4bc