Closed Bug 1074703 Opened 10 years ago Closed 10 years ago

ValueSelector is leaking by adding an event listener on the window

Categories

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: gerard-majax, Assigned: kgrandon)

References

Details

(Keywords: regression, Whiteboard: [systemsfe][MemShrink:P1])

Attachments

(7 files)

Reproduced on a master build from sept. 25th, running on Nexus S. Looking at get_about_memory report after ~1d of use of the phone, there is a lot of content in the "queued-ipc-messages" section.
Attached file first memory-reports (deleted) —
From a quick glance it seems that we're leaking ContentParent objects for dead child processes.
Attached file flame memory-reports (deleted) —
Reproduced on a Flame with the profile from the Nexus S restored. Downsized memory: fastboot oem mem 440 Then boot, use the device (launching several apps, playing with them). When logcat starts to expose: > 09-30 11:25:47.636 315 315 I Gecko : > 09-30 11:25:47.636 315 315 I Gecko : ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv > 09-30 11:25:47.636 315 315 I Gecko : seems to correlate when we see queued-ipc-messages growing.
Okay, so I think a set of conclusive STRs would be: 0. Start device 1. Start several apps 2. adb shell b2g-ps 3. Kill all apps except B2G and Nuwa processes Do it a couple of times, and you should get the list of "queued-ipc-messages" to grow in the memory report generated by get_about_memory.py
Whiteboard: [MemShrink]
Attached file dmd-b2g-27622.txt.gz (deleted) —
DMD dump for B2G main process on Flame
Attached file memory-reports with dmd (deleted) —
These aren't messages leaking; the amount for each entry is 0. However, it's a list of ContentParent objects with a refcount of 1 for dead processes, so that's worrying.
Yeah I started looking at this yesterday but didn't quite figure out what was going on.
Repro'd on 2.1: the more I open then kill (with adb shell kill PID) apps, the more I see the queued-ipc-messages getting bigger (example: Camera can appear 3 times). > Gaia-Rev a00d102abfe8ae15c4fd14771efa2335c6d3b8d9 > Gecko-Rev https://hg.mozilla.org/releases/mozilla-aurora/rev/cde28bd9a285 > Build-ID 20140930000203 > Version 34.0a2 > Device-Name flame > FW-Release 4.4.2 > FW-Incremental eng.cltbld.20140930.042005 > FW-Date Tue Sep 30 04:20:16 EDT 2014 > Bootloader L1TC10011800 Repro'd on 2.0 too: > Gaia-Rev 5c2303ec4e367da060aa1b807d541a6549b3d72a > Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/38a496c50d98 > Build-ID 20140930000204 > Version 32.0 > Device-Name flame > FW-Release 4.4.2 > FW-Incremental eng.cltbld.20140930.033845 > FW-Date Tue Sep 30 03:38:55 EDT 2014 > Bootloader L1TC10011800 Repro'd on 1.4 too: > Gaia-Rev b06fee13c1d80bd2a463be1f2fb1d59169af9298 > Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1dad764fbd52 > Build-ID 20140930000202 > Version 30.0 > Device-Name flame > FW-Release 4.3 > FW-Incremental 110 > FW-Date Fri Jun 27 15:57:58 CST 2014 > Bootloader L1TC00011230 Repro'd on 1.3 also: > Gaia-Rev 23f55be856cef53c6604a6fe4aeb09061afbc897 > Gecko-Rev 04ad4926a65c141eac5df9598c563f5efcdc8aee > Build-ID 20140930152850 > Version 28.0 > Device-Name flame > FW-Release 4.3 > FW-Incremental eng.jlorenzo.20140930.145820 > FW-Date Tue Sep 30 14:58:31 CEST 2014 > Bootloader L1TC00011230
I investigated a couple different leaked iframes in my local build. They're being entrained by the event listener added at https://github.com/mozilla-b2g/gaia/blame/master/apps/system/js/value_selector/value_selector.js#L32, which adds an edge from the system app Window that lives forever to ValueSelector which is transitory and specific to the app. This seems to be quite easy to reproduce with email. This should block 2.1 because leaks are bad. There may be other issues that we can find on older branches but this was added in 2.1.
Assignee: khuey → nobody
Blocks: 1054135
blocking-b2g: --- → 2.1?
Component: IPC → Gaia::System::Input Mgmt
Keywords: regression
Product: Core → Firefox OS
Summary: IPC messages leaking → ValueSelector is leaking by adding an event listener on the window
Version: Trunk → unspecified
Whiteboard: [MemShrink] → [MemShrink:P1]
I think I'll go ahead and take this.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Whiteboard: [MemShrink:P1] → [systemsfe][MemShrink:P1]
Target Milestone: --- → 2.1 S6 (10oct)
Wow, apparently we may have never been calling destroy() on sub_components for a while now. This may be causing a lot of the other issues we've been seeing, especially so since it seems we may have been missing the destroy() call on app_transition_controller, though I'm not 100% sure if this was causing issues or not. Opening a pull request now to see what gaia-try will do, and will work on a test in parallel.
Comment on attachment 8497864 [details] Pull request - Ensure removeEventListener is called on destroy() Hey Alive, Fred - This pull request fixes two things. 1 - A long standing bug in which we check the wrong property for destroying sub components. I wonder if this could be causing some other issues that we've been seeing. 2 - Removing of the event listener in the value selector destroy method.
Attachment #8497864 - Flags: review?(gasolin)
Attachment #8497864 - Flags: review?(alive)
Comment on attachment 8497864 [details] Pull request - Ensure removeEventListener is called on destroy() Good catch
Attachment #8497864 - Flags: review?(alive) → review+
Also had to add a simple !app guard to app_transition_controller like we do in the rest of the file: https://github.com/mozilla-b2g/gaia/pull/24584/files#diff-f70ce6c514b65fb6094cdde4d5382495R143
Comment on attachment 8497864 [details] Pull request - Ensure removeEventListener is called on destroy() looks good to me, thanks for fixing the issue
Attachment #8497864 - Flags: review?(gasolin) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Regression
blocking-b2g: 2.1? → 2.1+
Comment on attachment 8497864 [details] Pull request - Ensure removeEventListener is called on destroy() [Approval Request Comment] [Bug caused by] (feature/regressing bug #): There were two things fixed here. One was a long time hidden issue that was fixed here but exposed by this and was necessary to fix the memory leak. Bug 1054135 introduced the memory leak. [User impact] if declined: Inevitable system slowdown until crash I assume. [Testing completed]: Manual verification and unit tests that we clean up the event listeners. [Risk to taking this patch] (and alternatives if risky): Relatively low-risk as the patch is fairly simple. [String changes made]: None.
Attachment #8497864 - Flags: approval-gaia-v2.1?(fabrice)
Attachment #8497864 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
NI :tapas/inder so this is pulled into their branch immediately as well.
Flags: needinfo?(tkundu)
Do we have any patch for v2.1 here ?
Flags: needinfo?(tkundu) → needinfo?(bbajaj)
(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #23) > Do we have any patch for v2.1 here ? Its in comment #21.
Flags: needinfo?(bbajaj)
Flags: needinfo?(tkundu)
I've also cherry-picked this commit into v2.1 to ensure there are no conflicts. It merged cleanly: https://github.com/mozilla-b2g/gaia/commit/49f7b3f471e0345b719d92d6a3291c658b3ee4d8
Flags: needinfo?(tkundu)
Marking a dependency on bug 1075043 for the test. This has a unit test which catches this in CI, so testsuite+ for now to clear tracking, but we still need a marionette test here.
Depends on: 1075043
Flags: in-qa-testsuite+
Flags: in-qa-testsuite+ → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: