875710 - B2G RIL: Get PIN/PUK remaining retry count

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar]]

This is a follow-up bug for bug 860660.

In bug 860660, we add a attribute, retryCount, for remaining retry count of PIN1/PUK1 [1], it seems API user needs to check the cardState then decide the retryCount is represented for PIN1 or PUK1.  And it may have some synchronization problems [2]. Maybe we could have a better way to get retryCount, something like, getCardLockRetryCount(DOMString lockType), and this also align with current card lock API.

Thanks

[1] Please see bug 860660 comment #57.
[2] Please see bug 860660 comment #64.

Comment 1

[Edgar Chen [:edgar]]

Oh, and also we can move this function to mozIccManager in this bug.
Please see bug 874744 comment #5.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Edgar,

I'm looking for a not-too-hard bug to get more familiar with the RIL code. This one seems to be a good candidate. Can I have it?

Comment 3

[Edgar Chen [:edgar]]

Sure, just take it. :)

Comment 4

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

Bug 868896 is the emulator support for getting retry count.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 875710: Remove retryCount from nsIDOMMobileConnection

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The changes to Gaia are available at

  https://github.com/tdz/gaia/commit/1eda0430db2eeeb9f82c0876040106f68f3221e0

I talked to ferjm and I'd say this should not land before bug 883178 and bug 874000.

Next is the actual implementation for getCardLockRetryCount, as outlined on bug 868896.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 765355 [details] [diff] [review]
[02] Bug 875710: Remove retryCount from nsIDOMMobileConnection

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ -60,5 @@
> -   * or 'pukRequired'.  0 denotes the retry count is unavailable.
> -   *
> -   * Value is undefined for other cardState values.
> -   */
> -  readonly attribute long retryCount;

When interface has changes, you need assign a new UUID to it.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ -44,5 @@
>    void registerMobileConnectionMsg(in nsIMobileConnectionListener listener);
>    void unregisterMobileConnectionMsg(in nsIMobileConnectionListener listener);
>  
>    readonly attribute DOMString cardState;
> -  readonly attribute long retryCount;

Ditto

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Edgar,

Thanks for having a look.

> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ -60,5 @@
> > -   * or 'pukRequired'.  0 denotes the retry count is unavailable.
> > -   *
> > -   * Value is undefined for other cardState values.
> > -   */
> > -  readonly attribute long retryCount;
> 
> When interface has changes, you need assign a new UUID to it.

Oh, I should have mentioned that it's kind of intentional, because I had some merge conflicts here and the patch is not ready anyway. :(

> ::: dom/network/interfaces/nsIMobileConnectionProvider.idl
> @@ -44,5 @@
> >    void registerMobileConnectionMsg(in nsIMobileConnectionListener listener);
> >    void unregisterMobileConnectionMsg(in nsIMobileConnectionListener listener);
> >  
> >    readonly attribute DOMString cardState;
> > -  readonly attribute long retryCount;
> 
> Ditto

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 765354 [details] [diff] [review]
[01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider

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

::: dom/system/gonk/RILContentHelper.js
@@ +600,5 @@
> +    let request = Services.DOMRequest.createRequest(window);
> +    request.success = true;
> +    request.retryCount = this.retryCount;
> +    return request;
> +  },

Actually you should not assign any value to DOMRequest. This should be an asynchronous proccess. You can use |fireSuccess()| to trigger a callback after process is finished success. Or use |fireError()| when any error occurred.

And I did not see you handle lockType here. I guess this patch is not a formal one.  :)

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> And I did not see you handle lockType here. I guess this patch is not a
> formal one.  :)

I just wanted some feedback. :) For now, this only returns the same value as retryCount. I also want to implement bug 868896 for getting some actual values from the SIM card. It'll probably make sense to land these changes at the same time.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider

The Gaia changes are in bug 886238, the removal of the old interface in bug 886239.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 766572 [details] [diff] [review]
[01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider

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

That's nice, but please have a Marionette or Mochitest test case for it.  Besides, Andrew Overholt sent us a notice recently that we should do some thing more before exposing any new Web APIs.  A mail to dev-platform and dev-webapi specifically.  I'll forward that mail to you in private.  The new API needs DOM peer's super review, and I suggest you can request for their SR after that public description to the intention of the changes here.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vicamo,

Thanks for all the information.

I basically went with what was used in mozMobileConnection, but since we have to get the API right, I'd like to propose a change in semantics. This feature is optional and we currently don't actually support it. So I'd like to change the call to return an error message if the lockType is known, but the retry count cannot be read. The error message could be something like 'Unknown Retry Count". Are there similar cases of optional features in the RIL interface? If yes, how is this problem handled there?

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider (v2)

Hi Vicamo,

I know you already r+'ed this patch, but I wanted to change the interface before asking on dev-webapi.

The changes are just in ril_worker.js. It now generates an error of type 'NotSupportedError' for lock types which are supported by the API, but not available in the implementation. This feature is optional in the specs and seems not available in rild. I think the updated interface is a good way for dealing with this problem.

I also added more lock types.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 875710: Added marionette tests for getCardLockRetryCount

The marionette tests check the 'pin' and 'puk' lock types, and test an invalid lock type for the correct error.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

API documentation
=============

This patch set adds the following interfaces.

-  nsIDOMRequest nsIIccProvider.getCardLockRetryCount(lockType)

This function returns the number of remaining tries when unlocking a SIM card. It receives one parameter 'lockType' of type DOMString. It returns a DOM request of type nsIDOMRequest.

The parameter 'lockType' sets the type of the requested lock. Valid strings are

--  pin
--  puk
--  pin2
--  puk2
--  nck
--  cck
--  spck

These strings represent different locks that are available on a SIM card. More strings can be added if necessary.

On success the DOM request's result property contains the following fields:

--  DOMString lockType: the lock type that was supplied to getCardLockRetryCount
-- int retryCount:  the number of remaining tries for the lock; must be greater-or-equal to 0

On error the DOM request's error property contains one of the following error names.

--  'IllegalAccessError' if the supplied lock-type parameter is an invalid string
--  'NotSupportedError' if the supplied lock-type parameter is valid, but not supported by the implementation.

The second error type is necessary because reading the retry count for a lock is an optional feature, and might not be supported on all lock types, platforms, or SIM cards.

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Please replace 'nsIIccProvider' by 'nsIDOMMozIccManager' in the comment above.

Comment 19

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

Comment on attachment 767235 [details] [diff] [review]
[01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +1206,5 @@
> +      case "pin2":
> +      case "puk2":
> +      case "nck":
> +      case "cck":
> +      case "spck":

Can you show us the specification for retry count from 'nck', 'cck' and 'spck' ?

I checked TS 22.022 but didn't find any information for retry count.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Yoshi,

Sure. The specification I used is 3GPP TS 27.007, version 11.6.0 [1]. Section 8.65 standardizes AT+CPINR, the command for reading the counters. It says 'all values of AT+CPIN, except READY'. Section 8.3 of [1] standardizes AT+CPIN, which lists all lock types that are in the patch plus some more. I only added support for those locks that are used in our code.

[1] http://www.etsi.org/deliver/etsi_ts/127000_127099/127007/11.06.00_60/ts_127007v110600p.pdf

Comment 21

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

Okay, understood now. 
Thanks

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> Hi Vicamo,
> 
> Thanks for all the information.
> 
> I basically went with what was used in mozMobileConnection, but since we
> have to get the API right, I'd like to propose a change in semantics. This
> feature is optional and we currently don't actually support it. So I'd like
> to change the call to return an error message if the lockType is known, but
> the retry count cannot be read. The error message could be something like
> 'Unknown Retry Count". Are there similar cases of optional features in the
> RIL interface? If yes, how is this problem handled there?

First, I won't call it an optional feature because bug 886238 depends on this bug and it will become the correct way to retrieve lock retry counts.

Second, have generic error name is an even better solution here.  Recently we have similar changes in bug 883178, MMIError.  However that case has extra data to deliver to content but this one doesn't.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 767238 [details] [diff] [review]
[02] Bug 875710: Added marionette tests for getCardLockRetryCount

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

Test case rocks!

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vicamo
 
> First, I won't call it an optional feature because bug 886238 depends on
> this bug and it will become the correct way to retrieve lock retry counts.

Sure. What I meant was that the AT+CPINR command is optional in the spec, and our API has to tell the user when a retry count couldn't be retrieved.

> Second, have generic error name is an even better solution here.  Recently
> we have similar changes in bug 883178, MMIError.  However that case has
> extra data to deliver to content but this one doesn't.

Sorry for asking a dump question. :) By 'generic error name' you mean the GECKO_ERROR_* constants that are defined in ril_consts.js? If yes, I'd simply update the patches to use GECKO_ERROR_REQUEST_NOT_SUPPORTED and GECKO_ERROR_INVALID_PARAMETER. Do you want to review this change again?

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> Sorry for asking a dump question. :) By 'generic error name' you mean the
> GECKO_ERROR_* constants that are defined in ril_consts.js? If yes, I'd
> simply update the patches to use GECKO_ERROR_REQUEST_NOT_SUPPORTED and
> GECKO_ERROR_INVALID_PARAMETER. Do you want to review this change again?

I thought we may replace NotSupportedError in ril_worker with GECKO_ERROR_REQUEST_NOT_SUPPORTED, and InvalidAccessError with GECKO_ERROR_GENERIC_FAILURE.  Some day we might want to map all these RIL specific errors to standard errors like 'NotSupportedError' and 'SyntaxError'(?).

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider (v3)

Nothing new here. I just replaced the error codes.

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 875710: Added marionette tests for getCardLockRetryCount (v2)

Updated for the new error strings.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 768302 [details] [diff] [review]
[01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider (v3)

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

Thank you :)

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Sorry for the delay here and thanks for your reviews.

https://hg.mozilla.org/projects/birch/rev/d6d4f4cbcf6f
https://hg.mozilla.org/projects/birch/rev/0f2a87b9c705

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6d4f4cbcf6f
https://hg.mozilla.org/mozilla-central/rev/0f2a87b9c705

Comment 31

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Edgar, Thomas, our partner has one question while using this getCardLockRetryCount. They found that the API won't work until user has input an invalid SIM PIN. However, partner wants to display the total available SIM PIN input times when entering the SIM PIN unlock screen. Is there anyway partner can achieve that?

So what partner wants is :

enable SIM-PIM -> restart device -> device asks for SIM PIN, and in that screen, the device shows try count is 3

Comment 32

[Edgar Chen [:edgar]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #31)
> Hi Edgar, Thomas, our partner has one question while using this
> getCardLockRetryCount. They found that the API won't work until user has
> input an invalid SIM PIN. However, partner wants to display the total
> available SIM PIN input times when entering the SIM PIN unlock screen. Is
> there anyway partner can achieve that?
> 
> So what partner wants is :
> 
> enable SIM-PIM -> restart device -> device asks for SIM PIN, and in that
> screen, the device shows try count is 3

Hi Vance,

I believe the API won't work is caused by the lack of underlaying implementation. 

In APSO, there is no standard ril parcel for getting retry count in ril.h, so partner needs to have their own implementation for it.

In reference design (b2g emulator), we add a proprietary parcel, RIL_REQUEST_GET_UNLOCK_RETRY_COUNT, and use AT command +CPINR to get retry count from modem [1].

Thank you.

[1] Please see bug 868896.