899354 - BrowserElementParent isn't unregistering visibilityChangeHandler event listeners

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[Andrew McCreight [:mccr8]]

See bug 898990 for STR.  After around 3 days of running there are around 7400 event listeners that appears to be from BEP registered on a single nsGlobalWindow.  These are visibilityChangeHandler, ignoreClearCacheOn and ignoreClearCacheOff.  The latter two do not appear in any Mozilla repo, so it isn't clear where they are from, but they look like they are some other BEP event listener.  There are the same number of each of the three.

visibilityChangeHandler comes from here:
  http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#70

Comment 1

[Justin Lebar (not reading bugmail)]

The visibilityChangeHandler is meant to hang around for as long as the BEP hangs around.  So what we do is, when we get a visibilitychange notification, we check whether the BEP is still alive (using a weak ref).  If it's not alive, we unregister the listener.

It's not clear to me how to do better than this, without some sort of notification when an object goes away, or without an iterable weakset.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Bug 887781 might be useful here.

Comment 3

[Justin Lebar (not reading bugmail)]

Andrew suggested that, as a hack, we could keep a list of weakrefs and iterate over it every time we add to it.  That might be sufficient, and a sufficiently small change to be OK for b2g18.

Comment 4

[Andrew McCreight [:mccr8]]

I think it is okay to use Cu.nondeterministicWeakMapKeys.  It was intended to just be a testing function, but using that seems less hacky than some kind of set of XPCOM weak references that we have to manually purge.  It is worrying that we can't implement this pattern in "real" JS.

Comment 5

[Justin Lebar (not reading bugmail)]

I'm not going to try to fix the ignoreClearCache{On,Off} business, since that's a partner customization which we don't even have the complete source code for.  It's not even clear to me whether we should be looking at that source code, given that it's not being provided in compliance with the MPL.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1: Only ever add one visibilitychange listener to BrowserElementParent's owner window.

This prevents a leak when we run tests that create thousands of BrowserElementParents without ever turning the screen off.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 783343 [details] [diff] [review]
Patch, v1: Only ever add one visibilitychange listener to BrowserElementParent's owner window.

Review of attachment 783343 [details] [diff] [review]:
-----------------------------------------------------------------

Fun.

Comment 8

[Justin Lebar (not reading bugmail)]

When you uplift this, you'll need to change 'visibilitychange' to 'mozvisibilitychange' in two places.  Merge conflicts should point this out, and this should be the only non-trivial merge conflict.

Comment 9

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/projects/birch/rev/602e8b21a0ff

Comment 10

[Justin Lebar (not reading bugmail)]

Blocks a blocker.

Comment 11

[Andrew McCreight [:mccr8]]

On a side note, jlebar and I noticed that nsObserverService actually has a similar problem: weakly registered observers won't be cleared unless a notification happens.  nsObserverService has the advantage that every observer gets notified for all topics, compared to event listeners, where only things for a particular event get triggered.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/602e8b21a0ff

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 14

[Justin Lebar (not reading bugmail)]

> Needs a branch-specific patch for uplift.

I think it was just what was described in comment 8.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/11bb1b0eefff