psiddh Assignee
	Description • 11 years ago

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)

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 3 • 11 years ago

Hi Sidd, can you also write a test case?

	psiddh Assignee
	Comment 4 • 11 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 5 • 11 years ago

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

	psiddh Assignee
	Comment 6 • 11 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 7 • 11 years ago

(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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 12 • 11 years ago

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?

	psiddh Assignee
	Comment 13 • 11 years ago

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.

	psiddh Assignee
	Comment 14 • 11 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 15 • 11 years ago

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

	psiddh Assignee
	Comment 16 • 11 years ago

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

	psiddh Assignee
	Comment 18 • 11 years ago

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

	psiddh Assignee
	Comment 19 • 11 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 20 • 10 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 10 years ago

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?)

	psiddh Assignee
	Comment 22 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 27 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 28 • 10 years ago

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.

	psiddh Assignee
	Comment 29 • 10 years ago

Re-based the changes and updated the UUID

	psiddh Assignee
	Comment 30 • 10 years ago

>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'.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 32 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 33 • 10 years ago

(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.

	psiddh Assignee
	Comment 35 • 10 years ago

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

	psiddh Assignee
	Comment 36 • 10 years ago

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.

	psiddh Assignee
	Comment 37 • 10 years ago

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

	psiddh Assignee
	Comment 38 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 39 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 10 years ago

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