Closed Bug 1009928 Opened 5 years ago Closed 5 years ago

statusbar.js leaks bound functions as event handlers

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: bkelly, Assigned: mnjul)

References

Details

(Whiteboard: [MemShrink][partner-blocker])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
alive
: review+
bkelly
: feedback+
Details | Review
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.
Whiteboard: [MemShrink]
Attached file 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.
blocking-b2g: --- → 1.3T?
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!
Attachment #8422258 - Flags: review?(alive)
Attachment #8422258 - Flags: feedback+
Whiteboard: [MemShrink] → [MemShrink][partner-blocker]
Yang, can we land this WIP and run monkey test to verify?
Flags: needinfo?(yang.zhao)
Flags: needinfo?(dliang)
Comment on attachment 8422258 [details] [review]
Patch

r+ if there's an unit test for this.
Attachment #8422258 - Flags: review?(alive) → review+
(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?
Flags: needinfo?(james.zhang)
(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
I wrote/pushed the test and am waiting for it to pass Travis. Just a little more moments :)
triage: 1.3T+ to prevent mem leaks
blocking-b2g: 1.3T? → 1.3T+
to John since John has provided the patch. Thanks
Assignee: nobody → johu
It's John Lu not John Hu. Thanks.
Assignee: johu → jlu
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.
Flags: needinfo?(james.zhang)
Flags: needinfo?(dliang)
(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.
Flags: needinfo?(yang.zhao) → needinfo?(james.zhang)
(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.
Flags: needinfo?(timdream)
The errors are inconclusive. I re-run the tests. We can check it tomorrow.
Flags: needinfo?(timdream)
(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!
Flags: needinfo?(timdream)
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
Flags: needinfo?(timdream)
(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!^_^
Flags: needinfo?(timdream)
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.
Flags: needinfo?(timdream)
(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.
Flags: needinfo?(james.zhang)
(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 <ying.xu@spreadtrum.com>
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
You need to log in before you can comment on or make changes to this bug.