Closed Bug 731132 Opened 9 years ago Closed 9 years ago

PlacesUtils.asyncGetBookmarkIds leaks everything when scope is heavyweight

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Attachment #601197 - Flags: feedback?(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.
could you rather change the loop to something like
while (this._shutdownFunctions.length > 0) {
  this._shutdownFunctions.shift().apply(this);
}
(In reply to Marco Bonardo [:mak] from comment #2)
Yes, that should achieve the necessary effect.  Would you be the reviewer for this code?
sure
Attached patch fix leakSplinter Review
Attachment #601435 - Flags: review?(mak77)
Attachment #601197 - Attachment is obsolete: true
Attachment #601197 - Flags: feedback?(peterv)
Assignee: nobody → luke
Status: NEW → ASSIGNED
Comment on attachment 601435 [details] [diff] [review]
fix leak

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

Thank you!
Attachment #601435 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/51c1a0cd1efe
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.