Closed Bug 1119152 Opened 6 years ago Closed 6 years ago

[SecureElement] Implement SEReader.isSEPresent

Categories

(Firefox OS Graveyard :: NFC, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
2.2 S12 (15may)
Tracking Status
firefox41 --- fixed

People

(Reporter: psiddh, Assigned: tauzen)

References

Details

Attachments

(2 files, 5 obsolete files)

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
No longer depends on: 884594, 1118098
Assignee: nobody → kmioduszewski
Depends on: 1149177
Summary: SecureElement : Implement new idl with interfaces to detect secureelement state changes for DOM → [SecureElement] Implement SEReader.isSEPresent
Hello Yoshi, could I have your feedback on this patch?
Attachment #8586186 - Flags: feedback?(allstars.chh)
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?
Attachment #8586186 - Flags: feedback?(allstars.chh) → feedback-
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?
Flags: needinfo?(allstars.chh)
(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.
Flags: needinfo?(allstars.chh)
As proposed in comment 3, I'm removing contentObj variables and renaming chromeObj.
Attachment #8599230 - Flags: feedback?(allstars.chh)
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.
Attachment #8586186 - Attachment is obsolete: true
Attachment #8599234 - Flags: feedback?(allstars.chh)
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.
Attachment #8599230 - Flags: feedback?(allstars.chh)
(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 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.
Attachment #8599234 - Flags: feedback?(allstars.chh) → feedback-
Addressed all the issues pointed out in feedback comment 9 and discussed on IRC.
Attachment #8599234 - Attachment is obsolete: true
Attachment #8599914 - Flags: feedback?(allstars.chh)
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.
Attachment #8599914 - Flags: feedback?(allstars.chh) → feedback+
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.
Attachment #8599230 - Attachment is obsolete: true
Attachment #8602201 - Flags: review?(allstars.chh)
As suggested in feedback comment 11, I've renamed '_readerPresence' to '_sePresence'. Yoshi, could you review this also?
Attachment #8599914 - Attachment is obsolete: true
Attachment #8602202 - Flags: review?(allstars.chh)
Attachment #8602201 - Flags: review?(allstars.chh) → review+
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
Attachment #8602202 - Flags: review?(allstars.chh) → review+
Comment on attachment 8602201 [details] [diff] [review]
Bug 1119152 - Part 1: [SecureElement] Consolidate variable naming - chromeObj, contentObj removal

Hello Olli, could you review this?
Attachment #8602201 - Flags: review?(bugs)
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?
Attachment #8602202 - Flags: review?(bugs)
Attachment #8602201 - Flags: review?(bugs) → review+
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
Attachment #8602202 - Flags: review?(bugs) → review+
Addressed nits from comment 14 and comment 17. Thank you for the reviews.
Attachment #8602202 - Attachment is obsolete: true
Attachment #8604095 - Flags: review+
(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.
Keywords: checkin-needed
Target Milestone: --- → 2.2 S12 (15may)
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}]
Flags: needinfo?(kmioduszewski)
Typo, sorry. Thanks for pointing this out. Just opening an new bug to fix this.
Bug 1174683
Depends on: 1174683
Flags: needinfo?(kmioduszewski)
You need to log in before you can comment on or make changes to this bug.