Closed Bug 857837 Opened 7 years ago Closed 7 years ago

Enter NCK/CCK/SPCK popup should pop up at boot up if phone is in network/corporate/serviceprovider locked state

Categories

(Firefox OS Graveyard :: Gaia::System, defect, P3)

x86_64
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 IOT3 (3jun)
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: nsarkar, Assigned: julienw)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(3 files, 1 obsolete file)

The NCK/CCK/SPCK pop up for entering the code should show up at bootup. Currently, the pop up shows up after sometime (20 sec) and during that time, user can access other apps on the phone. This is not correct behavior because when a phone is locked to a network or service provider, it should not be useable with a different SIM (mcc+ mnc).
Bug 832814 is the cause of this problem.
Nivi, whether or not the user is allowed to use the phone for non telephony related functionality is OS dependent. In case of FFOS the user can use the phone for example if the SIM is PIN locked however Android doesn't let user go past the lock screen.

The real issue here is that NCK/CCK/SPCK popup doesn't happen at the boot time but after 20secs which seems pretty weird.
The problem in Bug 832814 was that the NCK popup was happening even if the user gave a good PIN code. The underlying problem was that the RIL was reporting a "networkLocked" status and we were waiting for 5 seconds which was not enough.

If it's possible to have a "registering" status from the RIL while it's trying to register with the pin, then I suppose we can get rid of this timeout at all.

Also, we can be smarter and have a timeout only if we got pinRequired before. But I think this logic should actually be in the RIL or at least the API instead...

We can also lower it at 10s because as I said in Bug 832814 comment 8 that value worked for me too but since it is a time race condition I wanted to feel safer.
Flags: needinfo?(josea.olivera)
(In reply to Julien Wajsberg [:julienw] from comment #3)
> The problem in Bug 832814 was that the NCK popup was happening even if the
> user gave a good PIN code. The underlying problem was that the RIL was
> reporting a "networkLocked" status and we were waiting for 5 seconds which
> was not enough.

We never had a SIM-locked phone to test this properly and there was the issue about the RIL reporting networkLocked state while the SIM PIN dance.

> If it's possible to have a "registering" status from the RIL while it's
> trying to register with the pin, then I suppose we can get rid of this
> timeout at all.
> 
> Also, we can be smarter and have a timeout only if we got pinRequired
> before. But I think this logic should actually be in the RIL or at least the
> API instead...

The RIL plumbing underneath is receiving `networkLocked` SIM card status in devices that don't have such SIM-locked feature on. Do you Anshul why this is possible? IHMO that's not makes sense and this is truly issue here.

> We can also lower it at 10s because as I said in Bug 832814 comment 8 that
> value worked for me too but since it is a time race condition I wanted to
> feel safer.

SGTM
Flags: needinfo?(josea.olivera)
Jose/Julien, RIL is reporting the state of networkLocked as a transient state with substate of ready, which is just a transient state stating that nothing is pending for perso and is part of SIM initialization sequence.

Commercial RIL handles this transient state information correctly and so I would request you to please back out the bug 832814 for the reasons states by nsarakar in this bug.
blocking-b2g: --- → tef?
Network locked feature is supported on v1 and so requesting for TEF+ as this issue causes the NCK bug to pop up after 20 secs of bootup instead of right away.
(In reply to Anshul from comment #5)
> Jose/Julien, RIL is reporting the state of networkLocked as a transient
> state with substate of ready, which is just a transient state stating that
> nothing is pending for perso and is part of SIM initialization sequence.
> 
> Commercial RIL handles this transient state information correctly and so I
> would request you to please back out the bug 832814 for the reasons states
> by nsarakar in this bug.


sorry but what does "correctly" mean exactly? could you please elaborate on which states the ril reports when entering a PIN code? IMHO the pin code use case is a lot more important than the nck use case.
Julien, when app state is perso locked but perso substate is ready, this should be not be considered as network locked. In case where phone is genuinely locked to the network the app state reported would be perso locked but perso substate reported would not be ready but would be say network locked for NCK.

For pin locked, the app state will always be RIL_APPSTATE_PIN and is not effected by bug 832814, which is only for NCK.
David, wdyt about this bug?
Flags: needinfo?(dpv)
Anshul, if I understand correctly, in Bug 832814, we should have checked the "perso substate" instead of raising the timeout ? Do we have this information in the API ? If yes I also think we should backout the patch in Bug 832814 and reopen that bug to fix it the correct way.
Anshul, do you mean that we are not reporting `networkLocked` ICC card state properly? I mean do we have to check perso substate as well?

Sadly current mozMobileConnection API doesn't provide such information (perso substate) so this is a matter of checking perso substate before reporting a truly `networkLocked` ICC card state. If I understand what Anshul is telling us. If so I guess we will be able to get rid of all these timeout workarounds at all.
Jose, yes you need to check for perso substate correctly before reporting network locked.

Julie, this should be taken care of by gecko and not gaia so you need not check for perso substate in gaia to display the network locked UI or not.
Hi, 

This is not a blocker for certification, but a nice-to-have. 

Thks!!
David
Flags: needinfo?(dpv)
tef- unless our partners disagree
blocking-b2g: tef? → ---
Flags: needinfo?(mvines)
It's pretty bad behaviour and the fix for this bug is simple/low risk for v1.0.1 commercial devices, back out the silly 20 second timer introduced by bug 710962, but I'm not sure if this issue would be blocking for all the target markets.
Flags: needinfo?(mvines)
I think you wanted to mention Bug 832814.


simply backing out that bug is not good as I believe the bug it fixes is more important than this one, and I don't want to bring it back.


before removing the timeout we need a gecko patch. Antonio, do you think you could do that?
Sorry, yeah wrong bug :)

Yes it seems like the reference RIL does need that 20 second hack from bug 832814, but the commercial devices will be fine without it.
ok then I suggest that we remove the timeout in 1.0.1 only, until we fix Mozilla's RIL. does that sound good?
I suggest doing the same on 1.1 as well.
Blocks: 859225
(In reply to Julien Wajsberg [:julienw] from comment #16)
> I think you wanted to mention Bug 832814.
> 
> 
> simply backing out that bug is not good as I believe the bug it fixes is
> more important than this one, and I don't want to bring it back.
> 
> 
> before removing the timeout we need a gecko patch. Antonio, do you think you
> could do that?

Sure, but I have other blockers. I'll work on this asap. IMHO we must fix the platform issue before dealing with other workarounds.
Michael, now that we have a clear path to fixing this bug, can we wait to fix the underlying problem on the Mozilla RIL before removing/lowering the setTimeout ?
(In reply to Anshul from comment #12)
> Jose, yes you need to check for perso substate correctly before reporting
> network locked.
This is already done. It was implemented by Yoshi and the code lives in mozilla-cental. See bug 849757 please. I guess that we could have this issue solved by uplifting bug 849757 to the corresponding release branches. I still have to test it but does it seem a solution?
Flags: needinfo?(anshulj)
See Also: → 849757
If uplifting bug 849857 is what needs to happen to back out bug 832814 than we should do it.  Bug 849857 carries no risk to the product as it only touches the reference RIL implementation, and RIL IDL file changes have no affect on binary compatibility between Gecko and the commercial RIL.
Also bug 832814 can already be backed out on v1.1 as bug 849857 is already available for v1.1.
Flags: needinfo?(anshulj)
s/849857/849757/ in ^^^^ of course.  I'm getting pretty good at mistyping bug #s it seems :(
If we uplift bug 849757 definitely I'd prefer to remove the timeout because it wouldn't be needed. I'll do a test and keep you updated.
leo+, as the solution now seems trivial for v1.1 -- please back out bug 832814.

We may want to consider the same for v1.0.1 as this bug has been reported there as well, and support for this in the v1.0.1 commercial RIL is already present so the risk is minimal.
blocking-b2g: --- → leo+
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26)
> If we uplift bug 849757 definitely I'd prefer to remove the timeout because
> it wouldn't be needed. I'll do a test and keep you updated.

I wonder if the timeout is necessary so that it doesn't try to display too early while the phone is booting.

For v1 I'd prefer to stay on the safe side and simply backout bug 832814. For master we can do more experiments.
Yoshi, please look into this bug.
Assignee: nobody → allstars.chh
It's a Gaia bug.
Assignee: allstars.chh → nobody
I'll do it as I'm the culprit in Bug 832814.
Assignee: nobody → felash
Bug 832814 is landed before Bug 849757.
However I think Bug 849757 did fix the root cause (For Mozilla RIL).
So my opinion is to backout Bug 832814 as well.

Also comment 8 from Ansull says
"when app state is perso locked but perso substate is ready, this should *not* be considered as network locked."
Bug 849757 Part 2 patches addresses this point.
(https://bugzilla.mozilla.org/attachment.cgi?id=724774&action=diff ril_worker.js line 2861)
If this bug ever needs tef+, then bug 849757 will need tef+ too.
Attached patch patch v1 (obsolete) — Splinter Review
After careful testing, I completely removed the setTimeout hack.

However I've no simlocked phone so I'd like that someone that do have a simlocked phone try this patch and tell me if it works in all situations. Especially I'd like to know if the NCK popup is displayed when the phone boots in this situation.

---
 apps/system/js/sim_lock.js |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)


(see also PR https://github.com/mozilla-b2g/gaia/pull/9118)
Attachment #736294 - Flags: review?(josea.olivera)
Flags: needinfo?(anshulj)
Comment on attachment 736294 [details] [diff] [review]
patch v1

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

Thanks for the patch Julien, that's the idea. I've applied and tested the patch and it seems everything is fine. The 'networkLocked' SIM card state is no longer reported during the SIM initialization dance (remember it was in the past). I'd would be nice to have a double-check from Anshul since he is able to test it on a SIM-locked device.

::: apps/system/js/sim_lock.js
@@ +96,1 @@
>          break;

Delete the break statement and return true here please.
Attachment #736294 - Flags: review?(josea.olivera) → review+
Nivi, please give this a test?

This bug also needs to be uplifted to tef as TEF partners have already raised this issue twice. Please see the "Blocks" section of this bug. Commercial RIL already has a equivalent fix for Bug 849757 on v1 so this should be a low risk for v1 too. Nsarkar can test this on both v1 and v1.1 and let you know.
Flags: needinfo?(anshulj) → needinfo?(nsarkar)
Duplicate of this bug: 859225
waiting for Nivi's advice before merging.
Depends on: 849757
asking tef per comment 36
blocking-b2g: leo+ → tef?
Attached patch patch v2Splinter Review
carrying r=jaoo

addressed the review comment.

Also updated the PR in https://github.com/mozilla-b2g/gaia/pull/9118
Attachment #736294 - Attachment is obsolete: true
Attachment #736715 - Flags: review+
blocking-b2g: tef? → tef+
I'd like to sum up guys how we should uplift this work and all its dependencies (both
gecko and gaia ones).

1.- Land bug 822522 patch in mozilla-b2g18_v1_0_1 release branch.
2.- Land bug 836257 patch in gaia v1.0.1 release branch.
3.- Land bug 849757 patches in mozilla-b2g18_v1_0_1 release branch.
4 .-Land this work (once Nivi checks if everything is fine) in gaia {master,
v1-train, v1.0.1} release branches.

Am I right?, please correct me if I am wrong. Thanks!
looks good to me.
Should we then also lift bug 837755 to v1.0.1?
I think this is not an obligation, I think it was not a regression produced by bug 822522.
(but we still can).
Hi guys,

I have verified the fix works on v1 and v1.1.

Thanks,
Nivi.
Flags: needinfo?(nsarkar)
master: 3fa788342953f33ab980fa1ba28deba1a1b8e650
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
v1-train: bc1fd377bf73fa8b6af19a39b20866ffc203b87b
waiting for the uplift of Bug 849757 before uplifting this to v1.0.1.
Bug 849757 was uplifted today. James, would you please uplift this ?
Flags: needinfo?(jlal)
:( Sorry this does not uplift cleanly still.
Flags: needinfo?(jlal) → needinfo?(felash)
v1.0.1: 2d86d82d38355c68cc4506b69d3b44042534e77c
Flags: needinfo?(felash)
Can you please provide steps to verify this fix - as we will blackbox test from the UI?

NCK = Net Code Key
NSCK = Net Subset Code Key
CCK = Corporate Code Key
SPCK = Service Provider Code Key

Not sure if i have access to all these?
On AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.107, unlock screen willn't popup permanently. In ICCIO.cpp, we find card state is unknown. Attached file is log by QXDM. Please help to investigate.
SIM card state is unknown. There is no unlock screen popup.
probably a commercial ril problem ?
Flags: needinfo?(anshulj)
Extremely sorry, this is a regression in commercial RIL. Identified and fixed the issue and it should be available in AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.116.
Flags: needinfo?(anshulj)
On AU121, unlock screen can popup and NW category single lock can be unlocked. But, NS/C/SP/SIM categories unlock screen will not popup. Please refer to log.
AU121 NS/SP/C/SIM lock will not popup
(In reply to buri.blff from comment #58)
> Created attachment 756483 [details]
> AU121 NS/SP/C/SIM lock will not popup
> 
> AU121 NS/SP/C/SIM lock will not popup

Please use standard formats for attachments that way everyone can see the logs. I'm no able to see the log and therefore I cannot help here.
Flags: needinfo?(buri.blff)
Based on comment 57 & 58, the verification result is not OK ,so  I reopen the Bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Per comment #57 ni?-ing Anshul. 
Flags: needinfo?(anshulj)
batch update on tef+ milestones. partner to make a final on 6/3 Asia time. TEF+ needs to be resolved by 6/3 to be in the final build. thanks
Target Milestone: --- → 1.0.1 IOT3 (3jun)
Please do not reopen bugs. File new bugs as followups to the original issue. Reopening will mess up our landing process, so we need new bugs filed for followup bugs.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Flags: needinfo?(anshulj)
Can you please provide steps to verify this fix - as we can perform black box testing from the UI?
You need a phone in a network locked state, when it starts, the NCK popup should appear as soon as possible.
We do not have access to a locked phone to be able to check this issue.
Whiteboard: QARegressExclude
Flags: needinfo?(buri.blff)
You need to log in before you can comment on or make changes to this bug.