1074703 - ValueSelector is leaking by adding an event listener on the window

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

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.

Comment 1

[:gerard-majax]

Attachment: first memory-reports

Comment 2

[:gerard-majax]

Attachment: memory-reports with --minimize

Comment 3

[Gabriele Svelto [:gsvelto]]

From a quick glance it seems that we're leaking ContentParent objects for dead child processes.

Comment 4

[:gerard-majax]

Attachment: flame memory-reports

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.

Comment 5

[:gerard-majax]

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

Comment 6

[:gerard-majax]

Attachment: queued-ipc-messages for Nexus S after ~1d

Comment 7

[:gerard-majax]

Attachment: dmd-b2g-27622.txt.gz

DMD dump for B2G main process on Flame

Comment 8

[:gerard-majax]

Attachment: memory-reports with dmd

Comment 9

[Josh Matthews [:jdm]]

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.

Comment 10

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

Yeah I started looking at this yesterday but didn't quite figure out what was going on.

Comment 11

[Johan Lorenzo [:jlorenzo]]

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

Comment 12

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

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.

Comment 13

[Kevin Grandon :kgrandon]

I think I'll go ahead and take this.

Comment 14

[Kevin Grandon :kgrandon]

Attachment: Pull request - Ensure removeEventListener is called on destroy()

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 15

[Kevin Grandon :kgrandon]

Comment on attachment 8497864 [details] [review]
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.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8497864 [details] [review]
Pull request - Ensure removeEventListener is called on destroy()

Good catch

Comment 17

[Kevin Grandon :kgrandon]

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 18

[[e-mail]]

Comment on attachment 8497864 [details] [review]
Pull request - Ensure removeEventListener is called on destroy()

looks good to me, thanks for fixing the issue

Comment 19

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/3953ea1e76fd157e55235cbc7f42ea19c08692f1

Comment 20

[Gregor Wagner [:gwagner]]

Regression

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8497864 [details] [review]
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.

Comment 22

[bhavana bajaj [:bajaj]]

NI :tapas/inder so this is pulled into their branch immediately as well.

Comment 23

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Do we have any patch for v2.1 here ?

Comment 24

[bhavana bajaj [:bajaj]]

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

Comment 25

[Kevin Grandon :kgrandon]

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

Comment 26

[Kevin Grandon :kgrandon]

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.