833796 - B2G's forms.js leaks due to a shutdown observer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

B2G's forms.js is loaded by Keyboard.jsm.  The keyboard loads forms.js into each new mozbrowser, by hooking onto the observer messages 'in-process-browser-or-app-shown' and 'remote-browser-frame-shown'.

But if we're not careful, forms.js can cause leaks by adding strong refs to itself from the outside.  Then, when its frame element goes away, the forms.js script stays alive, holding the frame element alive, and therefore holding alive all other frame scripts as well!

At the very least, I think forms.js is leaking via its use of the observer service:

>     Services.obs.addObserver(this, "xpcom-shutdown", false);

|false| means this is a strong observer, which means that this object is not released until shutdown.  So the code that's trying to help us avoid shutdown leaks is (at least partially) causing runtime leaks!

Comment 1

[Justin Lebar (not reading bugmail)]

I'm going to see if I can reproduce bug 833077 locally, and if removing the observer service stuff from forms.js fixes the issue.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

This shutdown observer is not doing any good, as far as I can tell.  (If you really want to keep it, we can make it use a weak ref.)

Comment 3

[Justin Lebar (not reading bugmail)]

This seems to fix bug 833077 for me on desktop B2G.  mvines, would you mind testing on your device?

Comment 4

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 705349 [details] [diff] [review]
Patch, v1

Looks good!  After 347 iterations Uss is holding around 10MB.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 705349 [details] [diff] [review]
Patch, v1

If this code fix the leak I'm all for it!

Comment 6

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/454569027d3a

Comment 7

[Justin Lebar (not reading bugmail)]

checkin-needed for b2g18 (after this lands on m-c; I didn't push to try first).  This can land without approval because it's a likely fix for bug 833077, which is tef+.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Could this leak also manifest in the main process?   We're also debugging some unbounded memory growth that monkey is triggering, sometimes, there.   Maybe we have a twofer here!

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to Michael Vines [:m1] from comment #8)
> Could this leak also manifest in the main process?   We're also debugging
> some unbounded memory growth that monkey is triggering, sometimes, there.  
> Maybe we have a twofer here!

Yes, that's possible, if the app is running in the main process.

When I tested this on desktop B2G, everything was running in the main process, and this patch fixed the problem there.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1

Wrong bug, sorry.

Comment 12

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e9c3657e5cc6

Comment 14

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

Nice find.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/454569027d3a

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Marking tef+ as it's duplicating 833077.

Comment 17

[Justin Lebar (not reading bugmail)]

This is already in b2g18_v1_0_1, so I don't think there's any additional action required here.

It's very easy to tell if a bug is resolved on a branch using git.  I did

  $ git log origin/b2g18_v1_0_1 --grep 833796

If you're on the git.mozilla.org/releases/gecko.git repo, you'd probably do the same, but substitute "b2g18_v1_0_1" with "v1_0_1".

Comment 18

[Justin Lebar (not reading bugmail)]

Since this landed before we branched b2g18 to b2g18_v1_0_1, the hg hash on 1.0.1 is the same as on b2g18:

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e9c3657e5cc6

Comment 19

[amiller]

Does not make sense to create a regression issue.