940426 - properly stop observing all the sources in nsXULTemplateBuilder

Status: RESOLVED FIXED

(Core :: XUL, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: properly stop observing all the sources in nsXULTemplateBuilder

So this is an interesting "leak".

If ContentRemoved is called, we stop observing events on the document.  But the
observer service is still holding references to us for dom-window-destroyed and
xpcom-shutdown.  Even though these are weak references, we'll stick around until
xpcom-shutdown, which IMHO isn't a good idea.  So the destructor never gets
called, since the observer service still holds onto us, and Uninit(true) is never
called.

The only other way Uninit(true) gets called is through NodeWillBeDestroyed...which
will never get called because we're no longer observing events on the appropriate
document anymore.

printf logging appears to show that the leaks are fixed.  I need to do a try run
for this and double-check about:memory dumps for mochitest-browser-chrome to
verify the fixes.

These weak references seem to be kind of a footgun.  WDYT about changing these
to strong references now, since we're detaching the observers properly?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8334571 [details] [diff] [review]
properly stop observing all the sources in nsXULTemplateBuilder

r=me if you nix the printfs.

And yeah, the observers can probably be strong here.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: don't observe xpcom-shutdown in nsXULTemplateBuilder

After thinking about this a little more, we shouldn't have to observe
xpcom-shutdown at all if we're already watching for window destruction.
I think the xpcom-shutdown observation was a hack put in when this
either did more or to properly release things...?

Anyway, try is surprisingly green with these patches:

https://tbpl.mozilla.org/?tree=Try&rev=6258d5a78e80

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8335400 [details] [diff] [review]
don't observe xpcom-shutdown in nsXULTemplateBuilder

r=me

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/828b7a8e6647
https://hg.mozilla.org/mozilla-central/rev/daf343a5478c