930394 - B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[shawn ku [:sku]]

Per current design, when there is no more PUK retry, ril_worker.js will map cardState to pukRequired due to "app_state":3. This is in-correct. We should refer to universalPINState/pin1_replaced/pin1 to determine if permanent blocked.


10-24 02:50:01.679   739   756 I Gecko   : RIL Worker[0]: iccStatus: {"cardState":1,"universalPINState":0,"gsmUmtsSubscriptionAppIndex":0,"cdmaSubscriptionAppIndex":-1,"imsSubscriptionAppIndex":-1,"apps":[{"app_type":2,"app_state":3,"perso_substate":0,"aid":"a0000000871002f886ff9289050b00ff","app_label":"434854","pin1_replaced":0,"pin1":5,"pin2":1}]}

Comment 1

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 1: RIL patch. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 2

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 2: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 3

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 3: Add test cases. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 4

[shawn ku [:sku]]

Add related bug link - 
Bug 921815 - [B2G][Helix][SIM][wangchao]No limit for the times of enter sim puk when no puk time left.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 821501 [details] [diff] [review]
Bug 930394 - Part 2: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

I am okay with this patch.
However IDL change should be in Part 1.
Please reorder you patches.

Comment 6

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 7

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 2: RIL patch. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 8

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 3: Add test cases. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Comment 9

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #5)
> Comment on attachment 821501 [details] [diff] [review]
> Bug 930394 - Part 2: add comment in IDL. B2G RIL: Correct cardState to
> permanentBlocked when no more PUK retry.
> 
> Review of attachment 821501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am okay with this patch.
> However IDL change should be in Part 1.
> Please reorder you patches.

Thanks Yoshi for reminding me this part.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 823119 [details] [diff] [review]
Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

I am okay with this patch.
However I'd like to request some comments from Hsinyi.

Currently cardState in IccManager is actually combining states from AppState, PersoState.
Now this patch will add PinState here.

Not sure if we need to file another bug to discuss this.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 823119 [details] [diff] [review]
Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

Press enter too fast,
see Comment 10.

Comment 12

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> Comment on attachment 823119 [details] [diff] [review]
> Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to
> permanentBlocked when no more PUK retry.
> 
> Review of attachment 823119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am okay with this patch.
> However I'd like to request some comments from Hsinyi.
> 
> Currently cardState in IccManager is actually combining states from
> AppState, PersoState.
> Now this patch will add PinState here.
> 
> Not sure if we need to file another bug to discuss this.

Hi Yoshi,
 
May I know more about your concern here? Have you foreseen any drawbacks if we combine states from AppState, PersoState, PinState into 'cardState' attribute? 

What if we don't combine them? So in API we will have 'cardState' 'AppState' 'persoState' and 'pinState.' Then how would gaia use these different states? Maybe you could shed some light here? Thanks.

Comment 13

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

Comment on attachment 823119 [details] [diff] [review]
Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

Press too quickly... I need some answers to the previous comment. :)

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 823120 [details] [diff] [review]
Bug 930394 - Part 2: RIL patch. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

::: dom/system/gonk/ril_worker.js
@@ +3007,5 @@
> +      if (app.pin1 === CARD_PINSTATE_ENABLED_PERM_BLOCKED) {
> +        newCardState = GECKO_CARDSTATE_PERMANENT_BLOCKED;
> +      }
> +    }
> +

let pinState = app.pin1_replaced ?
               iccStatus.universalPINState :
               app.pin1;
if (pinState === CARD_PINSTATE_ENABLED_PERM_BLOCKED) {
  ...
}

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 823121 [details] [diff] [review]
Bug 930394 - Part 3: Add test cases. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

Add r=me

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #12)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> > Comment on attachment 823119 [details] [diff] [review]
> > Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to
> > permanentBlocked when no more PUK retry.
> > 
> > Review of attachment 823119 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I am okay with this patch.
> > However I'd like to request some comments from Hsinyi.
> > 
> > Currently cardState in IccManager is actually combining states from
> > AppState, PersoState.
> > Now this patch will add PinState here.
> > 
> > Not sure if we need to file another bug to discuss this.
> 
> Hi Yoshi,
>  
> May I know more about your concern here? Have you foreseen any drawbacks if
> we combine states from AppState, PersoState, PinState into 'cardState'
> attribute? 
> 
> What if we don't combine them? So in API we will have 'cardState' 'AppState'
> 'persoState' and 'pinState.' Then how would gaia use these different states?
> Maybe you could shed some light here? Thanks.

I don't have any concern, I already gave r+ on this.
Just let you know the cardState will include PinState now.

Comment 17

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #16)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #12)
> > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> > > Comment on attachment 823119 [details] [diff] [review]
> > > Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to
> > > permanentBlocked when no more PUK retry.
> > > 
> > > Review of attachment 823119 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I am okay with this patch.
> > > However I'd like to request some comments from Hsinyi.
> > > 
> > > Currently cardState in IccManager is actually combining states from
> > > AppState, PersoState.
> > > Now this patch will add PinState here.
> > > 
> > > Not sure if we need to file another bug to discuss this.
> > 
> > Hi Yoshi,
> >  
> > May I know more about your concern here? Have you foreseen any drawbacks if
> > we combine states from AppState, PersoState, PinState into 'cardState'
> > attribute? 
> > 
> > What if we don't combine them? So in API we will have 'cardState' 'AppState'
> > 'persoState' and 'pinState.' Then how would gaia use these different states?
> > Maybe you could shed some light here? Thanks.
> 
> I don't have any concern, I already gave r+ on this.
> Just let you know the cardState will include PinState now.

Thanks, Yoshi. I think having combining state is easy to use if every state itself indeed presents clearly.

Comment 18

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

Comment on attachment 823119 [details] [diff] [review]
Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry.

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

r+ per comment 16.

Comment 19

[shawn ku [:sku]]

Attachment: [Final] Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi, r=hsinyi.

Comment 20

[shawn ku [:sku]]

Attachment: [Final] Bug 930394 - Part 2: RIL patch. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi.

Comment 21

[shawn ku [:sku]]

Attachment: [Final] Bug 930394 - Part 3: Add test cases. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi.

Comment 22

[shawn ku [:sku]]

try server -
https://tbpl.mozilla.org/?tree=Try&rev=ca32dab758f7

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Shawn
Please remove the '[Final]' words from your patches.

Comment 24

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #23)
> Hi, Shawn
> Please remove the '[Final]' words from your patches.

Thanks, Yoshi, Do it right now.

Comment 25

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 1: add comment in IDL. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi, r=hsinyi.

Comment 26

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 2: RIL patch. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi.

Comment 27

[shawn ku [:sku]]

Attachment: Bug 930394 - Part 3: Add test cases. B2G RIL: Correct cardState to permanentBlocked when no more PUK retry. r=yoshi.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e42cb0c56ca2
https://hg.mozilla.org/integration/b2g-inbound/rev/cbca363f09ee
https://hg.mozilla.org/integration/b2g-inbound/rev/844f6bf9bc17

Also CCing Anshul for this, we will add a cardState 'permanentBlocked' here.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e42cb0c56ca2
https://hg.mozilla.org/mozilla-central/rev/cbca363f09ee
https://hg.mozilla.org/mozilla-central/rev/844f6bf9bc17