WebappsApplication inherits from

Oops, hit enter too early. :) WebappsApplication inherits from DOMRequestIpcHelper. DOMRequestIpcHelper adds some strong refs to itself in initHelper. Those strong refs are manually unlinked on inner-window-destroyed.

Oh gosh, we may leak them forever. DOMRequestIpcHelper has a QI method. You can QI to nsIObserver. This observer is the thing that unlinks the DOMRequestIpcHelper when the window closes. But suppose an object "inherits" from DOMRequestIpcHelper and declares its own QI method, and suppose that doesn't include nsIObserver. I'd expect that the observer never fires, at this point. Both WebappsRegistry and WebappsApplication have this problem.

Even after (I think) fixing the leaks in Gecko, it seems like Gaia is leaking all app objects forever, or something. I traced the roots graph back to via nsXPCWrappedJS::mJSObj : 0x48c362c0 [Function] --[**UNKNOWN SLOT 0**]-> 0x48c345b0 [Object <no private>] --[app]-> 0x43dc3c40 [XPCWrappedNative_NoHelper 4721f7f0] via nsXPCWrappedJS::mJSObj : 0x43da2820 [Object <no private>] --[updatableApps]-> 0x43da6bf0 [Array <no private>] --[objectElements[40]]-> 0x48c345b0 [Object <no private>] --[app]-> 0x43dc3c40 [XPCWrappedNative_NoHelper 4721f7f0] I don't know how to tell what the first one is, but in the second one, "updatableApps" comes from Gaia.

Here are the roots for another one of these leaked objects. Again, this is all Gaia, afaict. > $ python gc/find_roots.py ~/code/moz/B2G/tools/gc-logs-0/gc-edges.3239.13729013 > 32.log 0x43d394e0 > Parsing /home/jlebar/code/moz/B2G/tools/gc-logs-0/gc-edges.3239.1372901332.log. Done loading graph. Reversing graph. Done. > > via nsXPCWrappedJS::mJSObj : > 0x43daab60 [Function ai_handleApplicationInstallEvent] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daac60 [Function ai_handleCancelDownloadCancel] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via nsXPCWrappedJS::mJSObj : > 0x43daab80 [Function ai_handleApplicationUninstall] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daabc0 [Function ai_showInstallCancelDialog] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via nsXPCWrappedJS::mJSObj : > 0x43daab40 [Function ai_handleChromeEvent] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daac20 [Function ai_showDownloadCancelDialog] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daac00 [Function ai_hideInstallCancelDialog] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daaba0 [Function ai_handleInstall] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daac40 [Function ai_handleConfirmDownloadCancel] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via mCallback : > 0x43daabe0 [Function ai_handleInstallCancel] > --[**UNKNOWN SLOT 0**]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via XPCWrappedNative::mFlatJSObject : > 0x43d240d0 [Window 45da4b20] > --[AppInstallManager]-> 0x43da2430 [Object <no private>] > --[handleApplicationReady]-> 0x43daad00 [Function ai_handleApplicationReady] > --[shape]-> 0x43dab370 [shape] > --[base]-> 0x43da5a10 [base_shape] > --[parent]-> 0x43daa7a0 [Function ai_handleApplicationReady] > --[script]-> 0x43d2ce80 [script app://system.gaiamobile.org/js/app_install_manager.js:73] > --[objects[0]]-> 0x43daa3a0 [Function ai_handleApplicationReady/<] > --[lazyScript]-> 0x43da87f0 [lazyscript] > --[realScript]-> 0x43d2cf00 [script app://system.gaiamobile.org/js/app_install_manager.js:80] > --[function]-> 0x43d33860 [Function ai_handleApplicationReady/<] > --[fun_callscope]-> 0x48c05cd0 [Call <no private>] > --[apps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via nsXPCWrappedJS::mJSObj : > 0x43d54d80 [Function a_uninstall] > --[fun_callscope]-> 0x43d55370 [Call <no private>] > --[self]-> 0x43d51920 [Object <no private>] > --[installedApps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550] > > via nsXPCWrappedJS::mJSObj : > 0x43d54d60 [Function a_install] > --[lazyScript]-> 0x43d581f0 [lazyscript] > --[enclosingScope]-> 0x43d54ba0 [Function a_init] > --[script]-> 0x43d28600 [script app://system.gaiamobile.org/js/applications.js:12] > --[objects[0]]-> 0x43d54a60 [Function getAllApps] > --[lazyScript]-> 0x43d58190 [lazyscript] > --[realScript]-> 0x48c39b00 [script app://system.gaiamobile.org/js/applications.js:16] > --[objects[0]]-> 0x43d54a80 [Function mozAppGotAll] > --[lazyScript]-> 0x43d58160 [lazyscript] > --[lazyScriptInnerFunction]-> 0x43d54aa0 [Function mozAppGotAll/<] > --[lazyScript]-> 0x43d58130 [lazyscript] > --[realScript]-> 0x43d2cd80 [script app://system.gaiamobile.org/js/applications.js:19] > --[function]-> 0x43d337e0 [Function mozAppGotAll/<] > --[fun_callscope]-> 0x43d55370 [Call <no private>] > --[self]-> 0x43d51920 [Object <no private>] > --[installedApps]-> 0x43d55340 [Object <no private>] > --[https://marketplace.firefox.com/packaged.webapp]-> 0x43d394e0 [XPCWrappedNative_NoHelper 48861550]

So...I think we should take the patch I've written for this, which will make it so Gecko no longer leaks these objects forever. Then we should probably focus our efforts on bug 889990, which will make Gaia's leaking of these objects not a huge deal.

(In reply to Justin Lebar [:jlebar] from comment #8) > Created attachment 771139 [details] [diff] [review] > Patch, v1 Whoops, ignore that hunk in nsMemoryInfoDumper.cpp. That is for a separate bug.

Comment on attachment 771139 [details] [diff] [review] Patch, v1 Review of attachment 771139 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMRequestHelper.jsm @@ +160,1 @@ > return; It looks like we will always hit this return case.

> It looks like we will always hit this return case. Could you give a little more context? I'm not sure which return you're talking about.

Comment on attachment 771139 [details] [diff] [review] Patch, v1 Review of attachment 771139 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMRequestHelper.jsm @@ +160,1 @@ > return; this._destroyed = true; if (this._destroyed) { return; } Setting _destroyed, then immediately checking if it's true?

Ah, I see; yes, those lines should be swapped, of course.

Comment on attachment 771139 [details] [diff] [review] Patch, v1 Review of attachment 771139 [details] [diff] [review]: ----------------------------------------------------------------- That looks mostly good, but I'd like to see a new version with comments addressed. ::: dom/apps/src/Webapps.js @@ +249,5 @@ > > let util = this._window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > this._id = util.outerWindowID; > cpmm.sendAsyncMessage("Webapps:RegisterForMessages", Didn't you forget to add Ci.nsISupportsWeakReference to WebappsRegistry object QueryInterface? ::: dom/base/DOMRequestHelper.jsm @@ +9,1 @@ > const Cu = Components.utils; Nit: feel free to clean this trailing whitespace. @@ +35,5 @@ > + * appropriate window is destroyed. We use DOMRequestIpcHelperMessageListener > + * for this, too. > + */ > +this.DOMRequestIpcHelperMessageListener = function(aHelper, aWindow, aMessages) { > + this._weakHelper = Cu.getWeakReference(aHelper); You're only using _weakHelper.get() so you could just keep that. ::: dom/push/src/Push.js @@ +57,5 @@ > let perm = Services.perms.testExactPermissionFromPrincipal(principal, > "push"); > if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) > return null; > Is is ok to not call initDomRequestHelper() at all? We won't hook up the inner-window-destroyed observer.

::: dom/apps/src/Webapps.js @@ +249,5 @@ > > let util = this._window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > this._id = util.outerWindowID; > cpmm.sendAsyncMessage("Webapps:RegisterForMessages", > Didn't you forget to add Ci.nsISupportsWeakReference to WebappsRegistry object > QueryInterface? Yeah, thanks. @@ +35,5 @@ > + * appropriate window is destroyed. We use DOMRequestIpcHelperMessageListener > + * for this, too. > + */ > +this.DOMRequestIpcHelperMessageListener = function(aHelper, aWindow, aMessages) { > + this._weakHelper = Cu.getWeakReference(aHelper); > > You're only using _weakHelper.get() so you could just keep that. I don't understand; what do you mean? ::: dom/push/src/Push.js @@ +57,5 @@ > let perm = Services.perms.testExactPermissionFromPrincipal(principal, > "push"); > if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) > return null; > > Is is ok to not call initDomRequestHelper() at all? We won't hook up the > inner-window-destroyed observer. It should be initDOMRequestHelper here, not initMessageListener, yes. That function doesn't even exist anymore. JS is hard.

(In reply to Justin Lebar [:jlebar] from comment #15) > @@ +35,5 @@ > > + * appropriate window is destroyed. We use > DOMRequestIpcHelperMessageListener > > + * for this, too. > > + */ > > +this.DOMRequestIpcHelperMessageListener = function(aHelper, aWindow, > aMessages) { > > + this._weakHelper = Cu.getWeakReference(aHelper); > > > > You're only using _weakHelper.get() so you could just keep that. > > I don't understand; what do you mean? instead of |this._weakHelper = Cu.getWeakReference(aHelper)|, doing |this._weakHelper = Cu.getWeakReference(aHelper).get()|. That's a really minor change though.

> instead of |this._weakHelper = Cu.getWeakReference(aHelper)|, doing |this._weakHelper = > Cu.getWeakReference(aHelper).get()|. Isn't that totally different? The latter makes this._weakHelper a strong reference, right?

(In reply to Justin Lebar [:jlebar] from comment #17) > > instead of |this._weakHelper = Cu.getWeakReference(aHelper)|, doing |this._weakHelper = > > Cu.getWeakReference(aHelper).get()|. > > Isn't that totally different? The latter makes this._weakHelper a strong > reference, right? You're right, my bad.

I think we forgot to leo? this this. This blocks two blockers.

Needs a branch-specific patch for uplift.

This will be pain in the rear to backport, and we had multiple regressions when landing on trunk. I may have a safer fix in bug 900221. Stay tuned.

Given bug 901759, I think we'd just need to change {add,remove}MessageListener to {add,remove}WeakMessageListener in DOMRequestHelpers.

> This will be pain in the rear to backport, and we had multiple regressions when landing on trunk. And moreover, backporting this bug isn't going to fix the entire problem, because we'll still leak message listeners, and that's going to slow lots of things down (e.g. dispatching messages).

Here's a git branch with the alternative approach we've discussed in bug 900221 and dependent bugs. It would be good to get some testing of these patches before we land them.

Oops; here's the link: https://github.com/jlebar/mozilla-central/tree/weak-ref-finalizer-b2g18

I'm going to move the leo+ here over to bug 900221, which intends to fix the same problem, but in a way that's safer for branches. In fact, if everything works as planned there, we should be able to back out this change.

Sorry for the flag spam.

Fabrice, you may want to back out these DOMRequestHelper changes now that we've switched too {add,remove}WeakMessageListener; it's not necessary anymore to split up the DOMRequestHelper class like this.