1249439 - xul:browser adds itself as a strong observer to observer service

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

While debugging Bug 1249226 and bug 1195295 I saw observer service keeping the element alive occasionally. Yes, destroy() method should be called usually, but apparently there are cases when it isn't. I haven't yet found a minimal testcase for the leak though.

I have no idea why strong observer is being used. The relevant code is _old_, from bug 207188 [1].
But anyhow, using strong observers is probably one of the most common ways our chrome js leaks.


[1] http://52.25.115.98/viewvc/main/mozilla/toolkit/content/widgets/browser.xml?revision=1.36&view=markup

Comment 1

[:Gijs (he/him)]

Don't we need to implement nsISupportsWeakReference for this to work?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

All the DOM nodes support weak references.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf2614f3458

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef81c79f6266

Comment 5

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08c4026b6c9b

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

and no information about what bustage

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b401a002f8c

Comment 8

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #6)
> and no information about what bustage

This patch didn't cause any bustage; the patch that you landed together with this one that modified the cycle collector (bug 1249451) did because it didn't compile. I don't think this should have been backed out, and you should feel free to reland it. Sorry I didn't get around to doing that earlier, I noticed, but got distracted again.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #6)
> > and no information about what bustage
> 
> This patch didn't cause any bustage; the patch that you landed together with
> this one that modified the cycle collector (bug 1249451) did because it
> didn't compile. I don't think this should have been backed out, and you
> should feel free to reland it. Sorry I didn't get around to doing that
> earlier, I noticed, but got distracted again.

cf. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef81c79f6266&selectedJob=22009392


/builds/slave/m-in-m64-st-an-d-0000000000000/build/src/xpcom/base/nsCycleCollector.cpp:1498:3: error: bad implicit conversion constructor for 'LogStringMessageAsync'
make[5]: *** [Unified_cpp_xpcom_base0.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [xpcom/base/target] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [compile] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
Return code: 2
'mach build' did not run successfully. Please check log for errors.
Running post_fatal callback...
Exiting -1
# TBPL FAILURE #
# TBPL FAILURE #

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9803cf0d7e9c

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9803cf0d7e9c