Closed Bug 900221 Opened 7 years ago Closed 7 years ago

Every DOMApplication object registers a message listener which stays alive as long as its window lives

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(blocking-b2g:leo+, firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla26
blocking-b2g leo+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [MemShrink:P2][qa-])

Attachments

(2 files)

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.
Component: General → DOM: Apps
Product: Boot2Gecko → Core
Version: unspecified → Trunk
I'm adding code to let nsIFrameMessageManager take weak references.  This involves writing a C++ WeakSet, which we oh-so-badly need.
Assignee: nobody → justin.lebar+bug
Depends on: 901746
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.
Depends on: 901759
No longer depends on: 901746
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/integration/b2g-inbound/rev/650dc99a362d

If this works, I should back out 889984.
Depends on: 902714
No longer depends on: 901759
Moved leo+ from bug 889984 over to this bug.  This is a safer change to take on branches and accomplishes the same thing.
blocking-b2g: --- → leo+
Blocks: 897684
(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.
https://hg.mozilla.org/mozilla-central/rev/650dc99a362d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [MemShrink:P2] → [MemShrink:P2][qa-]
This has already landed on m-c, with rs=fabrice.
Attachment #789829 - Flags: review?
Attachment #789826 - Flags: review?(fabrice)
Attachment #789829 - Flags: review? → review+
Please confirm if fixing this issue will also fix bug 886217
Flags: needinfo?(justin.lebar+bug)
> 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.
Flags: needinfo?(justin.lebar+bug)
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... ?
Attachment #789826 - Flags: review?(fabrice) → review+
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.
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.
Flags: needinfo?(justin.lebar+bug)
Unfortunately, last Friday was Justin's last day working for Mozilla :(  You'll have to find somebody else to handle your question.
Flags: needinfo?(justin.lebar+bug)
Hi Joe,

Would help us find somebody to check it?
Thanks.
Flags: needinfo?(jcheng)
Hi Fabrice, is this something you can assist with? Thanks
Flags: needinfo?(jcheng) → needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
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.
Depends on: 933711
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.