Closed
Bug 976402
Opened 11 years ago
Closed 10 years ago
[Parent Process - Nfc.js] Do not even add the target if the session IDs do not match
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: psiddh, Assigned: psiddh)
References
Details
Attachments
(3 files, 14 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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)
Attachment #8381117 -
Attachment is obsolete: true
Attachment #8381117 -
Flags: review?(allstars.chh)
Attachment #8381119 -
Flags: review?(allstars.chh)
Hi Sidd, can you also write a test case?
Sure Yoshi, I will. Do you suggest a test case @ gecko/dom/nfc/tests ?
Yes, gecko/dom/nfc/tests/marionette see sample from Bug 935525.
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?
(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
Attachment #8381119 -
Flags: review?(allstars.chh)
Updated•11 years ago
|
Blocks: b2g-NFC-2.0
Updated•11 years ago
|
blocking-b2g: --- → backlog
Attachment #8381119 -
Attachment is obsolete: true
Attachment #8420456 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8420456 -
Attachment is obsolete: true
Attachment #8420456 -
Flags: review?(allstars.chh)
Attachment #8420459 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8420457 -
Attachment is obsolete: true
Attachment #8420457 -
Flags: review?(allstars.chh)
Attachment #8420460 -
Flags: review?(allstars.chh)
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?
Attachment #8420459 -
Flags: review?(allstars.chh)
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.
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.
Any reason you don't like to use cpmm.sendSyncMessage?
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8420459 -
Attachment is obsolete: true
Attachment #8424273 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 18•11 years ago
|
||
Also cleaned up 'setSessionToken' (chrome only interface) as it is not required.
Attachment #8424274 -
Flags: superreview?(bugs)
Assignee | ||
Comment 19•11 years ago
|
||
Added try, catch block to catch invalid / bad session token usecases
Attachment #8420460 -
Attachment is obsolete: true
Attachment #8420460 -
Flags: review?(allstars.chh)
Attachment #8424275 -
Flags: review?(allstars.chh)
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?
Attachment #8424273 -
Flags: review?(allstars.chh) → review-
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?)
Attachment #8424274 -
Flags: superreview?(bugs)
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
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8424273 -
Attachment is obsolete: true
Attachment #8425176 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8424274 -
Attachment is obsolete: true
Attachment #8425177 -
Flags: review?(bugs)
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
Attachment #8425177 -
Flags: review?(bugs) → review+
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
Attachment #8425176 -
Flags: review?(allstars.chh) → review-
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.
Attachment #8424275 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 29•10 years ago
|
||
Re-based the changes and updated the UUID
Attachment #8425176 -
Attachment is obsolete: true
Attachment #8427902 -
Flags: review?(allstars.chh)
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'.
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8424275 -
Attachment is obsolete: true
Attachment #8427934 -
Flags: review?(allstars.chh)
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
Attachment #8427902 -
Flags: review?(allstars.chh) → review+
Attachment #8427934 -
Flags: review?(allstars.chh) → review+
(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.
Assignee | ||
Comment 35•10 years ago
|
||
After the bug 1012620 changes, marionette test case had to modified. Asking Yoshi for review just in case...
Attachment #8427934 -
Attachment is obsolete: true
Attachment #8429430 -
Flags: review?(allstars.chh)
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.
Attachment #8429430 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 37•10 years ago
|
||
Rebase Part 2 / Dom Patch (Since bug 952486 already landed)
Attachment #8425698 -
Attachment is obsolete: true
Assignee | ||
Comment 38•10 years ago
|
||
Try Server results mostly green : https://tbpl.mozilla.org/?tree=Try&rev=e8f5d9836e75
Keywords: checkin-needed
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
Updated•10 years ago
|
Keywords: checkin-needed
Target Milestone: --- → 2.0 S3 (6june)
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
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•