Last Comment Bug 833796 - B2G's forms.js leaks due to a shutdown observer
: B2G's forms.js leaks due to a shutdown observer
Status: RESOLVED FIXED
[MemShrink] QARegressExclude
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: B2G C4 (2jan on)
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 833077 (view as bug list)
Depends on:
Blocks: 833077
  Show dependency treegraph
 
Reported: 2013-01-23 06:38 PST by Justin Lebar (not reading bugmail)
Modified: 2013-03-27 12:21 PDT (History)
11 users (show)
amiller: in‑moztrap-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
tef+
fixed
fixed
fixed


Attachments
Patch, v1 (1.59 KB, patch)
2013-01-23 07:35 PST, Justin Lebar (not reading bugmail)
21: review+
mvines: feedback+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2013-01-23 06:38:21 PST
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) 2013-01-23 06:44:16 PST
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) 2013-01-23 07:35:20 PST
Created attachment 705349 [details] [diff] [review]
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) 2013-01-23 07:38:31 PST
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] 2013-01-23 08:13:35 PST
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) 2013-01-23 08:53:15 PST
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) 2013-01-23 08:58:15 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/454569027d3a
Comment 7 Justin Lebar (not reading bugmail) 2013-01-23 08:58:59 PST
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] 2013-01-23 09:28:00 PST
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) 2013-01-23 09:31:15 PST
(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] 2013-01-23 11:10:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-01-23 11:10:53 PST
(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) 2013-01-23 14:45:36 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e9c3657e5cc6
Comment 13 Justin Lebar (not reading bugmail) 2013-01-23 14:46:35 PST
*** Bug 833077 has been marked as a duplicate of this bug. ***
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-01-23 19:18:26 PST
Nice find.
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-01-24 09:54:19 PST
https://hg.mozilla.org/mozilla-central/rev/454569027d3a
Comment 16 Milan Sreckovic [:milan] 2013-03-06 10:36:12 PST
Marking tef+ as it's duplicating 833077.
Comment 17 Justin Lebar (not reading bugmail) 2013-03-06 10:39:16 PST
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) 2013-03-06 10:40:30 PST
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 2013-03-22 10:53:25 PDT
Does not make sense to create a regression issue.

Note You need to log in before you can comment on or make changes to this bug.