1141973 - [Settings]Enable SIM PIN, go back to Settings main view, but the status under "SIM Security" is still "Disabled".

Status: VERIFIED FIXED

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

Description

[Shine(Leave from Mozilla)]

[1.Description]:
[Nexus 5][v2.2 & v3.0][Settings]Toggle on "SIM PIN" to enable it, then go back to Settings main view.  But the status under "SIM Security" will not update, it is still "Disabled".
See attachment: Video.MP4 & logcat_1547.txt
Found time: 15:47

[2.Testing Steps]: 
Precondition: Insert a SIM card in device.
1. Launch Settings.
2. Tap "SIM Security".
3. Toggle on "SIM PIN" -> Set a PIN code -> Tap "Done".
4. Tap "<" icon to go back to Settings main view.


[3.Expected Result]: 
4. The status under "SIM Security" should update to "Enabled".

[4.Actual Result]: 
4. The status under "SIM Security" is still "Disabled".

[5.Reproduction build]: 
N5 v2.2 build:
Build ID               20150310002536
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1cda026f8996
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150310.042021
Firmware Date          Tue Mar 10 04:20:36 EDT 2015
Bootloader             HHZ12d

N5 v3.0 build:
Gaia-Rev        943c8b4039f59b08ba100390e164a076a20c892e
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/a9aff724afc7
Build-ID        20150310160234
Version         39.0a1
Device-Name     hammerhead
FW-Release      5.0
FW-Incremental  eng.cltbld.20150310.191407
FW-Date         Tue Mar 10 19:14:23 EDT 2015
Bootloader      HHZ12d
Flame 2.2 build:
Build ID               20150310002536
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1cda026f8996
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150310.042346
Firmware Date          Tue Mar 10 04:23:56 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

[8.Note]:
1. After disable the SIM PIN, the status under "SIM Security" can not update too, it is still "Enabled". Relaunch Settings app, the status will update.
2. This problem can be repro on N5 v3.0, and it cannot repro on Flame2.2.

Comment 1

[Shine(Leave from Mozilla)]

Attachment: Video

Comment 2

[Shine(Leave from Mozilla)]

Attachment: logcat

Comment 3

[Eric Chang [:ericcc] [:echang]]

Not sure if this is a dup, adding 'see also' first, https://bugzilla.mozilla.org/show_bug.cgi?id=1141973

Comment 4

[Hermes Cheng (inactive after July 27, 2015)]

I don't think it is a dup of bug 1138858.
NI settings developer for more information.

Comment 5

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

EJ, could you help check this issue? Thanks.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Comment 7

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Arthur, I just checked the source code and there are some problems I found and I need to discuss with you for more details first.

[Summary]
Check my patch !!

[TL;DR]
Not sure whether you are still remembering the case or not - if we lock / unlock sim card, for some devices (including Nexus 5), there is no "cardstatechange" emitting out from Gecko. After some testing, if we lock the card first and then reboot, the cardstate would be set to "pinRequired", so if we tried to unlock it, we would definitely get the "cardstatechange" for ONLY once ! After that, no matter how we lock / unlock the sim card, it will always be "normal" state, so that's why we won't trigger "cardstatechange".

And after taking a look at codes for simSecurityItem, I noticed that I also used the eventListener in a wrong way, so this may cause another problem that even if Gecko did send the right event back, Gaia can't retrieve it. I should use "window.addEventListener" instead. For more reference, please check this link : https://github.com/mozilla-b2g/gaia/blob/master/shared/js/simslot.js#L95

So, I got two different solutions in mind, the first one is to ask whether we can always get the right event "oncardstatechange" from Gecko no matter the card is locked or not. With this, we can make sure our cardstate would always reflect the real information from hardware. But I am not sure is it possible for Gecko to do so (I remembered there must be some reasons to make this not able to happen but I forgot, any comment ?)

For another solution (that's what I did for this current patch), we will keep requesting latest information from icc.getCardLock('pin') to make sure we would always use the latest value from device instead of relying on the event. When trying to do so, I just noticed for "root panel", its own "onBeforeHide" & "onHide" functions are all ignored by settings_service and no one would call that !!! It is really a mistake because we did write a lot of follow-up works for these two situations in root panel, so I think we have to fix this at the same time !

Comment 8

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

I think we should clarify the expected behavior with the RIL team first.

As for the root panel problem, actually the root panel should not be deactivated only when on tablets, so I think we should add that check instead.

Comment 9

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

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #7)
> [TL;DR]
> Not sure whether you are still remembering the case or not - if we lock /
> unlock sim card, for some devices (including Nexus 5), there is no
> "cardstatechange" emitting out from Gecko. After some testing, if we lock
> the card first and then reboot, the cardstate would be set to "pinRequired",
> so if we tried to unlock it, we would definitely get the "cardstatechange"
> for ONLY once ! After that, no matter how we lock / unlock the sim card, it
> will always be "normal" state, so that's why we won't trigger
> "cardstatechange".

Hi HsinYi !

About the simcard cardstatechange problem, do you know who is familiar with this part ? Can you help us forward this ni? flag to the right person ? Thanks :)

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

EJ, let me try to take a close look at this tomorrow :)

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sorry that I couldn't remember specific behaviour on various devices. But the suggestion I'd like to provide is that don't rely on cardstatechange event if we want to reflect the "sim pin enabled status." That's because, it's very likely that we enable or disable the sim pin enabled function but the sim card state remains ready. From the gecko API point of view, we should decouple "cardstatechange event" and "sim pin enabled" things. In this way, we may also be able to decouple from different hardware behaviour better. 

Hope your questions have been answered, EJ :)

Comment 12

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

Arthur, after discussing some details with HsinYi, we all think the better way here is that we have to keep updating the latest status when entering / leaving root panel. But you did mention that we can't let tablet call onHide, so I can't update its value easily, do you have any recommended way for this ?

Comment 13

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

If gecko lacks mechanism notifying gaia about the changes of the security settings, we may have to add a common module wrapping the API and emit the event in gaia. That way we can always keep the item updated.

Comment 14

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

 Okay, that sounds another possible way to do so, let me try to make a new patch for it.

Comment 15

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Arthur, I updated the patch and tried to use a intermediate module called SimPin to pass events. Can you help me review this patch ? Thanks !

Comment 16

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

It may require more change than I originally expected. I was thinking a module that wraps simpin related apis, provides methods for querying the current sim security state, and emits events when settings change, instead of just passing events.

The existing code in `simpin_dialog.js` does the work but now it is tight coupled with UI. We can move the business logic to a separate common module. Then `sim_security_item`, `simpin_dialog`, and any other modules interested in information regarding sim security could simply use the module.

Comment 17

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Arthur, how about this one ?

Comment 18

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

I think we are in the right direction but there are some places need to be refined. Please check my comments in github, thanks.

Comment 19

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

Arthur, I just made another change that may fit your opinions, can you help me check whether this one is good or not ?

For this updated patch, I tried to list down all possible status changes and make sure callee of SimSecurity can use them to do related UI updated if needed.

Thanks :)

Comment 20

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

Looks good! Please continue to write unit tests for the new module.

Comment 21

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

This patch looks better now, can you help me review it, Arthur?

Thanks.

Comment 22

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

The patch is looking good but there are a few things to be addressed before merging, please check them in github, thanks.

Comment 23

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Arthur, all related comments were addressed, can you help me check this patch again ? Big thanks !

Comment 24

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

Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

r=me, thanks a lot!

Comment 25

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#A_nfbmsRQy6bX4MkXhcFvQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 26

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

what the !? tried again.

Comment 27

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#TTGBRLBLSRiWwHAd0iaO2A

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 28

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/0bfa1219536af541d55d59ed00359f95a8698623

Comment 29

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

@Autolander, Goooood job !

Comment 30

[helen（leave from Mozilla）]

Attachment: verified_v3.0.mp4

This bug has been verified as pass on latest build of Nexus 5 v3.0 by the STR in Comment 0.

Results: The status under "SIM Security" will update to "Enabled" when turning SIM PIN on. Then disable the SIM PIN, the status under "SIM Security" update to "Disabled" as expected.

See attachment: verified_v3.0.mp4
Reproduce rate: 0/5


Device: Nexus 5 3.0 build(Pass)
Build ID               20150505160203
Gaia Revision          42dc5f02a9df006b129824cd9bffa93cab937ab2
Gaia Date              2015-05-05 11:06:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5907a8eca521
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150505.192812
Firmware Date          Tue May  5 19:28:29 EDT 2015
Bootloader             HHZ12f

Comment 31

[Norry.L.F(Leave from Mozilla)]

Hi Josh,

Could you help to check Whether it will uplift to v2.2? thanks. :)

Comment 32

[Eric Chang [:ericcc] [:echang]]

Considering the code change is quite a lot, and the user can restart the settings to make it showing correct status, suggest not to uplift to v2.2 at this stage.

Comment 33

[Josh Cheng [:josh]]

Agree to Eric per comment 32.
Mark 2.2 wontfix

Comment 34

[Shine(Leave from Mozilla)]

According to comment 33, set the status of issue to VERIFIED-FIXED.