1009928 - statusbar.js leaks bound functions as event handlers

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating bug 1007520 I noticed that there were a large number of |sb_updateWifi()| functions active in the parent process.  It appears this is due to the system app's statusbar.js registering new bound functions as event listeners when |setActive()| is called.

https://github.com/mozilla-b2g/gaia/blob/5b2a6b442566f469e928bd45c69a590c91041450/apps/system/js/statusbar.js#L553

These listeners never seem to be removed.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch

Hi Ben,

I've just made a patch, adding the removeEventListener for the 'wifi-statuschange' event in the "else" block of setActive(). The event also now goes to handleEvent() first, which dispatches it to bound update.wifi(), for the sake of code uniformity.

Euh..but I don't know whom I should ask for review. Should it be Alive Kuo?

Thanks.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8422258 [details] [review]
Patch

Thanks for jumping on this John!  This looks good to me, although I have not tested it.

Looking at the log it seems Alive or Tim would be the typical reviewer for this file.  They are both traveling this week, though, so I imagine it might be a bit delayed.  Lets flag Alive and if he doesn't have time maybe he can redirect it to someone better.

Thanks again!

Comment 3

[James Zhang (Spreadtrum)]

Yang, can we land this WIP and run monkey test to verify?

Comment 4

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

Comment on attachment 8422258 [details] [review]
Patch

r+ if there's an unit test for this.

Comment 5

[yang.zhao]

(In reply to James Zhang from comment #3)
> Yang, can we land this WIP and run monkey test to verify?

Do we need to wait for the unit test as comment 4 mentioned or just land this as a patch for monkey test?

Comment 6

[yang.zhao]

(In reply to James Zhang from comment #3)
> Yang, can we land this WIP and run monkey test to verify?

Landed under folder sprd_patch.commit:534bf0465c068673e404e0e5417bbdfec87d4546

Comment 7

[John Lu [:mnjul][Please NI or I'll miss]]

I wrote/pushed the test and am waiting for it to pass Travis. Just a little more moments :)

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ to prevent mem leaks

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

to John since John has provided the patch. Thanks

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

It's John Lu not John Hu. Thanks.

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Greetings everyone,

The patch with unit test has passed Travis test. As I don't have permission to merge codes, could anyone merge the codes into master for me?

At the same time, I will also propose the patches into v1.3 and v1.4 branches.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/4fd90529b9eec811f7e1deece2b4ef519f44ea32

Comment 13

[yang.zhao]

(In reply to James Zhang from comment #3)
> Yang, can we land this WIP and run monkey test to verify?
Hi,James
   Do we need to land this patch on v1.3t ?Since now it is under the sprd_patch folder.
   If it is needed,there are some conflicts in file apps/system/test/unit/statusbar_test.js when I try to cherry-pick it to v1.3t.

Comment 14

[yang.zhao]

pull request for v1.3t:
https://github.com/mozilla-b2g/gaia/pull/19470

Comment 15

[yang.zhao]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> 4fd90529b9eec811f7e1deece2b4ef519f44ea32

hi,Tim
  When I try to merge it to v1.3t,the Travis failed.Please see https://github.com/mozilla-b2g/gaia/pull/19470 
  Could you help me to check it ?Thank you.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The errors are inconclusive. I re-run the tests. We can check it tomorrow.

Comment 17

[yang.zhao]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #16)
> The errors are inconclusive. I re-run the tests. We can check it tomorrow.
Hi,Tim
    Where could I find the re-run tests result ?Did it turn to green ?Sorry to bother you,thank you again!

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Tests are run automatically when a pull request is submitted. There is a link on the pull request page that will bring you to the test summary page

pull request: https://github.com/mozilla-b2g/gaia/pull/19470
test summary: https://travis-ci.org/mozilla-b2g/gaia/builds/25673740

Looks like this still failing on one JS int test

https://s3.amazonaws.com/archive.travis-ci.org/jobs/25673743

Comment 19

[yang.zhao]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18)
> Tests are run automatically when a pull request is submitted. There is a
> link on the pull request page that will bring you to the test summary page
> 
> pull request: https://github.com/mozilla-b2g/gaia/pull/19470
> test summary: https://travis-ci.org/mozilla-b2g/gaia/builds/25673740
> 
> Looks like this still failing on one JS int test
> 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/25673743

When I try to cherry-pick to v1.3t,there are some conflicts in apps/system/test/unit/statusbar_test.js .Though I resolved the conflicts manually,I don't know whether the error is related with the conflicts.
Since this patch should merge into v1.3t,could you help to merge it to v1.3t?Thank you very much!^_^

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

You are responsible of fixing conflicts. 1.3t is considered released.

Alternatively, if you don't really care, you can disable the broken test instead.

Comment 21

[yang.zhao]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> You are responsible of fixing conflicts. 1.3t is considered released.
> 
> Alternatively, if you don't really care, you can disable the broken test
> instead.

Ok,I merged it into v1.3t although the Travis is failed.

Comment 22

[James Zhang (Spreadtrum)]

(In reply to yang.zhao from comment #21)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #20)
> > You are responsible of fixing conflicts. 1.3t is considered released.
> > 
> > Alternatively, if you don't really care, you can disable the broken test
> > instead.
> 
> Ok,I merged it into v1.3t although the Travis is failed.

commit 6ad553e9db091f7400069a4551941cf4249c0544
Merge: 2c0390f cd9e2f7
Author: sprd-ffos <[e-mail]>
Date:   Thu May 22 14:37:59 2014 +0800

    Merge pull request #19470 from leonazhao/bug1009928
    
    Bug 1009928 - statusbar.js leaks bound functions as event handlers, v1.3t