1183848 - BrowserElementParent leaks forever via mozfullscreenchange listener

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

http://hg.mozilla.org/mozilla-central/diff/447b220d7c55/dom/browser-element/BrowserElementParent.js#l1.30 replaced a weakly held observer service listener with a strongly held DOM event listener.  That event listener never gets removed, which causes every BrowserElementParent to be leaked forever.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

I'm not quite sure how to confirm whether the leakage is fixed with this patch. Kyle, could you verify that?

Comment 2

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

The data came out of a Firefox OS about:memory dump in bug 1183361, so I can't verify it either.  It would be easy to do with a b2g phone though (just use it for a while and see how high the orphan-nodes amount in the parent process gets).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633861 [details] [diff] [review]
patch

unload is not dispatched to document, so unless you're trying to catch some
other unload happening elsewhere in the tree, this isn't right.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8635073 [details] [diff] [review]
patch

So Bug 899354 added nondeterministicGetWeakMapKeys usage.


Hmm, technically we really should use addSystemEventListener to add the listener.

And the old code had a bug that if one removed all the browser-parents, the
listener was removed, but this._window._browserElementParents was still there.

So, if you use addSystemEventListener, and not the normal addEventListener this should be fine.

But khuey should definitely look at this too, given he reviewed Bug 899354

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch, r=smaug

Comment 7

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

I don't understand why it is ok to not remove the event listener?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

The listeners won't live longer than the window object, since they are only referenced by it. And since one window has at most one set of listeners, even if multiple BrowserElementParents are created on a same window, the number of listeners won't increase.

Comment 9

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

Comment on attachment 8635849 [details] [diff] [review]
patch, r=smaug

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

ok. I still haven't actually tested this though ...

::: dom/browser-element/BrowserElementParent.js
@@ +112,5 @@
> +      let els = Cc["@mozilla.org/eventlistenerservice;1"]
> +                  .getService(Ci.nsIEventListenerService);
> +      for (let event of windowEvents) {
> +        els.addSystemEventListener(this._window, event, handler,
> +                                   /* useCapture = */ true);

I'm assuming that swapping visibilitychange to use the system event listener and useCapture=true won't break anything ...

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fda440fa9aa1

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fda440fa9aa1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this something we need on Aurora for Fx41?

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

I don't think we are using browser element anywhere other than Firefox OS, hence it doesn't seem to be necessary to uplift the patch.