Closed Bug 988445 Opened 6 years ago Closed 6 years ago

[AirplaneModeHelper] getStatus method does not return the correct state

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.3T wontfix, b2g-v1.4 verified, b2g-v2.0 verified)

VERIFIED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3T --- wontfix
b2g-v1.4 --- verified
b2g-v2.0 --- verified

People

(Reporter: mai, Assigned: eragonj)

References

Details

Attachments

(2 files)

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
Blocks: 987283
blocking-b2g: --- → 1.4?
Sad news. Let me take it. 

Thanks Marina.
Assignee: nobody → ejchen
(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 !?
Flags: needinfo?(mri)
(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.
Status: NEW → ASSIGNED
Hi, 
  this solution is valid for me. Only a question, are you going to send a 'statechange' event when the initialization is finished?

Regards
Flags: needinfo?(mri)
(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 ?
Not, it's perfect for me, I completely agree with you.

Thanks for your quick answer
Hi Marina,

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

Thanks,
Mike
Flags: needinfo?(mri)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Flags: needinfo?(mri)
Tim, can you triage this?
Flags: needinfo?(timdream)
Flags: needinfo?(timdream) → needinfo?(arthur.chen)
Per comment 8 I think this should be a blocker.
Flags: needinfo?(arthur.chen)
blocking-b2g: 1.4? → 1.4+
Attached file patch on master
This is a WIP patch for this bug.
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 :)
Attachment #8399325 - Flags: review?(mri)
Attachment #8399325 - Flags: review?(arthur.chen)
Comment on attachment 8399325 [details] [review]
patch on master

r=me for the settings part with the nit addressed. Thanks!
Attachment #8399325 - Flags: review?(arthur.chen) → review+
(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 on attachment 8399325 [details] [review]
patch on master

Please fix the github comment
good work!
Attachment #8399325 - Flags: review?(mri)
Component: Gaia → Gaia::Settings
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 ;)
Attachment #8399325 - Flags: review?(mri)
Comment on attachment 8399325 [details] [review]
patch on master

It's fine! Thank you for the patch.
Attachment #8399325 - Flags: review?(mri) → review+
(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.
Thanks all, this patch got merged into Gaia/master : 56bac22fb1506e08c20e45a7dde04be79f5e0051
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
It seems that there is no costcontrol related changes on v1.4, I would exclude them on another specific v1.4 patch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8402586 - Flags: approval-gaia-v1.4?(fabrice)
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 :)
Attachment #8402586 - Flags: review?(arthur.chen)
Comment on attachment 8402586 [details] [review]
patch on v1.4

r=me, thanks!
Attachment #8402586 - Flags: review?(arthur.chen) → review+
Attachment #8402586 - Flags: approval-gaia-v1.4?(fabrice) → approval-gaia-v1.4?(release-mgmt)
QA,

Seems to be a big patch. Requesting verifyme on the same.
Keywords: qawanted, verifyme
Keywords: qawanted
Blocks: 991831
Hi all , 

is there any update from release team ?
Tony

Can we please have someone provide us test results? I'd like to see QA input before making a decision on approval.
Flags: needinfo?(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.
Flags: needinfo?(tchung)
Flags: needinfo?(mri)
Flags: needinfo?(ejchen)
@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 !!
Flags: needinfo?(ejchen)
EJ,

You don't need gaia-approval as this one is already 1.4+
Flags: needinfo?(ejchen)
HI, 
I've tested the patch with costcontrol app and it works fine. I agree with EJ, it's a low risk patch.
Flags: needinfo?(mri)
@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 :)
Flags: needinfo?(ejchen)
Attachment #8402586 - Flags: approval-gaia-v1.4?(release-mgmt)
Merged on :

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

Thanks all :)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S6 (25apr)
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
Flags: needinfo?(bbajaj)
(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 ?
Flags: needinfo?(bbajaj) → needinfo?(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.
Flags: needinfo?(wchang)
You need to log in before you can comment on or make changes to this bug.