The default bug view has changed. See this FAQ.

WebappsApplication objects are leaked until their corresponding window dies

RESOLVED FIXED in Firefox 25

Status

()

Core
DOM: Apps
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd wontfix)

Details

(Whiteboard: [MemShrink][qa-])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
WebappsApplication inherits from
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 851626
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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]
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 771134 [details]
gc edges from comments above (fix applied)
(Assignee)

Updated

4 years ago
Attachment #771134 - Attachment description: gc edges from comments above → gc edges from comments above (fix applied)
(Assignee)

Comment 7

4 years ago
Created attachment 771135 [details]
cc edges from comments above (fix applied)
(Assignee)

Comment 8

4 years ago
Created attachment 771139 [details] [diff] [review]
Patch, v1
Attachment #771139 - Flags: review?(fabrice)
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
> 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?
(Assignee)

Comment 13

4 years ago
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.
Attachment #771139 - Flags: review?(fabrice) → feedback+
(Assignee)

Comment 15

4 years ago
::: 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.
(Assignee)

Comment 17

4 years ago
> 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.
(Assignee)

Comment 19

4 years ago
Created attachment 772300 [details] [diff] [review]
Patch, v2
Attachment #771139 - Attachment is obsolete: true
Attachment #772300 - Flags: review?(fabrice)
Attachment #772300 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/4b00bb32356d
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 892647
Depends on: 893188
Depends on: 895104
(Assignee)

Comment 21

4 years ago
I think we forgot to leo? this this.  This blocks two blockers.
Blocks: 886217, 897684
blocking-b2g: --- → leo+
Needs a branch-specific patch for uplift.
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
Flags: needinfo?(justin.lebar+bug)
Keywords: branch-patch-needed
(Assignee)

Comment 23

4 years ago
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.
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Updated

4 years ago
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Comment 24

4 years ago
Given bug 901759, I think we'd just need to change {add,remove}MessageListener to {add,remove}WeakMessageListener in DOMRequestHelpers.
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Comment 25

4 years ago
> 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).
(Assignee)

Comment 26

4 years ago
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.
(Assignee)

Comment 27

4 years ago
Oops; here's the link: https://github.com/jlebar/mozilla-central/tree/weak-ref-finalizer-b2g18
(Assignee)

Comment 28

4 years ago
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.
blocking-b2g: leo+ → ---
(Assignee)

Updated

4 years ago
status-b2g18: affected → wontfix
status-b2g-v1.1hd: affected → wontfix
Keywords: branch-patch-needed
(Assignee)

Updated

4 years ago
No longer blocks: 897684
(Assignee)

Updated

4 years ago
status-b2g18: wontfix → fixed
status-b2g-v1.1hd: wontfix → affected
(Assignee)

Comment 29

4 years ago
Sorry for the flag spam.
status-b2g18: fixed → wontfix
status-b2g-v1.1hd: affected → wontfix
(Assignee)

Comment 30

4 years ago
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.
Whiteboard: [MemShrink] → [MemShrink][qa-]
You need to log in before you can comment on or make changes to this bug.