731132 - PlacesUtils.asyncGetBookmarkIds leaks everything when scope is heavyweight

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Luke Wagner [:luke]]

Attachment: a leak fix, maybe not the right one

A side-effect of bug 730497 is to make asyncGetBookmarkIds a "heavyweight" function which seems to cause the BackstagePass and 1000 other things to get leaked at shutdown.  However, this is nothing special about bug 730497, to see this on trunk, just put an "eval('')" at the head of asyncGetBookmarkIds (toolkit/components/places/PlacesUtils.jsm#2081) and simply start/close a debug browser.

What seems to be happening is that the call to registerShutdownFunction adds the lambda to the array _shutdownFunctions stored on the BackstagePass.  This lambda keeps the function scope alive which, in turn, keeps other things alive.  When registerShutdownFunction is not heavyweight, the lambda's environment is just the BackstagePass, so the problem must come from one of the locals/parameters/callee.

The obvious/naive change that seems to fix the leak is for PU_observe to empty out the array _shutdownFunctions once it calls all these functions at shutdown.  Honestly, I would have thought the cycle collector would clean all this up, so I'd be happy to hear if someone thought there was a more fundamental bug here.  Peter, what do you think?

Comment 1

[Peter Van der Beken [:peterv]]

(In reply to Luke Wagner [:luke] from comment #0)
> Honestly, I would have thought the cycle collector would clean
> all this up, so I'd be happy to hear if someone thought there was a more
> fundamental bug here.  Peter, what do you think?

There's an ordering issue, we could move final cycle collection to later in shutdown (for example after all components have been shutdown which I think would clear their global objects), but that doesn't really solve the problem because you can still create cycles that would need cycle collections at an even later point.

In general we've treated leaks like this as real leaks, because in almost all cases that I've encountered we're holding things alive for the lifetime of the application without a real need. If we really need to keep these functions alive until shutdown, then nulling out after use seems fine to me.

Comment 2

[Marco Bonardo [:mak]]

could you rather change the loop to something like
while (this._shutdownFunctions.length > 0) {
  this._shutdownFunctions.shift().apply(this);
}

Comment 3

[Luke Wagner [:luke]]

(In reply to Marco Bonardo [:mak] from comment #2)
Yes, that should achieve the necessary effect.  Would you be the reviewer for this code?

Comment 4

[Marco Bonardo [:mak]]

sure

Comment 5

[Luke Wagner [:luke]]

Attachment: fix leak

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 601435 [details] [diff] [review]
fix leak

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

Thank you!

Comment 7

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51c1a0cd1efe

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/51c1a0cd1efe