In bug 897684, we noticed that every WebappsApplication object registers a message listener that lives for as long as its window lives. This is by design after bug 889984 -- it's better than the previous situation, where we'd leak the whole WebappsApplication object until the window died. But it's still not good, and if we can get into situations where we have 13,000 message listeners, that's probably bad for performance, memory usage, or both.

I'm adding code to let nsIFrameMessageManager take weak references. This involves writing a C++ WeakSet, which we oh-so-badly need.

Justin, I don't want to derail your effort in 901746 but there may be a simpler way to register only one listener per window instead of per app. Do you want me to try that?

> there may be a simpler way to register only > one listener per window instead of per app. Do you want me to try that? I guess we could make this work. We'd register one message listener per window, and the message listener would have a WeakSet whose keys would be the DOMApplication objects in question. Then we could iterate over the WeakSet's keys using Cu.nondeterministicGetWeakMapKeys and dispatch the messages to the objects. That's essentially what adding a weak message listener does, just in the message manager instead of in JS. What I described above might be a safer way of doing this on b2g18; I dunno.

I have patches for the weak-set changes up on github. Here's a b2g18 branch: https://github.com/jlebar/mozilla-central/tree/weak-ref-finalizer-b2g18 We need to get some testing on this guy.

https://hg.mozilla.org/integration/b2g-inbound/rev/650dc99a362d If this works, I should back out 889984.

Moved leo+ from bug 889984 over to this bug. This is a safer change to take on branches and accomplishes the same thing.

(In reply to Justin Lebar [:jlebar] (limited availability 8/9 – 8/12) from comment #7) > https://hg.mozilla.org/releases/mozilla-b2g18/rev/dc5d28a645d5 Hi Justin, had to back this changes out in https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=218640747bcd since this checkin caused failed mochitests like https://tbpl.mozilla.org/php/getParsedLog.php?id=26327224&tree=Mozilla-B2g18

Bummer. But it did stick on trunk, which is interesting...

Ooh, I see. Okay, this is a simple thing to fix. The issue is that anything which inherits from DOMRequestHelper must implement nsISupportsWeakReference. We did this on trunk as part of bug 889984, but we didn't backport that to b2g18. I'll roll up the appropriate patch. We don't have tryserver on b2g18, so we'll just have to push and see.

This has already landed on m-c, with rs=fabrice.

Please confirm if fixing this issue will also fix bug 886217

> Please confirm if fixing this issue will also fix bug 886217 I don't know for sure. As best we understand the symptoms of bug 886217, it will. But it is possible that fixing bug 886217 will require other changes.

Comment on attachment 789826 [details] [diff] [review] Patch, v1 - (b2g18 only) Add Ci.nsISupportsWeakReference to applicable JS classes. Review of attachment 789826 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.js @@ +825,5 @@ > }, > > classID: Components.ID("{8c1bca96-266f-493a-8d57-ec7a95098c15}"), > > + QueryInterface: XPCOMUtils.generateQI([Ci.mozIDOMApplicationMgmt, Ci.nsISupportsWeakReference]), nit: can you align the Ci.nsISupportsWeakReference with Ci.mozIDOM... ?

Let's hope this sticks. (Without tryserver, it's hard to say.) https://hg.mozilla.org/releases/mozilla-b2g18/rev/6155a371223a https://hg.mozilla.org/releases/mozilla-b2g18/rev/171a90f58f74

Backed out for mochitest timeouts. https://hg.mozilla.org/releases/mozilla-b2g18/rev/4e35bfc690f4

Relanded with additional QI to nsIMessageManager. I have no idea how this code works without this QI. Maybe we implicitly add it somehow... https://hg.mozilla.org/releases/mozilla-b2g18/rev/367cb34c8518 https://hg.mozilla.org/releases/mozilla-b2g18/rev/e51b8e012b43

Looks like it's going to stick this time.

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bcd70a871ade https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e85db8ff0a7c

Dear Justin, In our device, this patch seemed will cause this issue. " If user leave the device during long time for example 30 minutos or longer in "IDLE" mode with screen turn off and after that if user press "power on" button device will take long time to wake up and show the completed screen. " I revert this patch, it's ok. Could you help to confirm it? Thanks.

Unfortunately, last Friday was Justin's last day working for Mozilla :( You'll have to find somebody else to handle your question.

Hi Joe, Would help us find somebody to check it? Thanks.

Hi Fabrice, is this something you can assist with? Thanks

Yes, I can check if I reproduce this bug today. I'll reopen the bug if this is the case.

Please file new bugs for regressions.

I'm looking into the DOMRequestHelper.jsm. It looks very weird to me that DOMRequestIpcHelperMessageListener holds just weak reference to DOMRequestIpcHelper. Supposing an (DOMApplication) object inherits from DOMRequestIpcHelper and it can be garbage collected at any time when no JS variables refers to it on the content site, then DOMRequestIpcHelperMessageListener cannot pass its received message to the DOMRequestIpcHelper object because the DOMRequestIpcHelper object might already be released at this moment. Taking IAC Message Port (which I'm working on) for example, the port object should always live with the window so that it can be kept alive to do messaging.

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #28) > I'm looking into the DOMRequestHelper.jsm. It looks very weird to me that > DOMRequestIpcHelperMessageListener holds just weak reference to > DOMRequestIpcHelper. Supposing an (DOMApplication) object inherits from > DOMRequestIpcHelper and it can be garbage collected at any time when no JS > variables refers to it on the content site, then > DOMRequestIpcHelperMessageListener cannot pass its received message to the > DOMRequestIpcHelper object because the DOMRequestIpcHelper object might > already be released at this moment. That's actually what we want for Applications DOM objects. > Taking IAC Message Port (which I'm working on) for example, the port object > should always live with the window so that it can be kept alive to do > messaging. It looks like in some cases we want the non-weak version.

(In reply to sync-1 from comment #22) > Dear Justin, > > In our device, this patch seemed will cause this issue. > > " If user leave the device during long time for example 30 minutos or longer > in > "IDLE" mode with screen turn off and after that if user press "power on" > button > device will take long time to wake up and show the completed screen. " > I revert this patch, it's ok. > > Could you help to confirm it? > Thanks. FWIW I couldn't reproduce this issue with latest b2g18 + v1-train.

Part 1 modifying the DOMRequestHelper appears to cause a resource leak in the clock app on b2g18. See bug 915220. Is this backout worthy?

It appears comment 22 is a real issue that is now effecting hd1.1. See bug 933711. I think this may be related to the b2g18 patch landed here adding weak message listeners, but leaving the strong observer reference.