976402 - [Parent Process - Nfc.js] Do not even add the target if the session IDs do not match

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

It is better to explicitly check for sessionToken passed by content at the time of creation of MozTag or MozPeer objects before registering the target (content process)

Comment 1

[psiddh]

Attachment: (v1) Perform sanity check on 'Session Token' while creating MozNfcTag and MozNfcPeer objects

Comment 2

[psiddh]

Attachment: 0001-Bug-959109-Do-not-add-content-target-when-sessions-d.patch

Comment 3

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

Hi Sidd, can you also write a test case?

Comment 4

[psiddh]

Sure Yoshi, I will. Do you suggest a test case @ gecko/dom/nfc/tests ?

Comment 5

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

Yes, gecko/dom/nfc/tests/marionette
see sample from Bug 935525.

Comment 6

[psiddh]

Hi Yoshi, When I am running the test cases, I get the following messages

START LOG:
INFO TEST-START: /home/siddartha/B2G_Jan14/FirefoxOS/gecko/dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js Tue Mar 04 2014 21:06:06 GMT-0500 (EST)
INFO Skipping test on system without NFC Tue Mar 04 2014 21:06:06 GMT-0500 (EST)
INFO TEST-END: /home/siddartha/B2G_Jan14/FirefoxOS/gecko/dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js Tue Mar 04 2014 21:06:06 GMT-0500 (EST)
END LOG:

Is there some issue with my setup? 

On the test case part, Was your idea to add similar nfc tests on the lines of 'gecko/dom/system/gonk/tests' that construct a 'fake nfc message (say sessionToken = 'ABCD') other params and bubble the message through the Nfc stack ? And then upon subsequent creation of Nfc TAG obj using say a different sessionToken , say mozNfc.getNFCTag('123'), the test case should fail as sessionTokens are different.

If not, could you please suggest me how I can write a test case that can validate a session token?

Comment 7

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

(In reply to Siddartha P from comment #6)
> Hi Yoshi, When I am running the test cases, I get the following messages
> 
> START LOG:
> INFO TEST-START:
> /home/siddartha/B2G_Jan14/FirefoxOS/gecko/dom/nfc/tests/marionette/
> test_nfc_manager_tech_discovered.js Tue Mar 04 2014 21:06:06 GMT-0500 (EST)
> INFO Skipping test on system without NFC Tue Mar 04 2014 21:06:06 GMT-0500
> (EST)

Are you using a JellyBean emulator?
Only JellyBean emulator is with NFC enabled, IceCream doesn't have this.

If you're already using a JellyBean, try to launch the emulator, then
adb shell getprop| grep ro.moz.nfc.enabled

This property should be true.

This property comes from device/lge/mako/device.mk
If your device.mk doesn't have this property defined, 
try to do 'repo sync' and remove 'out' folder and rebuild again.

If it already has this property, you don't have to do 'repo sync', just rm -rf out && build.sh

Then try to run the marionette test

B2G$>./mach marionette-webapi gecko/dom/nfc/tests/marionette/test_nfc_manager_enabled.js

> 
> On the test case part, Was your idea to add similar nfc tests on the lines
> of 'gecko/dom/system/gonk/tests' 

No,
This tests are xpcshell-tests, and we dind't implement it yet, Bug 907252

Comment 8

[psiddh]

Attachment: (v1) Part 1: Do not add target to registered list if token is invalid

Comment 9

[psiddh]

Attachment: (v1) Part 2: Marionette Test case for validating invalid session token

Comment 10

[psiddh]

Attachment: (v1) Part 1: Do not add target to registered list if token is invalid. r=allstars.chh

Comment 11

[psiddh]

Attachment: (v1) Part 2: Marionette Test case for validating invalid session token. r=allstars.chh

Comment 12

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

Comment on attachment 8420459 [details] [diff] [review]
(v1) Part 1: Do not add target to registered list if token is invalid. r=allstars.chh

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

Shouldn't we also modify nsNfc.js so getNFCTag and getNFCPeer should fail or return null?

Comment 13

[psiddh]

Yes Yoshi, I agree . we should do that. But I wasn't  sure if you would be ok with getNFCTag and getNFCPeer changes as part of this change (bug). But since you have asked now, I will update the patches and upload the new one for review.

Comment 14

[psiddh]

Hi Yoshi,

Since 'getNFCTag(..)' and 'getNFCPeer(...)' return MozNFCTag  and  MozNFCPeer respectively and not 'DOMRequest', Is there a nice way to propagate 'BadSession ID'  error response from Nfc.js to gaia applications in a synchronous way ? Maybe not, unless we change the signature of getNFCxxx interfaces to a 'DOMRequest' ?  One option that I tried was to add 'Throws' keyword as below.
  [Throws]
  MozNFCTag getNFCTag(DOMString sessionId);
  [Throws]
  MozNFCPeer getNFCPeer(DOMString sessionId);  
But here too, we may not be able to accomplish desired results.
In short, the dom calls to getNFCxxx apis can return 'null' in bad scenarios only if the entire flow is synchronous. We will lose the synchronous flow the moment the NfcContentHelper.js notifies Nfc.js 
'cpmm.sendAsyncMessage("NFC:SetSessionToken", {sessionToken: sessionToken,}); 

Any suggestions?. Maybe I am missing something here.

Comment 15

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

Any reason you don't like to use cpmm.sendSyncMessage?

Comment 16

[psiddh]

oh nice! I did not know of this. Let me try it out. Thanks!

Comment 17

[psiddh]

Attachment: (v1.1) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

Comment 18

[psiddh]

Attachment: (v1) Part 2: Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

Also cleaned up 'setSessionToken' (chrome only interface) as it is not required.

Comment 19

[psiddh]

Attachment: (v1.1) Part 3: Marionette Test case for validating invalid session token. r=allstars.chh

Added try, catch block to catch invalid / bad session token usecases

Comment 20

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

Comment on attachment 8424273 [details] [diff] [review]
(v1.1) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +125,3 @@
>      }
>      // Report session to Nfc.js only.
> +    return cpmm.sendSyncMessage("NFC:SetSessionToken", {

The retval of cpmm.sendSyncMessage doesn't match with setSessionToken.

Does your patch really work?

Comment 21

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

Comment on attachment 8424274 [details] [diff] [review]
(v1) Part 2:  Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

I don't see any code returning NFC_GECKO_SUCCESS.
(NFC_GECKO_SUCCESS is a bit ugly. Why not throw or some such? or return true or false?)

Comment 22

[psiddh]

Thanks for your comments.
Hi Olli, I have now changed the return val to a boolean. Please provide your feedback.

Yes Yoshi, earlier patch does work. I guess the reason for it working earlier is that the returned value is of a primitive type (number). Therefore when nsNfc.js checks for return value 'ret' , it internally checks for ret[0], I guess.
> let ret = this._nfcContentHelper.setSessionToken(sessionToken);
>    if (ret === NFC_GECKO_SUCCESS) {

However I corrected it with the latest patch. Please review

Comment 23

[psiddh]

Attachment: (v1.2) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

Comment 24

[psiddh]

Attachment: (v1.1) Part 2: Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

Comment 25

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

Comment on attachment 8425177 [details] [diff] [review]
(v1.1) Part 2:  Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

>+    obj.initialize(this._window, sessionToken);
>+    let isSet = this._nfcContentHelper.setSessionToken(sessionToken);
>+    if (isSet === true) {
>+      let nfcTag = this._window.MozNFCTag._create(this._window, obj);
>       return nfcTag;
>-    }

So why not just
if (this._nfcContentHelper.setSessionToken(sessionToken)) {
  return this._window.MozNFCTag._create(this._window, obj);
}


>+    obj.initialize(this._window, sessionToken);
>+    let isSet = this._nfcContentHelper.setSessionToken(sessionToken);
>+    if (isSet === true) {
>+      let nfcPeer = this._window.MozNFCPeer._create(this._window, obj);
>       return nfcPeer;
Similar here

Comment 26

[psiddh]

Attachment: (v1.2) Part 2: Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

Comment 27

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

Comment on attachment 8425176 [details] [diff] [review]
(v1.2) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

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

I applied this patch (and Part 2 nsNfc.js) and found mozNFC.getNFCTag/Peer() always throws error now.
It should be the uuid changed is missing.
r- for this.

::: dom/system/gonk/Nfc.js
@@ +371,5 @@
>        switch (msg.name) {
>          case "NFC:SetSessionToken":
> +          if (msg.json.sessionToken !== this.nfc.sessionTokenMap[this.nfc._currentSessionId]) {
> +            debug("Received invalid Session Token: " + msg.json.sessionToken + " - Do not register this target");
> +            return NFC.GECKO_NFC_ERROR_GENERIC_FAILURE;

update error code base on Bug 933595

@@ +377,1 @@
>            this._registerMessageTarget(this.nfc.sessionTokenMap[this.nfc._currentSessionId], msg.target);

let token = this.nfc.sessionTokenMap[this.nfc._currentSessionId];
if (msg.json.sessionToken !== token) {
  return ...;
}

this._registerMessageTarget(token);

@@ +377,4 @@
>            this._registerMessageTarget(this.nfc.sessionTokenMap[this.nfc._currentSessionId], msg.target);
>            debug("Registering target for this SessionToken : " +
>                  this.nfc.sessionTokenMap[this.nfc._currentSessionId]);
> +          return NFC.GECKO_NFC_ERROR_SUCCESS;

rebase again when Bug 933595 is landed.

::: dom/system/gonk/NfcContentHelper.js
@@ +129,2 @@
>      });
> +    return ((val[0] === NFC.GECKO_NFC_ERROR_SUCCESS) &&

return val[0] === NFC.SUCCESS;

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +25,3 @@
>  };
>  
>  [scriptable, uuid(10b2eb1b-3fe0-4c98-9c67-9e4c2274cd78)]

uuid

Comment 28

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

Comment on attachment 8424275 [details] [diff] [review]
(v1.1) Part 3: Marionette Test case for validating invalid session token. r=allstars.chh

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

::: dom/nfc/tests/marionette/test_nfc_peer.js
@@ +65,5 @@
> +  is(msg.techList[0], "P2P", "check for correct tech type");
> +  nfc.onpeerready = (function(evt) {
> +    try {
> +      // Use the following 'fakeSessionToken' instead of 'evt.detail'
> +      let peer = nfc.getNFCPeer("fakeSessionToken");

It seems you can do this check directly without callign extra tech-discover and setting onpeerready.

Comment 29

[psiddh]

Attachment: (v1.3) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

Re-based the changes and updated the UUID

Comment 30

[psiddh]

>I applied this patch (and Part 2 nsNfc.js) and found mozNFC.getNFCTag/Peer() always throws error now.
>It should be the uuid changed is missing.
While making IDL changes,I generally delete the 'objdir-gecko' dir. I guess that's the reason why the patches worked in my work-space in both cases: 'bad session token usage'  and 'proper usage of session tokens'.

Comment 31

[psiddh]

Attachment: (v1.1) Part 3: Marionette Test case for validating invalid session token. r=allstars.chh

Comment 32

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

Comment on attachment 8427902 [details] [diff] [review]
(v1.3) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

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

::: dom/system/gonk/NfcContentHelper.js
@@ +126,3 @@
>      });
> +    return ((val[0] === NFC.NFC_SUCCESS) &&
> +            (val.length == 1)) ? true : false;

check my previous comment

Comment 33

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

(In reply to Siddartha P from comment #30)
> While making IDL changes,I generally delete the 'objdir-gecko' dir. 

The first thing you should ask yourself 'Did I update the UUID'?
That's one most common mistake made by many developers.

And you don't have to do a fresh build every time you modified IDL.

Comment 34

[psiddh]

Attachment: (v1.4) Part 1: Do not add target to registered list, if session token is invalid. r=allstars.chh

Comment 35

[psiddh]

Attachment: (v1.2) Part 3: Marionette Test case for validating invalid session token. r=allstars.chh

After the bug 1012620 changes, marionette test case had to modified. Asking Yoshi for review just in case...

Comment 36

[psiddh]

Comment on attachment 8429430 [details] [diff] [review]
(v1.2) Part 3: Marionette Test case for validating invalid session token. r=allstars.chh

T.C passes as it already supports promises.

Comment 37

[psiddh]

Attachment: (v1.3) Part 2: Throw error while using getNFCTag() & getNFCPeer() interfaces with invalid session tokens r=smaug

Rebase Part 2 / Dom Patch (Since bug 952486 already landed)

Comment 38

[psiddh]

Try Server results mostly green : https://tbpl.mozilla.org/?tree=Try&rev=e8f5d9836e75

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/88a815058943
https://hg.mozilla.org/integration/b2g-inbound/rev/1c4652d7f594
https://hg.mozilla.org/integration/b2g-inbound/rev/b9ef939b3385

Comment 40

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/88a815058943
https://hg.mozilla.org/mozilla-central/rev/1c4652d7f594
https://hg.mozilla.org/mozilla-central/rev/b9ef939b3385