Closed Bug 1183848 Opened 5 years ago Closed 5 years ago

BrowserElementParent leaks forever via mozfullscreenchange listener


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

Not set



blocking-b2g 2.5+
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
b2g-master --- fixed


(Reporter: khuey, Assigned: xidorn)



(Keywords: memory-leak)


(1 file, 2 obsolete files) 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.
Attached patch patch (obsolete) — Splinter Review
I'm not quite sure how to confirm whether the leakage is fixed with this patch. Kyle, could you verify that?
Flags: needinfo?(khuey)
Attachment #8633861 - Flags: review?(bugs)
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).
Flags: needinfo?(khuey)
Comment on attachment 8633861 [details] [diff] [review]

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.
Attachment #8633861 - Flags: review?(bugs) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #8633861 - Attachment is obsolete: true
Attachment #8635073 - Flags: review?(khuey)
Attachment #8635073 - Flags: review?(bugs)
Comment on attachment 8635073 [details] [diff] [review]

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
Attachment #8635073 - Flags: review?(bugs) → review+
Attached patch patch, r=smaugSplinter Review
Attachment #8635073 - Attachment is obsolete: true
Attachment #8635073 - Flags: review?(khuey)
Attachment #8635849 - Flags: review?(khuey)
I don't understand why it is ok to not remove the event listener?
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(quanxunzhen)
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[";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 ...
Attachment #8635849 - Flags: review?(khuey) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Is this something we need on Aurora for Fx41?
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(quanxunzhen)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.