1043854 - FFOS [flame] FmRadio is turned on while turning on/off airplay mode.

Status: RESOLVED DUPLICATE of bug 1042478

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

Description

[孟宪茂]

steps to reproduce:

1.turn on FmRadio app,then stop FmRadio by click the stop button;

2.show utility tray,and start the airplay mode;

3.turn off the airplay mode;


test result:

1.when turn on the the airplay mode,the FmRadio will be turned on for about 2second;

2.when turn off the the airplay mode,FmRadio will be turned.

Comment 1

[孟宪茂]

The main cause is that:

the addEventListener of 'statechange' is not been removed successfully in airplay mode helper，when opening the FmRadio at the first time.

Comment 2

[Peipei Cheng (needinfo if you need my action)]

Just FYI.
If there is no SIM card, this issue cannot be reproduced. 
When there is SIM card inserted, it took longer time to turn off and on airplane mode. Not sure if it's related.

Comment 3

[Pin Zhang [:pzhang] (inactive)]

(In reply to 孟宪茂 from comment #1)
> The main cause is that:
> 
> the addEventListener of 'statechange' is not been removed successfully in
> airplay mode helper，when opening the FmRadio at the first time.

Hi 宪茂, would you mind make a patch and send a PR? You can flag r? on me for review, thanks.

Comment 4

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

Looks like Pin is handling it :)

Comment 5

[孟宪茂]

Attachment: airplay_mode.patch

Comment 6

[孟宪茂]

(In reply to Pin Zhang [:pzhang] from comment #3)
> (In reply to 孟宪茂 from comment #1)
> > The main cause is that:
> > 
> > the addEventListener of 'statechange' is not been removed successfully in
> > airplay mode helper，when opening the FmRadio at the first time.
> 
> Hi 宪茂, would you mind make a patch and send a PR? You can flag r? on me for
> review, thanks.

Hi Pin Zhang，
Please help review the patch.
Thanks!

Comment 7

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8463196 [details] [diff] [review]
airplay_mode.patch

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

Hi 宪茂, thanks for your patch. As your patch, the problem here is that the same callback could be registered as event listener multiple times (in |addEventListener|, the event listener is simply cached by appending to an array without checking if it has been registered), but only the first one is removed when calling |removeEventListener|. So the right way to go should be register callback function only once by checking if it's already registered when calling |addEventListener|.

::: shared/js/airplane_mode_helper.js
@@ +57,5 @@
>      },
>      removeEventListener: function(eventName, callback) {
>        if (eventName === kEventName) {
> +        for (var i = 0; i < this._callbacks.length; i++) {
> +          if(this._callbacks[i].name === callback.name){

Nits, s/(/ (/, please leave a space before the opening bracket, and s/)/) /, i.e. leave a space after the closing bracket.

I don't think this is the right way to go, you can't rely on the function name, name could be shared by anonymous functions, like:

  var callbacks = [];
  // Push an anonymous function with name |a|.
  callbacks.push(function a() {});
  // Push another anonymous function with the same name |a|.
  callbacks.push(function a() {});
  // true
  console.log(callbacks[0].name == callbacks[1].name);
  // false
  console.log(callbacks[0] == callbacks[1]);

@@ +60,5 @@
> +        for (var i = 0; i < this._callbacks.length; i++) {
> +          if(this._callbacks[i].name === callback.name){
> +            this._callbacks.splice(i, 1);
> +          }
> +         };

Nits, wrong indention, and no semicolon.

BTW, this kinda nits could be found by running |gjslint --nojsdoc airplane_mode_helper.js|[1].

[1] https://developer.mozilla.org/fr/Firefox_OS/Platform/Gaia/Hacking#Additional_rules

Comment 8

[孟宪茂]

(In reply to Pin Zhang [:pzhang] from comment #7)
> Comment on attachment 8463196 [details] [diff] [review]
> airplay_mode.patch
> 
> Review of attachment 8463196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi 宪茂, thanks for your patch. As your patch, the problem here is that the
> same callback could be registered as event listener multiple times (in
> |addEventListener|, the event listener is simply cached by appending to an
> array without checking if it has been registered), but only the first one is
> removed when calling |removeEventListener|. So the right way to go should be
> register callback function only once by checking if it's already registered
> when calling |addEventListener|.
> 
> ::: shared/js/airplane_mode_helper.js
> @@ +57,5 @@
> >      },
> >      removeEventListener: function(eventName, callback) {
> >        if (eventName === kEventName) {
> > +        for (var i = 0; i < this._callbacks.length; i++) {
> > +          if(this._callbacks[i].name === callback.name){
> 
> Nits, s/(/ (/, please leave a space before the opening bracket, and s/)/) /,
> i.e. leave a space after the closing bracket.
> 
> I don't think this is the right way to go, you can't rely on the function
> name, name could be shared by anonymous functions, like:
> 
>   var callbacks = [];
>   // Push an anonymous function with name |a|.
>   callbacks.push(function a() {});
>   // Push another anonymous function with the same name |a|.
>   callbacks.push(function a() {});
>   // true
>   console.log(callbacks[0].name == callbacks[1].name);
>   // false
>   console.log(callbacks[0] == callbacks[1]);
> 
> @@ +60,5 @@
> > +        for (var i = 0; i < this._callbacks.length; i++) {
> > +          if(this._callbacks[i].name === callback.name){
> > +            this._callbacks.splice(i, 1);
> > +          }
> > +         };
> 
> Nits, wrong indention, and no semicolon.
> 
> BTW, this kinda nits could be found by running |gjslint --nojsdoc
> airplane_mode_helper.js|[1].
> 
> [1]
> https://developer.mozilla.org/fr/Firefox_OS/Platform/Gaia/
> Hacking#Additional_rules


Hi Pin Zhang,

Thank you for your advices.

1.It is all right that "only the first one is removed when calling |removeEventListener|";

2.use the function name to remove the callback may not the good way, but mostly the callbacks being removed are not same,as they  all have meaningful names.

BUT the current design in airplane_mode_helper.js, when the 'statechange' happen,we can not remove the callback.

    addEventListener: function(eventName, callback) {
      if (eventName === kEventName) {
        this._callbacks.push(callback);
        console.log('-->' + this._callbacks.indexOf(callback));//the result is 0
      }

     removeEventListener: function(eventName, callback) {
       if (eventName === kEventName) {
        var index = this._callbacks.indexOf(callback);
        console.log(--> "The index now is " + index);//the result is -1
        if (index >= 0) {
          this._callbacks.splice(index, 1);
        }

Just using the way "this._callbacks.splice(index, 1);" to remove the callback does not work,as the callback is in the different action scope,they are not exactly the same.

In my opinion,using the function name to remove the callback is fine.
Could you have suggestion to fix this issue?

Comment 9

[Pin Zhang [:pzhang] (inactive)]

Hi 宪茂, my suggestion is modifying |addEventListener| instead of |removeEventListener|, i think checking if the callback already be registered will be enough, so the code looks like:

  addEventListener: function(eventName, callback) {
    if (eventName === kEventName && this._callbacks.indexOf(callback) === -1) {
      this._callbacks.push(callback);
    }
  }

does it make sense?

Comment 10

[孟宪茂]

(In reply to Pin Zhang [:pzhang] from comment #9)
> Hi 宪茂, my suggestion is modifying |addEventListener| instead of
> |removeEventListener|, i think checking if the callback already be
> registered will be enough, so the code looks like:
> 
>   addEventListener: function(eventName, callback) {
>     if (eventName === kEventName && this._callbacks.indexOf(callback) ===
> -1) {
>       this._callbacks.push(callback);
>     }
>   }
> 
> does it make sense?

Hi  Pin Zhang,

It seem that  it does make any sense,as the callback has been registered.

From my log,the callback is already been registered.
 console.log('-->' + this._callbacks.indexOf(callback));//the result is 0;

When we remove the callback,we can not find the callback.Actually，it is in the first place of _callbacks.
My log is bellow:
console.log(--> "The index now is " + index);//the result is -1

The main cause is that callback to be pushed and removed is in the different action scope,they are not exactly the same.So we can not remove it.

So I think we should modify |removeEventListener|,and change the removing strategy,not the |addEventListener|.

Is it right?

Comment 11

[Pin Zhang [:pzhang] (inactive)]

OK ... so could you give me more detailed information about when is the callback is registered and unregistered? It would be helpful if you give the exactly where the code is, thanks!

Comment 12

[Pin Zhang [:pzhang] (inactive)]

BTW, could you help to check if bug 1042478 fix this? You can get the patch from here:
  https://bugzilla.mozilla.org/attachment.cgi?id=8460719

Comment 13

[孟宪茂]

(In reply to Pin Zhang [:pzhang] from comment #11)
> OK ... so could you give me more detailed information about when is the
> callback is registered and unregistered? It would be helpful if you give the
> exactly where the code is, thanks!

The process of registering and unregistering is:

1.When opening the Fm app,call AirplaneModeHelper.ready(which is in /gaia/shared/js/airplay_mode_helper.js):
 window.addEventListener('load', function(e) {
  AirplaneModeHelper.ready(function() {
    airplaneModeEnabled = AirplaneModeHelper.getStatus() == 'enabled';

2.If the AirplaneModeHelper.init() is not ready,it can not get _cachedStatus;
so in the ready attribute of AirplaneModeHelper,the status of this._cachedStatus === '' is true;
then register the callback (function onChangeEvent()) using the addEventListener attribute of AirplaneModeHelper;

3.At this moment,it has got _cachedStatus,and the status of this._cachedStatus !== '' is true;
then unregister the callback (function onChangeEvent()) using the removeEventListener attribute of AirplaneModeHelper;

4.If the callback is removed normally,it can not |addEventListener| the 'statechange',and AirplaneModeHelper.ready executes only once;
If the callback can not been removed, when the 'statechange' event happen,AirplaneModeHelper.ready will execute,then the init() of FmRadio executes and then FmRadio is opened.

Besides,in [2],if AirplaneModeHelper.init() is ready and get the _cachedStatus,the callback of registering and unregistering will not happen,the Fm function is normal.

Actually,if the case in [2] happened,every time you turn on and off airplay mode,the FmRadio will be opened twice,as the 'statechange' has status "enabling" and "enabled".

Do you have any suggestion?
Thanks!

Comment 14

[孟宪茂]

Hi peipei,

The issue seems more serious.Please help add the owner airplay mode to check the issue.
Thanks!

Comment 15

[孟宪茂]

(In reply to Pin Zhang [:pzhang] from comment #12)
> BTW, could you help to check if bug 1042478 fix this? You can get the patch
> from here:
>   https://bugzilla.mozilla.org/attachment.cgi?id=8460719

It seem that it is related to this issue,it may sovle the callback's difference,I will check it.
Thanks!

Comment 16

[Peipei Cheng (needinfo if you need my action)]

Pin, is there any risk of landing the fix for 1042478 in V1.4?

Comment 17

[Pin Zhang [:pzhang] (inactive)]

(In reply to Peipei Cheng from comment #16)
> Pin, is there any risk of landing the fix for 1042478 in V1.4?

No, i don't think there is anything risk to land it in v1.4.

Comment 18

[Peipei Cheng (needinfo if you need my action)]

I didn't encounter this problem after applying the patch of bug 1042478. So it should be the same reason. 
duplicate to bug 1042478.