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

RESOLVED FIXED in Firefox 21

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

unspecified
B2G C4 (2jan on)
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 fixed, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [MemShrink] QARegressExclude)

Attachments

(1 attachment)

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.
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink]
Assignee: nobody → justin.lebar+bug
Posted patch Patch, v1Splinter Review
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?
(Assignee)

Updated

6 years ago
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+.
Keywords: checkin-needed
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → B2G C4 (2jan on)
Flags: in-testsuite+
(Assignee)

Updated

6 years ago
Summary: B2G's forms.js leaks (I think...) → B2G's forms.js leaks due to a shutdown observer
(Assignee)

Updated

6 years ago
Duplicate of this bug: 833077
https://hg.mozilla.org/mozilla-central/rev/454569027d3a
Status: NEW → RESOLVED
Last Resolved: 6 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

Comment 19

6 years ago
Does not make sense to create a regression issue.
Flags: in-moztrap-

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink] QARegressExclude
You need to log in before you can comment on or make changes to this bug.