1118102 - SecureElement : Proper errors should be propagated to apps using SE API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

Currently most of the errors in parent process are propagated north bound as 'Generic Errors'. This bug may track error handling / propagation across parent and child process  in the SE stack.

Comment 1

[psiddh]

Attachment: (v1) Fix error handling by rejecting the promises with specific SE errors.

Comment 2

[psiddh]

Attachment: 'SE-API-Test-App' corresponding Gaia dev app changes

Here is the patch for 'SE API Test App'. With the Gecko + (this) Gaia patch, the test app passes all the test cases (except one which is related to ACE).

If earlier Gecko patch is Ok'd then I'd send this patch / pull request to Alison for her further review!

FYI:There is another issue to be fixed in SE stack (as a result intermittently shows up in 'SE API Test App'). It looks like there is a race condition that needs to be fixed. (More on this bug later)

Comment 3

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

Comment on attachment 8589446 [details] [diff] [review]
(v1) Fix error handling by rejecting the promises with specific SE errors.

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

::: dom/secureelement/DOMSecureElement.js
@@ +152,5 @@
>          this._sessions = [];
>          resolver.resolve();
>        }, (reason) => {
> +        let error = new SEError(SE.ERROR_BADSTATE,
> +          " Unable to close all channels associated with this reader");

extra space

@@ +206,5 @@
>    },
>  
>    openLogicalChannel: function openLogicalChannel(aid) {
> +    if (this._isClosed) {
> +        return PromiseHelpers.rejectWithSEError(SE.ERROR_BADSTATE,

indent

@@ +217,1 @@
>               " Invalid AID length - " + aidLen);

extra space.

@@ +236,5 @@
>    },
>  
>    closeAll: function closeAll() {
> +    if (this._isClosed) {
> +        return PromiseHelpers.rejectWithSEError(SE.ERROR_BADSTATE,

indent

@@ +346,1 @@
>                 ", MANAGE CHANNEL command not permitted");

What does ', ' mean ?
forgot to remove this?
same for below

Comment 4

[psiddh]

Thanks Yoshi for the comments. I was caught with few other things. Will follow up on this now.

Comment 5

[psiddh]

Attachment: (v2) Fix error handling by rejecting the promises with specific SE errors.

Hi Yoshi,

I have taken care of your comments. Since it has been more than a month (and ACE code has landed as well), requesting a review one more time just in case.

Based on my recent testing, I observe that couple of test cases fail. These test cases are merely test-case failures in the app at this point and not functional. I will raise another bug (after analyzing it further) where we can discuss on how to address these test-case failures.

Comment 6

[psiddh]

Attachment: (v2) Fix error handling by rejecting the promises with specific SE errors.

Comment 7

[psiddh]

Here are try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8b3e8f6987

Comment 8

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

Comment on attachment 8611592 [details] [diff] [review]
(v2) Fix error handling by rejecting the promises with specific SE errors.

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

::: dom/secureelement/DOMSecureElement.js
@@ +34,5 @@
>  
> +// Extend / Inherit from Error object
> +function SEError(name, message) {
> +  this.name = name || SE.ERROR_GENERIC;
> +  this.message = message || '';

""

@@ +353,5 @@
>      // TODO remove this once it will be possible to have a non-optional dict
>      // in the WebIDL
>      if (!command) {
> +      return PromiseHelpers.rejectWithSEError(SE.ERROR_ILLEGALPARAMETER,
> +             " SECommand dict must be defined");

remove extra space in the beginning, "SECommand.."

::: dom/webidl/SecureElement.webidl
@@ +14,5 @@
>    "SEIoError",                  // I/O Error while communicating with the secure element.
>    "SEBadStateError",            // Error occuring as a result of bad state.
>    "SEInvalidChannelError",      // Opening a channel failed because no channel is available.
>    "SEInvalidApplicationError",  // The requested application was not found on the secure element.
> +  "SEIllegalParameterError",    // Request operation does not have valid parameters.

Where have SENotPresentError gone?

Comment 9

[psiddh]

(In reply to Yoshi Huang[:allstars.chh] from comment #8)
> Comment on attachment 8611592 [details] [diff] [review]
> (v2) Fix error handling by rejecting the promises with specific SE errors.
> 
> Review of attachment 8611592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/secureelement/DOMSecureElement.js
> @@ +34,5 @@
> >  
> > +// Extend / Inherit from Error object
> > +function SEError(name, message) {
> > +  this.name = name || SE.ERROR_GENERIC;
> > +  this.message = message || '';
> 
> ""
> 
> @@ +353,5 @@
> >      // TODO remove this once it will be possible to have a non-optional dict
> >      // in the WebIDL
> >      if (!command) {
> > +      return PromiseHelpers.rejectWithSEError(SE.ERROR_ILLEGALPARAMETER,
> > +             " SECommand dict must be defined");
> 
> remove extra space in the beginning, "SECommand.."
> 
> ::: dom/webidl/SecureElement.webidl
> @@ +14,5 @@
> >    "SEIoError",                  // I/O Error while communicating with the secure element.
> >    "SEBadStateError",            // Error occuring as a result of bad state.
> >    "SEInvalidChannelError",      // Opening a channel failed because no channel is available.
> >    "SEInvalidApplicationError",  // The requested application was not found on the secure element.
> > +  "SEIllegalParameterError",    // Request operation does not have valid parameters.
> 
> Where have SENotPresentError gone?

My mistake. I have added this constant back in the next version

Comment 10

[psiddh]

Attachment: (v3) Fix error handling by rejecting the promises with specific SE errors.

Addressed earlier comments

Comment 11

[psiddh]

Here are the latest try results : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1dacebc345e

Comment 12

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

Comment on attachment 8614979 [details] [diff] [review]
(v3) Fix error handling by rejecting the promises with specific SE errors.

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

::: dom/secureelement/gonk/se_consts.js
@@ +60,3 @@
>  this.ERROR_NOTPRESENT         = "SENotPresentError";
> +this.ERROR_ILLEGALPARAMETER   = "SEIllegalParameterError";
> +this.ERROR_GENERIC            = "SEGenericError";

nit, this.ERROR_GENERIC should be put in the original location, instead of moving it to the end.

Comment 13

[psiddh]

Attachment: (v3.1) Fix error handling by rejecting the promises with specific SE errors. r=allstars.chh r=khuey

Comment 14

[psiddh]

Try Results for the latest patch : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8df878d4f70

Comment 15

[Ryan VanderMeulen [:RyanVM]]

webidl changes need DOM peer review

Comment 16

[psiddh]

Comment on attachment 8615727 [details] [diff] [review]
(v3.1) Fix error handling by rejecting the promises with specific SE errors. r=allstars.chh r=khuey

Requesting Kyle Huey to review the relevant bits of the patch.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a40b2c6672cd

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/a40b2c6672cd

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a40b2c6672cd