Closed Bug 833796 Opened 7 years ago Closed 7 years ago
.js leaks due to a shutdown observer
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!
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.
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.)
Attachment #705349 - Flags: review?(21)
This seems to fix bug 833077 for me on desktop B2G. mvines, would you mind testing on your device?
Attachment #705349 - Flags: feedback?(mvines)
Comment on attachment 705349 [details] [diff] [review] Patch, v1 Looks good! After 347 iterations Uss is holding around 10MB.
Attachment #705349 - Flags: feedback?(mvines) → feedback+
Comment on attachment 705349 [details] [diff] [review] Patch, v1 If this code fix the leak I'm all for it!
Attachment #705349 - Flags: review?(21) → review+
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+.
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!
(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.
Target Milestone: --- → B2G C4 (2jan on)
(In reply to Ryan VanderMeulen from comment #10) > https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1 Wrong bug, sorry.
Summary: B2G's forms.js leaks (I think...) → B2G's forms.js leaks due to a shutdown observer
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Marking tef+ as it's duplicating 833077.
blocking-b2g: --- → tef+
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".
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
Does not make sense to create a regression issue.
You need to log in before you can comment on or make changes to this bug.