Last Comment Bug 889984 - WebappsApplication objects are leaked until their corresponding window dies
: WebappsApplication objects are leaked until their corresponding window dies
Status: RESOLVED FIXED
[MemShrink][qa-]
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 893188 895104
Blocks: 851626 886217 892647
  Show dependency treegraph
 
Reported: 2013-07-03 11:56 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-09-10 16:22 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
wontfix
wontfix
wontfix
wontfix


Attachments
gc edges from comments above (fix applied) (1000.73 KB, application/x-xz)
2013-07-03 18:50 PDT, Justin Lebar (not reading bugmail)
no flags Details
cc edges from comments above (fix applied) (434.43 KB, application/x-xz)
2013-07-03 18:50 PDT, Justin Lebar (not reading bugmail)
no flags Details
Patch, v1 (28.79 KB, patch)
2013-07-03 19:03 PDT, Justin Lebar (not reading bugmail)
fabrice: feedback+
Details | Diff | Review
Patch, v2 (28.93 KB, patch)
2013-07-08 13:48 PDT, Justin Lebar (not reading bugmail)
fabrice: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2013-07-03 11:56:07 PDT
WebappsApplication inherits from
Comment 1 Justin Lebar (not reading bugmail) 2013-07-03 11:57:29 PDT
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.
Comment 2 Justin Lebar (not reading bugmail) 2013-07-03 15:13:38 PDT
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.
Comment 3 Justin Lebar (not reading bugmail) 2013-07-03 18:41:24 PDT
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.
Comment 4 Justin Lebar (not reading bugmail) 2013-07-03 18:47:41 PDT
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]
Comment 5 Justin Lebar (not reading bugmail) 2013-07-03 18:49:15 PDT
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.
Comment 6 Justin Lebar (not reading bugmail) 2013-07-03 18:50:04 PDT
Created attachment 771134 [details]
gc edges from comments above (fix applied)
Comment 7 Justin Lebar (not reading bugmail) 2013-07-03 18:50:48 PDT
Created attachment 771135 [details]
cc edges from comments above (fix applied)
Comment 8 Justin Lebar (not reading bugmail) 2013-07-03 19:03:13 PDT
Created attachment 771139 [details] [diff] [review]
Patch, v1
Comment 9 Justin Lebar (not reading bugmail) 2013-07-03 19:09:12 PDT
(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 10 Mike Habicher [:mikeh] (high bugzilla latency) 2013-07-04 07:32:29 PDT
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.
Comment 11 Justin Lebar (not reading bugmail) 2013-07-04 10:28:24 PDT
> 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 12 Mike Habicher [:mikeh] (high bugzilla latency) 2013-07-04 10:30:59 PDT
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?
Comment 13 Justin Lebar (not reading bugmail) 2013-07-04 10:35:23 PDT
Ah, I see; yes, those lines should be swapped, of course.
Comment 14 [:fabrice] Fabrice Desré 2013-07-08 11:55:02 PDT
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.
Comment 15 Justin Lebar (not reading bugmail) 2013-07-08 12:39:40 PDT
::: 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.
Comment 16 [:fabrice] Fabrice Desré 2013-07-08 12:44:59 PDT
(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.
Comment 17 Justin Lebar (not reading bugmail) 2013-07-08 12:59:41 PDT
> 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?
Comment 18 [:fabrice] Fabrice Desré 2013-07-08 13:02:17 PDT
(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.
Comment 19 Justin Lebar (not reading bugmail) 2013-07-08 13:48:11 PDT
Created attachment 772300 [details] [diff] [review]
Patch, v2
Comment 20 Ed Morley [:emorley] 2013-07-09 02:33:02 PDT
https://hg.mozilla.org/mozilla-central/rev/4b00bb32356d
Comment 21 Justin Lebar (not reading bugmail) 2013-07-31 13:34:15 PDT
I think we forgot to leo? this this.  This blocks two blockers.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-08-01 09:28:20 PDT
Needs a branch-specific patch for uplift.
Comment 23 Justin Lebar (not reading bugmail) 2013-08-02 10:29:11 PDT
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.
Comment 24 Justin Lebar (not reading bugmail) 2013-08-05 16:14:31 PDT
Given bug 901759, I think we'd just need to change {add,remove}MessageListener to {add,remove}WeakMessageListener in DOMRequestHelpers.
Comment 25 Justin Lebar (not reading bugmail) 2013-08-05 17:30:08 PDT
> 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).
Comment 26 Justin Lebar (not reading bugmail) 2013-08-06 13:40:48 PDT
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.
Comment 27 Justin Lebar (not reading bugmail) 2013-08-06 13:42:45 PDT
Oops; here's the link: https://github.com/jlebar/mozilla-central/tree/weak-ref-finalizer-b2g18
Comment 28 Justin Lebar (not reading bugmail) 2013-08-08 13:54:55 PDT
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.
Comment 29 Justin Lebar (not reading bugmail) 2013-08-08 14:11:37 PDT
Sorry for the flag spam.
Comment 30 Justin Lebar (not reading bugmail) 2013-08-31 16:22:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.