Closed Bug 875710 Opened 7 years ago Closed 7 years ago

B2G RIL: Get PIN/PUK remaining retry count

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: edgar, Assigned: tzimmermann)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 files, 5 obsolete files)

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.
Oh, and also we can move this function to mozIccManager in this bug.
Please see bug 874744 comment #5.
Blocks: 857414
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?
Flags: needinfo?(echen)
Sure, just take it. :)
Flags: needinfo?(echen)
Bug 868896 is the emulator support for getting retry count.
Depends on: 868896
Assignee: nobody → tzimmermann
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 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
Attachment #765355 - Flags: feedback?(echen)
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 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.  :)
Attachment #765354 - Flags: feedback?(echen)
> 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.
The Gaia changes are in bug 886238, the removal of the old interface in bug 886239.
Attachment #765354 - Attachment is obsolete: true
Attachment #765355 - Attachment is obsolete: true
Attachment #766572 - Flags: review?(echen)
Attachment #766572 - Flags: review?(echen) → review?(vyang)
Blocks: 868896
No longer depends on: 868896
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.
Attachment #766572 - Flags: review?(vyang) → review+
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?
Flags: needinfo?(vyang)
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.
Attachment #766572 - Attachment is obsolete: true
Attachment #767235 - Flags: review?(vyang)
The marionette tests check the 'pin' and 'puk' lock types, and test an invalid lock type for the correct error.
Attachment #767238 - Flags: review?(vyang)
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.
Please replace 'nsIIccProvider' by 'nsIDOMMozIccManager' in the comment above.
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.
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
Okay, understood now. 
Thanks
(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.
Flags: needinfo?(vyang)
Attachment #767235 - Flags: review?(vyang) → review+
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!
Attachment #767238 - Flags: review?(vyang) → review+
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?
Flags: needinfo?(vyang)
Attachment #767235 - Flags: superreview?(mounir)
(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'(?).
Flags: needinfo?(vyang)
Keywords: testcase-wanted
Nothing new here. I just replaced the error codes.
Attachment #767235 - Attachment is obsolete: true
Attachment #767235 - Flags: superreview?(mounir)
Attachment #768302 - Flags: review?(vyang)
Attachment #768302 - Flags: superreview?(mounir)
Updated for the new error strings.
Attachment #767238 - Attachment is obsolete: true
Attachment #768304 - Flags: review?(vyang)
Attachment #767235 - Attachment is obsolete: false
Attachment #767235 - Attachment is obsolete: true
Attachment #768302 - Flags: superreview?(mounir) → superreview+
Comment on attachment 768302 [details] [diff] [review]
[01] Bug 875710: Added getCardLockRetryCount to nsIIccProvider (v3)

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

Thank you :)
Attachment #768302 - Flags: review?(vyang) → review+
Attachment #768304 - Flags: review?(vyang) → review+
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
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/d6d4f4cbcf6f
https://hg.mozilla.org/mozilla-central/rev/0f2a87b9c705
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Flags: needinfo?(tzimmermann)
Flags: needinfo?(echen)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] 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.
Flags: needinfo?(tzimmermann)
Flags: needinfo?(echen)
You need to log in before you can comment on or make changes to this bug.