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

VERIFIED FIXED

Status

VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: yue.xia, Assigned: eragonj)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 wontfix, b2g-master verified)

Details

(Whiteboard: [v2.2-nexus-5-l][flame-l-testing-needed])

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
[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.
(Reporter)

Comment 1

4 years ago
Created attachment 8575878 [details]
Video
(Reporter)

Comment 2

4 years ago
Created attachment 8575879 [details]
logcat
(Reporter)

Updated

4 years ago
status-b2g-v2.2: --- → affected
See Also: → bug 1138858
Not sure if this is a dup, adding 'see also' first, https://bugzilla.mozilla.org/show_bug.cgi?id=1141973
I don't think it is a dup of bug 1138858.
NI settings developer for more information.
Flags: needinfo?(arthur.chen)
EJ, could you help check this issue? Thanks.
Flags: needinfo?(arthur.chen) → needinfo?(ejchen)
Created attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master
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 !
Flags: needinfo?(ejchen)
Attachment #8584334 - Flags: feedback?(arthur.chen)
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.
Attachment #8584334 - Flags: feedback?(arthur.chen)
Assignee: nobody → ejchen
(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 :)
Flags: needinfo?(htsai)
EJ, let me try to take a close look at this tomorrow :)
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 :)
Flags: needinfo?(htsai)
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 ?
Flags: needinfo?(arthur.chen)
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.
Flags: needinfo?(arthur.chen)
 Okay, that sounds another possible way to do so, let me try to make a new patch for it.
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 !
Attachment #8584334 - Flags: review?(arthur.chen)
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.
Attachment #8584334 - Flags: review?(arthur.chen)
Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

Arthur, how about this one ?
Attachment #8584334 - Flags: review?(arthur.chen)
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.
Attachment #8584334 - Flags: review?(arthur.chen) → feedback+
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 :)
Flags: needinfo?(arthur.chen)
Looks good! Please continue to write unit tests for the new module.
Flags: needinfo?(arthur.chen)
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.
Attachment #8584334 - Flags: review?(arthur.chen)
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.
Attachment #8584334 - Flags: review?(arthur.chen)
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 !
Attachment #8584334 - Flags: review?(arthur.chen)
Comment on attachment 8584334 [details] [review]
[gaia] EragonJ:bug-1141973 > mozilla-b2g:master

r=me, thanks a lot!
Attachment #8584334 - Flags: review?(arthur.chen) → review+
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
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.
what the !? tried again.
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
@Autolander, Goooood job !
Created attachment 8601984 [details]
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
QA Whiteboard: [MGSEI-Triage+]
status-b2g-master: --- → fixed
status-b2g-master: fixed → verified
Hi Josh,

Could you help to check Whether it will uplift to v2.2? thanks. :)
Flags: needinfo?(jocheng)
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.
Whiteboard: 2.2-nexus-5-l → [v2.2-nexus-5-l][flame-l-testing-needed]

Comment 33

4 years ago
Agree to Eric per comment 32.
Mark 2.2 wontfix
status-b2g-v2.2: affected → wontfix
Flags: needinfo?(jocheng)
(Reporter)

Comment 34

4 years ago
According to comment 33, set the status of issue to VERIFIED-FIXED.
Status: RESOLVED → VERIFIED

Updated

3 years ago
Depends on: 1205596
Depends on: 1232840
You need to log in before you can comment on or make changes to this bug.