window manager is leaking apploaded event listeners causing long CC times

RESOLVED FIXED

Status

P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
After 3 days of running a test that opens an app every 10 seconds, we end up with 7000 apploaded event listeners registered on an iframe element.  See bug 898990 for further details.

apploaded is defined here:
  http://mxr.mozilla.org/gaia/source/apps/system/js/window_manager.js#800

These event listeners are being marked gray, so this is probably contributing to the long CC times in the test.  It would be good to not leak those event listeners, but it is also a little odd that they aren't being optimized out of the CC graph.
(Reporter)

Comment 1

5 years ago
These event listeners are all registered on a single iframe:

0x4c25c1c0 [FragmentOrElement (xhtml) iframe class='' app://system.gaiamobile.org/index.html]
    --[[via hash] mListenerManager]--> 0x4a9025b0 [nsEventListenerManager]
    --[mListeners[i] mListener]--> 0x4506bc08 [nsXPCWrappedJS (nsIDOMEventListener)]
    --[mJSObj]--> 0x44a26e20 [JS Object (Function - apploaded)]

It looks like this this iframe is probably legitimately alive, so we're just failing to unregister the event listener when something dies, maybe?

The reflector for the iframe is being held alive by a path that looks kind of like this, so maybe this is a real iframe for the homescreen app:
    --[WindowManager]--> 0x44a9df70 [Object <no private>]
    --[retrieveHomescreen]--> 0x44adfa60 [Function retrieveHomescreen]
    --[fun_callscope]--> 0x44a9dee0 [Call <no private>]
    --[runningApps]--> 0x44ad8310 [Object <no private>]
    --[app://homescreen.gaiamobile.org]--> 0x4c485b80 [Object <no private>]
    --[iframe]--> 0x4c48f9a0 [HTMLIFrameElement 4b459cd0]

Comment 2

5 years ago
Well, just having thousands of extra objects in jsholders hashtable sure makes CC slower.
(Reporter)

Comment 3

5 years ago
Yeah, and having to UnmarkGray 15000 things is probably going to be kind of slow, too...
(Reporter)

Updated

5 years ago
Blocks: 898990
No longer depends on: 898990
(Assignee)

Comment 4

5 years ago
To be clear, these event listeners are either 'mozbrowserloadend' or 'appopen', I think.  We don't know which.  The code is in window_manager.js.
(Reporter)

Comment 5

5 years ago
Oh, okay, these listeners aren't being optimized out because this is an AllTraces log.  In a real CC, they are hopefully being optimized out, but I'd guess we're still getting huge pauses (maybe even in the CC cleanup?) because there are just so many objects.  That's just conjecture without a non-AllTraces CC log, however.
(Assignee)

Comment 6

5 years ago
What's ironic about all this is that the only reason we have this event listener is to dispatch apploadtime, which is purely diagnostic.

I think we can rewrite this to use just one event listener.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 7

5 years ago
Oh, and the timing is half-broken; it only works for cold-launches of apps, in v1-train.  For warm launches, it just displays 0.

To enable app timing, dig through settings:

 - device information
 - more information
 - developer
 - show time to load
(Assignee)

Comment 8

5 years ago
> Oh, and the timing is half-broken; it only works for cold-launches of apps, in v1-train.

But this is fixed in master.  I guess we just didn't want to take the one-character fix on v1-train.
(Assignee)

Comment 9

5 years ago
FTR:

diff --git a/apps/system/js/window_manager.js b/apps/system/js/window_manager.js
index 8166395..f4e9580 100644
--- a/apps/system/js/window_manager.js
+++ b/apps/system/js/window_manager.js
@@ -403,7 +403,7 @@ var WindowManager = (function() {
       origin: displayedApp,
       isHomescreen: (manifestURL === homescreenManifestURL)
     });
-    frame.dispatchEvent(evt);
+    iframe.dispatchEvent(evt);
   }
 
   // Executes when app closing transition finishes.
(Reporter)

Updated

5 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 10

5 years ago
Created attachment 782867 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11236

Pointer to Github pull-request
(Assignee)

Updated

5 years ago
Attachment #782867 - Flags: review?(alive)
(Assignee)

Comment 11

5 years ago
Created attachment 782871 [details]
Pointer to Github pull request for v1-train.

Pointer to Github pull-request
(Assignee)

Updated

5 years ago
Attachment #782871 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11238 → Pointer to Github pull request for v1-train.
Attachment #782871 - Flags: review?(alive)
Attachment #782867 - Flags: review?(alive) → review+
Attachment #782871 - Flags: review?(alive) → review+
(Assignee)

Comment 12

5 years ago
Merged to master: https://github.com/mozilla-b2g/gaia/commit/4b8a6d1d2aa3f5557b66c9863e397d2946e4e69b

and to v1-train (blocks a blocker): https://github.com/mozilla-b2g/gaia/commit/6c635e809baedb6b3393bd351b8ce95304782ac6
blocking-b2g: --- → leo+
status-b2g18: --- → fixed
status-b2g-v1.1hd: --- → affected
(Assignee)

Comment 13

5 years ago
Thanks, Alive.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
v1.1.0hd: 6c635e809baedb6b3393bd351b8ce95304782ac6
v1.1.0hd: dea92acb6be86623b9f66e196ef39bb895c4bd37
status-b2g-v1.1hd: affected → fixed
You need to log in before you can comment on or make changes to this bug.