1119152 - [SecureElement] Implement SEReader.isSEPresent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

In order to support 'SEReader' isSEPresent attribute a new idl with interfaces namely 'onsepresent()' & 'onseremoved' should be implemented by DOM. These callbacks will be called appropriately by parent process

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Attachment: WIP SEReader.isSEPresent implementation

Hello Yoshi, could I have your feedback on this patch?

Comment 2

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

Comment on attachment 8586186 [details] [diff] [review]
WIP SEReader.isSEPresent implementation

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

::: dom/secureelement/DOMSecureElement.js
@@ +131,5 @@
>      return PromiseHelpers.createSEPromise((resolverId) => {
>        let chromeObj = new SESessionImpl();
>        chromeObj.initialize(this._window, this);
>        let contentObj = this._window.SESession._create(this._window, chromeObj);
> +      this._sessions.push(chromeObj);

See my comments below.
Try to use contentObj or chromeObj consistenly.
Or if you prefer use chromeObj,
add a new patch to covert it.

@@ +504,5 @@
>      this._window = null;
>    },
>  
>    getSEReaders: function getSEReaders() {
> +    // ivalidate previous readers on new request

invalidate.

@@ +544,5 @@
>        case "SE:GetSEReadersResolved":
>          let readers = new this._window.Array();
> +        Object.keys(result.readers).forEach(type => {
> +          let readerImpl = new SEReaderImpl();
> +          readerImpl.initialize(this._window, type, result.readers[type]);

The 3rd parameter 'result.readers[type]' is for isPresent?
It doesn't look like a boolean value, but a reader object.
Use a better naming.

@@ +547,5 @@
> +          let readerImpl = new SEReaderImpl();
> +          readerImpl.initialize(this._window, type, result.readers[type]);
> +          this._readers.push(readerImpl);
> +          readers.push(this._window.SEReader._create(this._window, readerImpl));
> +        });

So there are two arrays for readers here
one for Chrome Obj, and the other for content side.
Why don't you use just one? and add delegation to it.

readerImpl.contentObj = this._window.SEReader._create(...);
this._readers.push(readerImpl);

@@ +585,5 @@
>        case "SE:TransmitAPDURejected":
>          let error = result.error || SE.ERROR_GENERIC;
>          resolver.reject(error);
>          break;
> +      case "SE:ReaderStateChange":

ReaderState is confusing too

::: dom/secureelement/gonk/nsISecureElementConnector.idl
@@ +48,5 @@
>  
> +[scriptable, uuid(b7c79694-8116-42f0-8d62-b086f68de0a8)]
> +interface nsISecureElementStateListener : nsISupports
> +{
> +  void handleSEStateChange(in DOMString type, in boolean isPresent);

notifyXXXChanged.
And I think SEState is a confusing term.

The first argument should be seType.

@@ +107,5 @@
>      */
>     void closeChannel(in long channel,
>                       in nsISEChannelCallback callback);
> +
> +   void addSEStateListener(in nsISecureElementStateListener listener);

ditto, SEState is confusing.

Also what's the use case for multiple listeners?

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Thanks Yoshi for your feedback, I'll address your comments in the next version of the patch. Some additional clarification from my side, would be good if you could also comment on this.

(In reply to Yoshi Huang[:allstars.chh] from comment #2)
> @@ +547,5 @@
> > +          let readerImpl = new SEReaderImpl();
> > +          readerImpl.initialize(this._window, type, result.readers[type]);
> > +          this._readers.push(readerImpl);
> > +          readers.push(this._window.SEReader._create(this._window, readerImpl));
> > +        });
> 
> So there are two arrays for readers here
> one for Chrome Obj, and the other for content side.
> Why don't you use just one? and add delegation to it.
> 
> readerImpl.contentObj = this._window.SEReader._create(...);
> this._readers.push(readerImpl);

I was actually thinking about removing the contentObj variable usage at all and just use chromeObj.__DOM_IMPL__ when it's needed. I can do this in a separate patch. The second |readers| array is used as the argument for Promise.resolve, I wasn't sure if it's a good idea to resolve the promise with |this._readers|.  

> @@ +107,5 @@
> >      */
> >     void closeChannel(in long channel,
> >                       in nsISEChannelCallback callback);
> > +
> > +   void addSEStateListener(in nsISecureElementStateListener listener);
> 
> ditto, SEState is confusing.
> 
> Also what's the use case for multiple listeners?

ACE Access Rules Manager could also listen for SE presence changes and cache/flush access rules accordingly. Are you ok with this approach?

Comment 4

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

(In reply to Krzysztof Mioduszewski[:tauzen] from comment #3)
> Thanks Yoshi for your feedback, I'll address your comments in the next
> version of the patch. Some additional clarification from my side, would be
> good if you could also comment on this.
> 
> (In reply to Yoshi Huang[:allstars.chh] from comment #2)
> > @@ +547,5 @@
> > > +          let readerImpl = new SEReaderImpl();
> > > +          readerImpl.initialize(this._window, type, result.readers[type]);
> > > +          this._readers.push(readerImpl);
> > > +          readers.push(this._window.SEReader._create(this._window, readerImpl));
> > > +        });
> > 
> > So there are two arrays for readers here
> > one for Chrome Obj, and the other for content side.
> > Why don't you use just one? and add delegation to it.
> > 
> > readerImpl.contentObj = this._window.SEReader._create(...);
> > this._readers.push(readerImpl);
> 
> I was actually thinking about removing the contentObj variable usage at all
> and just use chromeObj.__DOM_IMPL__ when it's needed. I can do this in a
> separate patch. The second |readers| array is used as the argument for
> Promise.resolve, I wasn't sure if it's a good idea to resolve the promise
> with |this._readers|.  

fine by me,

> 
> > @@ +107,5 @@
> > >      */
> > >     void closeChannel(in long channel,
> > >                       in nsISEChannelCallback callback);
> > > +
> > > +   void addSEStateListener(in nsISecureElementStateListener listener);
> > 
> > ditto, SEState is confusing.
> > 
> > Also what's the use case for multiple listeners?
> 
> ACE Access Rules Manager could also listen for SE presence changes and
> cache/flush access rules accordingly. Are you ok with this approach?

You could do that, but that does not seem to be a priority to me.
When the SIM is pull out and put back again, currently the modem needs to reboot to detect the new SIM, I think RIL only knows the SIM is removed but cannot know the SIM is put back again, in that case, SE API should throw if content tried to access the SE, so it seems listening the SE presence changed in ACE doesn't bring too much value.
It will help ASSD, but not for now.

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

Attachment: WIP Bug 1119152 Part 1: Consolidate variable naming - chromeObj, contentObj removal

As proposed in comment 3, I'm removing contentObj variables and renaming chromeObj.

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

Attachment: WIP Bug 1119152 - Part 2: SEReader.isSEPresent implementation

Addressed previous feedback round comments. Yoshi could I have your feedback on this? If you'll be ok with this patch, I'll prepare new versions of patches (split parent process and DOM impl) and ask for review.

Additional answers to issues raised previously:
1. RIL actually knows when the SIM is put back in. It takes some time to be notified about this (~10 sec) but it works fine.

2. About the usage of two arrays:
(In reply to Yoshi Huang[:allstars.chh] from comment #2)
> @@ +547,5 @@
> > +          let readerImpl = new SEReaderImpl();
> > +          readerImpl.initialize(this._window, type, result.readers[type]);
> > +          this._readers.push(readerImpl);
> > +          readers.push(this._window.SEReader._create(this._window, readerImpl));
> > +        });
> 
> So there are two arrays for readers here
> one for Chrome Obj, and the other for content side.
> Why don't you use just one? and add delegation to it.
> 
> readerImpl.contentObj = this._window.SEReader._create(...);
> this._readers.push(readerImpl); 

I think we need these two arrays, this._readers is a chrome array storing reader implementations used for updating isSEPresent. The second array is content array (created by new this._window.Array()) which stores reader.__DOM_IMPL__ and is used in resolving the promise. If i try to resolve the promise with this._readers I'm getting "Permission denied to access property "then"" error because it's not a content array.

Comment 7

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

Comment on attachment 8599230 [details] [diff] [review]
WIP Bug 1119152 Part 1: Consolidate variable naming - chromeObj, contentObj removal

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

::: dom/secureelement/DOMSecureElement.js
@@ +526,3 @@
>          if (context) {
>            // Notify context's handler with SEChannel instance
> +          context.onChannelOpen(channelImpl);

Is this line correct?

Or the original code is already wrong ?
The original code returns content object.

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Yoshi Huang[:allstars.chh] from comment #7)
> Comment on attachment 8599230 [details] [diff] [review]
> WIP Bug 1119152 Part 1: Consolidate variable naming - chromeObj, contentObj
> removal
> 
> Review of attachment 8599230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/secureelement/DOMSecureElement.js
> @@ +526,3 @@
> >          if (context) {
> >            // Notify context's handler with SEChannel instance
> > +          context.onChannelOpen(channelImpl);
> 
> Is this line correct?
> 
> Or the original code is already wrong ?
> The original code returns content object.

Yes it's correct. The context here is SessionImpl object, which stores channels to be able to close them if Session.closeAll is called. Channels are closed using close() method which is exposed through WebAPI, so storing content object is actually enough here. But from my perspective, to be consistent with the whole implementation we should store the reference to chromeObj here as we're doing in other cases, and access content object through __DOM_IMPL__ if needed. Additionally in the second patch I need to have access to chromeObj to call SEChannelImpl.invalidate() method which is not exposed as WebAPI.

Comment 9

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

Comment on attachment 8599234 [details] [diff] [review]
WIP Bug 1119152 - Part 2: SEReader.isSEPresent implementation

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

::: dom/secureelement/DOMSecureElement.js
@@ +120,5 @@
> +  },
> +
> +  _checkPresence: function _checkClosed() {
> +    if (!this._isSEPresent) {
> +      throw new Error(SE.ERROR_NOTPRESENT);

Should we also add documentation on the WebIDL?

@@ +169,5 @@
> +  },
> +
> +  invalidate: function invalidate() {
> +    debug("Invalidating SE reader: " + this.type);
> +    this._isSEPresent = false;

is this line needed?
You already set it in line 165.

::: dom/secureelement/gonk/SecureElement.js
@@ +238,5 @@
>      ppmm = null;
>    },
>  
> +  _registerSEPresenceListeners: function() {
> +    this._sePresenceListener = {

also define this in prototype

@@ +240,5 @@
>  
> +  _registerSEPresenceListeners: function() {
> +    this._sePresenceListener = {
> +      notifySEPresenceChanged: (type, isPresent) => {
> +        this._readers[type] = isPresent;

I think I mentioned this before.

this._readers[] looks like an array of readers, but it stores the presence of the SE,
and I think this line should be moved inside _notifyReaderPresenceChanged.

@@ +248,4 @@
>  
> +    SE.SUPPORTED_SE_TYPES.forEach((type) => {
> +      let connector = getConnector(type);
> +      if (connector) {

nit, bail out early.

if (!connector) {
  return;
}

@@ +255,5 @@
> +    });
> +  },
> +
> +  _unregisterSEPresenceListeners: function() {
> +    this._readers.forEach((type) => {

not sure how this works
_readers is a plain JS object defined in line 217, not Array nor Map.

Does this work?

@@ +270,5 @@
> +    // we need to notify all targets, even those without open channels,
> +    // app could've stored the reader without actually using it
> +    debug("notifying DOM about SE state change");
> +    gMap.getTargets().forEach(target => {
> +      var result = { type: type, isPresent: isPresent };

var?

@@ +390,5 @@
>    },
>  
>    _handleGetSEReadersRequest: function(msg, target, callback) {
> +    gMap.registerSecureElementTarget(msg.appId, target);
> +    var readers = Object.keys(this._readers).map(type => {

var?

::: dom/secureelement/gonk/UiccConnector.js
@@ +88,5 @@
>  
>    _init: function() {
>      Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> +
> +    this._iccListener = {

add this to phototype

@@ +130,3 @@
>                        uiccNotReadyStates.indexOf(cardState) == -1;
> +
> +    if (this._isPresent !== uiccPresent) {

bail out early

@@ +341,1 @@
>  

extra line

::: dom/secureelement/gonk/nsISecureElementConnector.idl
@@ +46,5 @@
>    void notifyError(in DOMString error);
>  };
>  
> +[scriptable, uuid(b7c79694-8116-42f0-8d62-b086f68de0a9)]
> +interface nsISecureElementPresenceListener : nsISupports

I'll go shorter name, like nsISEPresenceListener
I don't know if there's other kind of listener,
but if no,
we could just call it 'nsISEListener' and make it a more general event listener rather than a Presence event listener.

::: dom/secureelement/gonk/se_consts.js
@@ +61,5 @@
>  
>  this.TYPE_UICC = "uicc";
>  this.TYPE_ESE = "eSE";
>  
> +this.SUPPORTED_SE_TYPES = [this.TYPE_UICC];

This shouldn't be here.

1. This is not a const value
2. This relates to the implementatin, for example if a device supports eSE and uicc, then it should have those two values.
So this value should be set during runtime.

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

Attachment: WIP (1.1) Bug 1119152 - Part 2: SEReader.isSEPresent implementation

Addressed all the issues pointed out in feedback comment 9 and discussed on IRC.

Comment 11

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

Comment on attachment 8599914 [details] [diff] [review]
WIP (1.1) Bug 1119152 - Part 2: SEReader.isSEPresent implementation

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

::: dom/secureelement/gonk/SecureElement.js
@@ +216,4 @@
>                         Ci.nsIObserver]
>    }),
>  
> +  _readerPresence: {},

Should this be named '_sePresence' ?
As in the original codebase, it is called 'connector'.

And it looks you are implementing SE presence, and not reader presence.

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Bug 1119152 - Part 1: [SecureElement] Consolidate variable naming - chromeObj, contentObj removal

Hello Yoshi, thank you very much for your previous feedback. 

Could you review this patch? If your review will be positive, I'll ask a DOM Peer (:smaug) next.

Comment 13

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

As suggested in feedback comment 11, I've renamed '_readerPresence' to '_sePresence'. Yoshi, could you review this also?

Comment 14

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

Comment on attachment 8602202 [details] [diff] [review]
Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

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

::: dom/secureelement/gonk/SecureElement.js
@@ +193,5 @@
>   * instances (UiccConnector, eSEConnector)' to perform various
>   * SE-related (open,close,transmit) operations.
> + * @TODO: Bug 1118096 (?)
> + * @TODO: Bug 1118097
> + * @TODO: Bug 1118101 (?)

Make better descrition on TODO

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8602201 [details] [diff] [review]
Bug 1119152 - Part 1: [SecureElement] Consolidate variable naming - chromeObj, contentObj removal

Hello Olli, could you review this?

Comment 16

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8602202 [details] [diff] [review]
Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

Olli, could you also review the DOM part of this patch?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8602202 [details] [diff] [review]
Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

> 
>+  _sePresence: {},
>+
This could use a comment what kind of objects the property is keeping alive.




I assume we'll get "sepresent"  and "seremoved"[1] events in some other bug?


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1119152#c0

Comment 18

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

Addressed nits from comment 14 and comment 17. Thank you for the reviews.

Comment 19

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Olli Pettay [:smaug] from comment #17)
> I assume we'll get "sepresent"  and "seremoved"[1] events in some other bug?

I've raised Bug 1163586 for discussing/implementing this.

Comment 20

[Krzysztof Mioduszewski[:tauzen]]

Try build progressing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e67f15e6d80

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/53efa93717cd
https://hg.mozilla.org/integration/b2g-inbound/rev/be989a229115

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/53efa93717cd
https://hg.mozilla.org/mozilla-central/rev/be989a229115

Comment 23

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

Comment on attachment 8604095 [details] [diff] [review]
(v1.1) Bug 1119152 - Part 2: [SecureElement] SEReader.isSEPresent implementation

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

::: dom/secureelement/gonk/UiccConnector.js
@@ +329,5 @@
> +
> +  unregisterListener: function(listener) {
> +    let idx = this._SEListeners.indexOf(listener);
> +    if (idx !== -1) {
> +      this._listeners.splice(idx, 1);

[JavaScript Error: "TypeError: this._listeners is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/UiccConnector.js" line: 331}]

Comment 24

[Krzysztof Mioduszewski[:tauzen]]

Typo, sorry. Thanks for pointing this out. Just opening an new bug to fix this.

Comment 25

[Krzysztof Mioduszewski[:tauzen]]

Bug 1174683