1110660 - [Flame] The SIM2's PIN1 code will not pop up when user only insert SIM card to slot 2.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Shally Li（leave from Mozilla）]

Attachment: logcat_1535.txt

[1.Description]:
[Flame v2.1&v2.2][First Time Experience]The SIM2's PIN1 code will not pop up and no SIM2 signal on notifications bar when user only insert SIM card to slot 2 and then power on device.
Attch:SIM2_PIN1.mp4 and logcat_1535.txt
Found time:15:35

[2.Testing Steps]: 
1.Insert SIM2 card with PIN1 code to Slot2 (Slot 1 is empty).
2.Power on device.
3.Tap Settings->SIM manager->SIM Security.
4.Disable PIN1 code.
** SIM2 can be identified normally and call out successfully, the signal will display on notifications bar.

[3.Expected Result]: 
2.The SIM2's PIN1 code should pop up to prompt the user input PIN1 code.

[4.Actual Result]: 
2.The SIM2's PIN1 code will not pop up and no SIM2 signal shows on notifications bar.

[5.Reproduction build]: 
Flame 2.1 Build:
Gaia-Rev        97873dca486abf4162a3345e71b375806937bdec
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9faa165ac85d
Build-ID        20141211001204
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141211.034507
FW-Date         Thu Dec 11 03:45:22 EST 2014
Bootloader      L1TC00011880

Flame 2.2 Build:
Gaia-Rev        1a956437210d2b16988d2ddbf40c9a38d8474435
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/d92db71d4e67
Build-ID        20141211040208
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141211.074025
FW-Date         Thu Dec 11 07:40:36 EST 2014
Bootloader      L1TC00011880

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

Comment 1

[Shally Li（leave from Mozilla）]

Attachment: SIM2_PIN1.MP4

Comment 2

[Shally Li（leave from Mozilla）]

[7.Note]
If user insert SIM1 card without PIN1 code and SIM2 card with PIN1 code, restart device, the SIM2's PIN1 code also doesn't pop up and only show SIM1 signal in status bar.

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

Guillaume, can you take a look here?

Comment 4

[siiaceon]

1007948 related also

Comment 5

[Yang Yang]

Alberto, I think this problem is happend by these changes below (for bug 1090859):

 // Always showing the first slot first.
- if (!this._alreadyShown && index > 1) {
+ if (!this._alreadyShown && index > 0) {
return false;
}

if there is only sim2 is locked, the sim2 unlock dialog will never open 
because the case (!this._alreadyShown && index > 0) is always be true.

I want to know for what reason you add the case (!this._alreadyShown && index > 0).

Thank you very much for your kindly reply.

Comment 6

[Yang Yang]

Attachment: no_sim2_unlock_dialog.patch

Comment 7

[Yang Yang]

To solve this problem, i made some modify: 

(1)add the case for two slots both locked.

-      if (!this._alreadyShown && index > 0) {
+      var _isBothSlotsLocked = isBothSlotsLocked();
+      if (_isBothSlotsLocked && (!this._alreadyShown && index > 0)) {

(2)besides, let (this._alreadyShown = false;) at the head of init function,
because in the showIfLocked function, the flag this._alreadyShown might change to true.


Alberto, please review if my changes can slove this problem, thank you.
The particular modification can be find in the attachment no_sim2_unlock_dialog.patch.

Comment 8

[Candice Serran (:cserran)]

assigning to yang.yang who has patch

Comment 9

[Alberto Pastor [:albertopq]]

Is this patch for 2.1? It seems that sim_lock.js doesn't exist anymore in master. Yang Yang, could you please provide a patch for master, and then we'll uplift it to 2.1? Let me know if you need any help.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Lock screen was refactored since 2.1, so we'll need 2.1 (if we are considering uplift) and 2.2/master specific patches. Cc-ing Greg Weng who has been dealing with this.

Comment 11

[Yang Yang]

(In reply to Alberto Pastor [:albertopq] from comment #9)
> Is this patch for 2.1? It seems that sim_lock.js doesn't exist anymore in master. 
 
  Yes, my previous patch is for 2.1.

> Yang Yang, could you please provide a patch for master, and then
> we'll uplift it to 2.1? Let me know if you need any help.

  I made a patch for master, please review it: no_sim2_unlock_dialog_master.patch.

Comment 12

[Yang Yang]

Attachment: patch for master

Comment 13

[Alberto Pastor [:albertopq]]

Comment on attachment 8544952 [details] [diff] [review]
patch for master

The patch looks good to me. I cannot review it, as I'm not a peer, so forwarding the review to :alive.

My only comment is that we should add some unit tests.

Thanks!

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8544952 [details] [diff] [review]
patch for master

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

1. Avoid to create a new function inside a function; please move the isBothLocked function into SimLockManager.prototype.isBothLocked
2. Please create a github pull request
3. Please have a simple unit test in sim_lock_manager_test.js

Thank you!

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alberto Pastor [:albertopq] from comment #13)
> Comment on attachment 8544952 [details] [diff] [review]
> patch for master
> 
> The patch looks good to me. I cannot review it, as I'm not a peer, so
> forwarding the review to :alive.
> 
> My only comment is that we should add some unit tests.
> 
> Thanks!

You have my words to gain the ability to review sim lock stuff from now on :)

Comment 16

[Alberto Pastor [:albertopq]]

Comment on attachment 8543748 [details] [diff] [review]
no_sim2_unlock_dialog.patch

Please, flag me again after fix :alive's comments on master.

Thanks!

Comment 17

[Yang Yang]

Github pull-request:    https://github.com/mozilla-b2g/gaia/pull/27291

Alberto & Alive, please review it, thank you

Comment 18

[Yang Yang]

(In reply to Yang Yang from comment #17)
> Github pull-request:    https://github.com/mozilla-b2g/gaia/pull/27291
> 
> Alberto & Alive, please review it, thank you

I am sorry, 27291 is closed

please review this one:
https://github.com/mozilla-b2g/gaia/pull/27292

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Hi, please append a new request (or edit pr 27292's title and reopen the pr) and obsolete the old request. Also, alberto will review this from now on.

Comment 20

[Yang Yang]

Attachment: [master] pull request

Comment 21

[Alberto Pastor [:albertopq]]

Comment on attachment 8547343 [details] [review]
[master] pull request

The code looks good to me, but there are some Linting errors. Also, some unit tests are failing https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=bddda8608af7 (I think a isLocked() stub is missing). Please, reflag me after fixing those.

Thanks!

Comment 22

[Yang Yang]

Attachment: patch for master

Comment 23

[Yang Yang]

(In reply to Alberto Pastor [:albertopq] from comment #21)
> Comment on attachment 8547343 [details] [review]
> [master] pull request
> 
> The code looks good to me, but there are some Linting errors. Also, some
> unit tests are failing
> https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=bddda8608af7
> (I think a isLocked() stub is missing). Please, reflag me after fixing those.
> 
> Thanks!

albertopq, please review again, thank you

Comment 24

[Alberto Pastor [:albertopq]]

Comment on attachment 8552319 [details] [review]
patch for master

Good work! Just one more little thing. Can we add a simple test that checks isBothSlotsLocked logic? Right now, all the tests are mocking that method. Just trying to avoid changes to the slots affecting this. r=me as soon as that's added. Also, remember to set r=albertopq at the end of the pull request title.
Thanks!

Comment 25

[Yang Yang]

Attachment: [master] pull request: https://github.com/mozilla-b2g/gaia/pull/27621

Comment 26

[Alberto Pastor [:albertopq]]

Comment on attachment 8553567 [details] [review]
[master] pull request:  https://github.com/mozilla-b2g/gaia/pull/27621

Great job! Thanks!

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/df9f4f23147c8c9adbcf2ff5c0a065ab408b34ee

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

Kevin, do we want this for 2.1?

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8553567 [details] [review]
[master] pull request:  https://github.com/mozilla-b2g/gaia/pull/27621

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Bug 1090859.

[User impact] if declined:
Disabling SIM2's pin when booting device doesn't work.

[Testing completed]:
Manually tested, added unit tests.

[Risk to taking this patch] (and alternatives if risky):
This patch effects how we display the SIM pin dialog when booting the phone, and makes sure we show the SIM pin in more cases. If anything it is more risky not to accept this patch.

[String changes made]: none.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/9bc5a10d2fc2f472ffeba061b8b47a5523f6b004

Comment 31

[Yang Yang]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #30)
> v2.2:
> https://github.com/mozilla-b2g/gaia/commit/
> 9bc5a10d2fc2f472ffeba061b8b47a5523f6b004

Because 2.1 has the same problem, we also want this for 2.1

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Yang Yang from comment #31)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #30)
> > v2.2:
> > https://github.com/mozilla-b2g/gaia/commit/
> > 9bc5a10d2fc2f472ffeba061b8b47a5523f6b004
> 
> Because 2.1 has the same problem, we also want this for 2.1

Please request 2.1 uplift approval, like I did in comment 29 for 2.2.

Comment 33

[Yang Yang]

Comment on attachment 8553567 [details] [review]
[master] pull request:  https://github.com/mozilla-b2g/gaia/pull/27621

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Bug 1090859.

[User impact] if declined:
Disabling SIM2's pin when booting device doesn't work.

[Testing completed]:
Manually tested, added unit tests.

[Risk to taking this patch] (and alternatives if risky):
This patch effects how we display the SIM pin dialog when booting the phone, and makes sure we show the SIM pin in more cases. If anything it is more risky not to accept this patch.

[String changes made]: none.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Needs a rebasing for v2.1 uplift.

Comment 35

[GitHub Autolander Bot]

Attachment: [PullReq] YangYangSPRD:bug1110660_v2.1 to mozilla-b2g:v2.1

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8560228 [details] [review]
[PullReq] YangYangSPRD:bug1110660_v2.1 to mozilla-b2g:v2.1

This has Gaia unit test failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=498982&repo=gaia-try

Comment 37

[Yang Yang]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #36)
> Comment on attachment 8560228 [details] [review]
> [PullReq] YangYangSPRD:bug1110660_v2.1 to mozilla-b2g:v2.1
> 
> This has Gaia unit test failures.
> https://treeherder.mozilla.org/logviewer.html#?job_id=498982&repo=gaia-try

I am working for it now。

Comment 38

[GitHub Autolander Bot]

Attachment: [PullReq] YangYangSPRD:bug1110660_v2.1 to mozilla-b2g:v2.1

Comment 39

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/85a8e878e73e2e068995213ab8c5226587667ba4

Comment 40

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on latest Flame v2.2&3.0.
See attachment: verified_v2.2_(1).mp4 and verified_v2.2_(2).mp4
Reproduce rate: 0/5.

Flame 2.2 build:
Build ID               20150209002504
Gaia Revision          e827781324cbde91d2434b388f5dead3303a85ee
Gaia Date              2015-02-06 20:54:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0552759956d3
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150209.040038
Firmware Date          Mon Feb  9 04:00:51 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 :
Build ID               20150209010211
Gaia Revision          0d7b35f23402c4cb29bca6b98280fec48a196dec
Gaia Date              2015-02-08 20:44:20
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/3436787a82d0
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150209.045655
Firmware Date          Mon Feb  9 04:57:06 EST 2015
Bootloader             L1TC000118D0

Comment 41

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2_(1).MP4

Comment 42

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2_(2).MP4

Comment 43

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on latest 2.1S and Flame v2.1.
See attachment: verified_v2.1_(1).mp4 and verified_v2.1_(2).mp4
Reproduce rate: 0/5.

Flame 2.1 build:
Build ID               20150210002200
Gaia Revision          7dd130a312f12c89b2d41948f8557effa56afbf6
Gaia Date              2015-02-10 06:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2de03dfa9aac
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150210.040336
Firmware Date          Tue Feb 10 04:03:47 EST 2015
Bootloader             L1TC000118D0

2.1S (256m):
Build ID               20150210002201
Gaia Revision          7dd130a312f12c89b2d41948f8557effa56afbf6
Gaia Date              2015-02-10 06:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/5017c32553c7
Gecko Version          34.0
Device Name            scx15_sp7715ga
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150210.040821
Firmware Date          Tue Feb 10 04:08:33 EST 2015

2.1S (512m):
Build ID               20150210161202
Gaia Revision          7dd130a312f12c89b2d41948f8557effa56afbf6
Gaia Date              2015-02-10 06:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/df6f80e36912
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  122
Firmware Date          Thu Feb  5 12:42:58 CST 2015

Comment 44

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1_(1).MP4

Comment 45

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1_(2).MP4

Comment 46

[Kai-Chih Hu [:khu]]

clear ni because it's in 2.1.