988445 - [AirplaneModeHelper] getStatus method does not return the correct state

Status: VERIFIED FIXED

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

Description

[Marina Rodríguez [:mai]]

Currently the AirplaneModeHelper module does not inform when its initialization is finished.

This module has an asynchronous initialization, and its method getStatus is synchronous.For this reason, when an app calls the getStatus method it could receive a unexpected value (the expected values are 'enabled' or 'disabled').

The only way to deal with this behaviour is checking if the returned value for this method is different than the expected values and, if this occurs, putting a 'changestate' event listener to wait for the correct status value.

IMHO, this module should also have a field to indicate wether the initialization is finished. Maybe, it' would be fantastic that the module would send an specific event to notify the finishing of its initialization.

Regards

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Sad news. Let me take it. 

Thanks Marina.

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to marina rodríguez [:mai] from comment #0)
> Currently the AirplaneModeHelper module does not inform when its
> initialization is finished.
> 
> This module has an asynchronous initialization, and its method getStatus is
> synchronous.For this reason, when an app calls the getStatus method it could
> receive a unexpected value (the expected values are 'enabled' or 'disabled').
> 
> The only way to deal with this behaviour is checking if the returned value
> for this method is different than the expected values and, if this occurs,
> putting a 'changestate' event listener to wait for the correct status value.
> 
> IMHO, this module should also have a field to indicate wether the
> initialization is finished. Maybe, it' would be fantastic that the module
> would send an specific event to notify the finishing of its initialization.
> 
> Regards

Hi Marina,

after discussion with Arthur, we think it would be better to wrap getStatus() inside ready(). Here comes our proposal : 

AirplaneModeHelper.ready(function() {
  var status = AirplaneModeHelper.getStatus();
  // do whatever you want with status
});

I think this change would make it easier to use instead of listening lots of evernts. 

What do you think !?

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #2)

Currently, only getStatus() related functions need to be put inside the ready function like getStatus() and setEnabled(), while the others like add/removeEventListeners will not be influenced in this change.

I would put more details and how to use information on its top comments.

Comment 4

[Marina Rodríguez [:mai]]

Hi, 
  this solution is valid for me. Only a question, are you going to send a 'statechange' event when the initialization is finished?

Regards

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to marina rodríguez [:mai] from comment #4)
> Hi, 
>   this solution is valid for me. Only a question, are you going to send a
> 'statechange' event when the initialization is finished?
> 
> Regards

I don't think so because for users, they don't need to know when this event happens if they just want to use getStatus(). All they need to know is `getStatus()` and do whatever they want and that's it ! 

Do you have any concern about this ?

Comment 6

[Marina Rodríguez [:mai]]

Not, it's perfect for me, I completely agree with you.

Thanks for your quick answer

Comment 7

[Mike Lee [:mlee]]

Hi Marina,

Is the end-user impact that a phone user may see an incorrect Airplane Mode enabled/disabled status? 

Thanks,
Mike

Comment 8

[Marina Rodríguez [:mai]]

Hi Mike,
I don't know what is the better answer for your question because it depends of the application and the use of this method.

On my case, the user will see an incorrect status and also if the app doesn't detect if the airplane Mode status is enabled, the listners to detect when the airplane mode is disabled are not activated. For this reason my app does not restart automatically when the airplanemode is disconected and the user has to close and reopen the app.

Regards.

Comment 9

[Gregor Wagner [:gwagner]]

Tim, can you triage this?

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Per comment 8 I think this should be a blocker.

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

This is a WIP patch for this bug.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8399325 [details] [review]
patch on master

Arthur,
can you help me review the settings part of this bug ? 

Marina, 
can you help me review the costcontrol part of this bug (If you are not able to review, please help me bypass to the right one :P)

I already did a quick test to make sure there is nothing broken in this patch, but I still need your feedback and please point out the missing part. 

Thank you guys :)

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8399325 [details] [review]
patch on master

r=me for the settings part with the nit addressed. Thanks!

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Arthur Chen [:arthurcc] from comment #13)
> Comment on attachment 8399325 [details] [review]
> patch on master
> 
> r=me for the settings part with the nit addressed. Thanks!

OK ! I just added the check ;)

Comment 15

[Marina Rodríguez [:mai]]

Comment on attachment 8399325 [details] [review]
patch on master

Please fix the github comment
good work!

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8399325 [details] [review]
patch on master

HI Marina, 

just addressed your comments and it makes senses to lazyload scripts first. 

Any suggestions about this patch is welcome, thanks ;)

Comment 17

[Marina Rodríguez [:mai]]

Comment on attachment 8399325 [details] [review]
patch on master

It's fine! Thank you for the patch.

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to marina rodríguez [:mai] from comment #17)
> Comment on attachment 8399325 [details] [review]
> patch on master
> 
> It's fine! Thank you for the patch.

Thanks for your quick review, Marina. I would start to squash the commits and merge to master.

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, this patch got merged into Gaia/master : 56bac22fb1506e08c20e45a7dde04be79f5e0051

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

It seems that there is no costcontrol related changes on v1.4, I would exclude them on another specific v1.4 patch

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on v1.4

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8402586 [details] [review]
patch on v1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): AirplaneModeHelper.
[User impact] if declined: There is an flaw in AirplaneModeHelper which would make developers get undefined value about current airplaneMode status.
[Testing completed]: Manual tests and related unit tests have been added.
[Risk to taking this patch] (and alternatives if risky): low.
[String changes made]: no.

Comment 23

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8402586 [details] [review]
patch on v1.4

Arthur,

I just made another clone of this bug excluding costcontrol. It is basically the same patch with the original one in Gaia/master (I just cherry-picked it).

Please help me r+ this patch. Thanks :)

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8402586 [details] [review]
patch on v1.4

r=me, thanks!

Comment 25

[Preeti Raghunath(:Preeti)]

QA,

Seems to be a big patch. Requesting verifyme on the same.

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi all , 

is there any update from release team ?

Comment 27

[Preeti Raghunath(:Preeti)]

Tony

Can we please have someone provide us test results? I'd like to see QA input before making a decision on approval.

Comment 28

[Tony Chung [:tchung]]

EJ / Mai, 

are there any core apps that is part of the production build that will be using this getStatus() method?   i can't tell from the comments above if you say the Usage app does or not.   Regardless, the question is around two things:

1. Risk factors of regressing the current method of airplanemode (you mentioned low risk, so would like to hear more
2. Which apps will use this method, and if so, how can it be verified fixed?

Thanks.

Comment 29

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Tony,

I just did the command on the root of v1.4 gaia folder (`cd apps && ag AirplaneModeHelper -l`) and got files using AirplaneModeHelper : 

costcontrol/js/widget.js
settings/js/airplane_mode.js
settings/js/simcard_manager.js
settings/js/telephony_items_handler.js
settings/js/telephony_settings.js
settings/test/unit/airplane_mode_helper_test.js (test case)
settings/test/unit/mock_airplane_mode_helper.js (test case)
settings/test/unit/simcard_manager_test.js (test case)
system/js/airplane_mode.js (Only in comments, so just ignore this)

You can notice there are only settings app & costcontrol app using this helper. (We narrowed down the impact scope to these two apps). The reason why this bug was reported because in costcontrol app, they found a timing issue when using this function. 

You may got curious about why there is no this problem when I wrote it and tested on settings app is because in settings app, files using AirplaneModeHelper are all under IdleObserver or got lazy-loaded after airplane_mode_helper and this makes the bug unreproducible in settings app. 

To be short, in order to verify the bug, we have to make sure listed files on the list still work as usual. 

@Mai, any suggestions would be welcome !!

Comment 30

[Preeti Raghunath(:Preeti)]

EJ,

You don't need gaia-approval as this one is already 1.4+

Comment 31

[Marina Rodríguez [:mai]]

HI, 
I've tested the patch with costcontrol app and it works fine. I agree with EJ, it's a low risk patch.

Comment 32

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Preeti,

I thought I have to ask approval like what we did in 1.3+ bug. 

Thanks for your information, no matter how, it is still nice to put the risk factor on the bug to let people know what the risk is.

I would start to merge it onto v1.4 by the way.

Thanks :)

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged on :

Gaia/master : 56bac22fb1506e08c20e45a7dde04be79f5e0051
Gaia/v1.4 : c8f916c8569f6ee652237fd10ac925e08cd3d9bc

Thanks all :)

Comment 34

[bmorgan]

1.4: Airplane Mode blocks phone from using SIM card, blocks Usage app from opening
2.0: Airplane Mode blocks phone from using SIM card

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140527040202
Gaia: 6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko: cbe4f69c2e9c
Version: 32.0a1
Firmware Version: 10g-2

Comment 35

[Wei Gao (Spreadtrum)]

Hi Bhavana

I think v1.3t also needs this patch?
But I can't find block 1.3t flag.
Could you help me?

Thanks a great.

Comment 36

[bhavana bajaj [:bajaj]]

(In reply to Wei Gao (Spreadtrum) from comment #35)
> Hi Bhavana
> 
> I think v1.3t also needs this patch?
> But I can't find block 1.3t flag.
> Could you help me?
> 
> Thanks a great.

wayne, can you confirm this block 1.3T and set the blocking flag accordingly so Wei can uplift this ?

Comment 37

[Wayne Chang [:wchang]]

From reading the comments -
low user impact, large patch.

I don't see this as a blocker for 1.3t, not at its current stage.
We're closing 1.3t off now and are avoiding crash landing stuff.