673293 - Memory leaks when adding a function to PlacesUIUtils

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

[Martijn Wargers (dead)]

Attachment: example extension that shows the leak

I'm getting these leaks with the example extension:

Summary:
Leaked 0 out of 0 DOM Windows
Leaked 22 out of 67 documents
Leaked 0 out of 7 docshells
Leaked content nodes in 37 out of 84 documents

It turns out that PlacesUIUtils.randomfunction = function() {} is causing the leaks.
I don't know why it happens.
PlacesUIUtils.jsm is placed here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/PlacesUIUtils.jsm

I need to override a method from lacesUIUtils in an extension that I made. I'm surprised this is causing leaks.

I guess maybe what I'm doing is just bound to cause leaks?

Comment 1

[Martijn Wargers (dead)]

Ok, from irc it sounded that this bug may be invalid.

Callek>	mw22: ahh so you (*were*) creating the replacement from a .js/overlay/whatever attached to a regular firefox window?
[01:37]	<mw22>	Callek, yes
[01:37]	<Callek>	mw22: if so, yea you'll be leaking |window| objects, creating a .jsm is easy :-)
[01:37]	<mw22>	Callek, a .js/overlay/whatever is easier
[01:37]	<taras>	jrmuizel: enjoy
[01:38]	<Callek>	mw22: so you'll want to read https://developer.mozilla.org/en/JavaScript_code_modules/Using first
[01:38]	<jrmuizel>	taras: thanks
[01:39]	<taras>	jrmuizel: you need to do some makefile/include screwery to include chrome headers(but you might already know that)
[01:39]	<jrmuizel>	taras: I'll figure it out
[01:39]	<Callek>	mw22: once you create a module (I'd suggest with something like myextPlacesUIOverride.jsm and have an .init() function that does the replacement, and use Components.utils.import in the firefox window, and call init()
[01:39]	<Callek>	you only need to replace it once, so you should certainly have an internal (bool) |alreadyDidInit| type bar
[01:39]	<Callek>	s/bar/var/
[01:40]	<mw22>	Callek, ok, thanks for the info. I'll try to use that
[01:40]	<Callek>	the scope of your jsm is basically empty, (not having |window| to use, so for _any_ |window| based values, you'll need to get a window sort of like the PlacesUIUtils you are replacing does
[01:40]	<Callek>	mw22: I hope that was fairly clear

Comment 2

[Marco Bonardo [:mak]]

I was about to say you are binding the scope of your function to the module scope, that is mostly what they said. Yes this is bound to leak.

Comment 3

[Justin Wood (:Callek)]

In short, this is causing leaks because the function you are adding to PlacesUIUtils is holding a strong reference to the |window| object you load.

Basically if your firefox.xul (whatever) window has a <script>function foo() {}</script> then its really referenced by |window.foo()| DOM0 just makes this all easy for you.

When you assign it to the .jsm (which stays alive longer than the normal window/code) you are essentially telling it to store |myWindow.foo()| which keeps that |window| object alive, and all the documents/nodes it references, etc.

So there are two solutions, #1 is to use a jsm...
#2 is to store (locally, somehow) the original func from the PlacesUIUtils you are trying to replace, and at window unload restore it, so your replacement is essentially "lost" and thus the window-scope-bleed is not happening.

Not a firefox bug so RESO/INVALID