907505 - [System] Screen_Manager registers duplicated event listeners

Status: RESOLVED FIXED

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

Description

[Jinho Hwang [:jeffhwang]]

Attachment: screen_manager.patch

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Screen turning on/off many times


Actual results:

In screen_manager.js, System App keep registering listeners for the below events
 1. devicelight
 2. will-unlock
 3. lockpanelchang

Comment 1

[Jinho Hwang [:jeffhwang]]

The attachment is a test patch to avoid the duplicated event listener registering.

Comment 2

[Jinho Hwang [:jeffhwang]]

There is one more event which System App keeps asking to add without removing.
  onwifi-statuschange  from statusbar.js

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 793236 [details] [diff] [review]
screen_manager.patch

Review of attachment 793236 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like there are some scoping issues, maybe |stopShortIdleTimeout| is meant to be bound to |ScreenManager|?

::: apps/system/js/screen_manager.js
@@ +299,4 @@
>      var screenOff = function scm_screenOff() {
>        self._setIdleTimeout(0);
>  
> +      window.removeEventListener('devicelight', this);

typo? Looks like you still want |self| here.

@@ +402,5 @@
>        this._setIdleTimeout(10, true);
>        var self = this;
> +      self._listenerSet = true;
> +      window.addEventListener('will-unlock', self.stopShortIdleTimeout);
> +      window.addEventListener('lockpanelchange', self.stopShortIdleTimeout);

probably worth putting those 3 lines in a |startShortIdleTimeout| method.

@@ +498,5 @@
>      var idleCallback = function idle_proxy() {
>        self.turnScreenOff(instant, 'idle_timeout');
> +      if(self._listenerSet) {
> +        window.removeEventListener('will-unlock', self.stopIdleTimeout);
> +        window.removeEventListener('lockpanelchange', self.stopIdleTimeout);

do you mean |self.stopShortIdleTimeout|?

Comment 4

[Jinho Hwang [:jeffhwang]]

(In reply to Etienne Segonzac (:etienne) from comment #3)
Thank you, Etienne, for the feedback.

And sorry I didn't explain much about this problem and the patch was just for test.
The problem is that we don't remove event listeners which we added when the screen turns on.
In detail, we don't remove the two event listeners( for events 'will-unlock' and 'lockpanelchange') in below cases. 
 case 1. user turns off the screen on lockscreen. 
 case 2. the idle timer fired.

We only remove them when the event handler(stopShortIdleTimeout) got called.


Additionally, we keep removing the listener for the 'devicelight' event without adding it, if the _deviceLightEnabled is setted to false.

I'll attach a patch for this problem but I'm not good at JS.
So, probably it's better to find someone for this bug.

Comment 5

[Jinho Hwang [:jeffhwang]]

Attachment: screen_manager.patch

Comment 6

[Etienne Segonzac (:etienne)]

Attachment: Updated patch with test fix

The patch looks good.
Just updated the test accordingly in this version.

Alive can you have a look at Jinho's patch?
I'm having a hard time figuring out if this breaks bug 880609.

Comment 7

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

Comment on attachment 799464 [details] [diff] [review]
Updated patch with test fix

Review of attachment 799464 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me.

::: apps/system/js/screen_manager.js
@@ -392,5 @@
>      } else if (LockScreen.locked) {
>        this._setIdleTimeout(10, true);
> -      var self = this;
> -      var stopShortIdleTimeout = function scm_stopShortIdleTimeout() {
> -        window.removeEventListener('will-unlock', stopShortIdleTimeout);

var stopShortIdleTimeout = function scm_stopShortIdleTimeout() {
I think this is root cause...mismatch function name OOPS...
But the way of this patch should also be fine.

Comment 8

[Etienne Segonzac (:etienne)]

Jhino, can you send a pull request to gaia or upload a |git format-patch| here so that we can get this formally reviewed/landed?

Thanks!

Comment 9

[Jinho Hwang [:jeffhwang]]

Attachment: Patch URL to pull request #11980

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 800606 [details]
Patch URL to pull request #11980

Forwarding the review.

Comment 11

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

Comment on attachment 800606 [details]
Patch URL to pull request #11980

Basically r+ but travis is unhappy now.
Please try 'git commit --amend' and change the commit message and force push again to get travis work again.

Comment 12

[Jinho Hwang [:jeffhwang]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11980/files

Pointer to Github pull-request

Comment 13

[Jinho Hwang [:jeffhwang]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11980/files

Pointer to Github pull-request

Comment 14

[Jinho Hwang [:jeffhwang]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12232/files

Pointer to Github pull-request

Comment 15

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

https://github.com/mozilla-b2g/gaia/commit/327bc39dd806b6e532ddc125c3d7fa8534a92802