User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36 Steps to reproduce: Nfc Chrome process to handle different errors such as Invaid Sessions, Read / Write Access issues etc and other error codes as per Gonk/nfcd protocol. This is is necessary to report the appropriate error back to content / gaia other than simply reporting 'GENERIC_FAILURE' for all error scenarios.

Sid, maybe you can take this bug and add test case for this if it is possible.

This depends on nfcd needs to add to error code first, Bug 972248. However I spot one problem today which I think should be fixed in this bug. Right now in NfcContentHelper.js it always calls fireRequestError with the error code (right now it's 1), but the error here should be error message, which is of string format, like "GenericError".

Ok, so I understand that once we will have the error codes supported in nfcd the next steps would be: 1. Define a mapping between error codes and human-readable error messages 2. Make sure that error messages are propagated to content/gaia instead of error codes I'll investigate this later on once 972248 will land. Right now I assume that to have point 2. working the best way would be to modify: https://github.com/mozilla/gecko-dev/blob/master/dom/system/gonk/NfcContentHelper.js#L386 and use the mapping defined in point 1. to fire Services.DOMRequest.fireError(request, errorMessage).

This still needs to wait for Bug 1009483 to land as some test scenarios are failing because of it - sessionToken is not being cleared when NFC is disabled. The test cases are checking mostly Gecko originated bugs, which are using the same code paths as the ones which originate from nfcd. I was trying to find a way to trigger nfcd errors from tests, but it's quite difficult with emulator command which we have right now. Dimi maybe you could suggest a way to generate nfcd errors in emulator?

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #8) > This still needs to wait for Bug 1009483 to land as some test scenarios are > failing because of it - sessionToken is not being cleared when NFC is > disabled. The test cases are checking mostly Gecko originated bugs, which > are using the same code paths as the ones which originate from nfcd. I was > trying to find a way to trigger nfcd errors from tests, but it's quite > difficult with emulator command which we have right now. > > Dimi maybe you could suggest a way to generate nfcd errors in emulator? I am sorry but currently emulator do not support generate nfcd error code. Maybe we could check with thomas about if it is possible to support that in the future.

Thomas do you know if there is some scenario which would trigger an nfcd error in the emulator? If no is this possible to support this in the future?

It took me a lot of work to _not_ make the emulator trigger errors in nfcd. :D We could definitely add some commands for fault injection if that makes sense for testing. What kind of error do you think of?

Preferably some problems with writing/sending ndef messages which would result in this error code https://github.com/mozilla-b2g/platform_system_nfcd/blob/master/src/NfcGonkMessage.h#L55

Generally speaking, [NCI], Sec 4.5, 4.6 define CORE_GENERIC_ERROR_NTF and CORE_INTERFACE_ERROR_NTF. We shouldn't fail in the emulator if nfcd sent a correct command IMHO, but we could return them where failure is possible. My guess is that NFC_ERROR_WRITE is generated when a data transmission fails. [NCI] Sec 8.2.3.4, 8.2.4.4, 8.3.4.2, 8.3.3.4, 8.4.3.3, 8.4.4.4 define behavior in cases where the layer 1 and 2 transport protocols fail. We could emulate such failures and generate them from the console interface. There can be a lot of message passing behind the scenes. I guess the biggest problem is to trigger the error at the right point.

This is seems like too much work. As you wrote it's better to have the emulator working properly and support more commands than generate errors ;). Maybe triggering NFC_ERROR_BAD_TECH_TYPE would be easier? I'll try to use the current sessionToken which i get when RE0 is transmitting and will try to do mozNfcTag.connect with some other tech.

Comment on attachment 8424073 [details] [diff] [review] Part 1: Code changes in gecko to pass error messages to gaia Review of attachment 8424073 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Nfc.js @@ +465,5 @@ > * > * @param message > * An nsIMessageListener's message parameter. > */ > + sendNfcErrorResponse: function sendNfcErrorResponse(message, errorCode) { It seems we can reuse the 'message' object here so we don't need another argument. Also I think we could do the error message mapping here so NfcContentHelper could be simpler. So the message.json object could have following properties { sessionId: .., requestId: .., errorMsg: ... } @@ +474,5 @@ > let nfcMsgType = message.name + "Response"; > message.target.sendAsyncMessage(nfcMsgType, { > sessionId: message.json.sessionToken, > requestId: message.json.requestId, > + status: errorCode Therefore this line could be just message.target.sendAsyncMessage(nfcMsgType, message.json); ::: dom/system/gonk/NfcContentHelper.js @@ +382,5 @@ > ", result: " + JSON.stringify(result)); > Services.DOMRequest.fireSuccess(request, result); > }, > > fireRequestError: function fireRequestError(requestId, error) { s/error/errorMsg @@ +386,5 @@ > fireRequestError: function fireRequestError(requestId, error) { > let request = this.takeRequest(requestId); > + let errorMsg = (error in NFC.NFC_ERROR_MSG) ? > + NFC.NFC_ERROR_MSG[error] : > + NFC.NFC_ERROR_MSG[NFCG.NFC_GECKO_ERROR_GENERIC_FAILURE]; Don't need this anymore. Or do this in Nfc.js @@ +420,5 @@ > case "NFC:WriteNDEFResponse": > case "NFC:MakeReadOnlyNDEFResponse": > case "NFC:NotifySendFileStatusResponse": > case "NFC:ConfigResponse": > + if (result.status !== NFC.NFC_SUCCESS) { if (result.errorMsg) { this.fireRequestError(atob(result.requestId), result.errorMsg); } ::: dom/system/gonk/nfc_worker.js @@ +153,5 @@ > message.type = "ReadNDEFResponse"; > message.sessionId = sessionId; > message.records = records; > + message.status = error; > + Remove the extra line, same for below.

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #14) > This is seems like too much work. As you wrote it's better to have the > emulator working properly and support more commands than generate errors ;). > Yeah I also agree it's too much work, since we have a bug for xpcshell-test Bug 907252, we can move some unit test code there.

Comment on attachment 8424075 [details] [diff] [review] Added promises to toggleNFC and activateRE0 helper method Review of attachment 8424075 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/head.js @@ +35,5 @@ > run: run > }; > }()); > > function toggleNFC(enabled, callback) { If you're making this as 'Promise', I think we should remove the callback arg here, and callers could do toggleNFC().then(() => callback) on their own.

Comment on attachment 8424076 [details] [diff] [review] test cases for mozNFC.checkP2PRegistration Review of attachment 8424076 [details] [diff] [review]: ----------------------------------------------------------------- check wrapDomRequestAsPromise [1] to see if it can make your patch shorter. [1] http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#91

Comment on attachment 8424073 [details] [diff] [review] Part 1: Code changes in gecko to pass error messages to gaia Add 'Part 1' prefix.

Comment on attachment 8424075 [details] [diff] [review] Added promises to toggleNFC and activateRE0 helper method Review of attachment 8424075 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/head.js @@ +35,5 @@ > run: run > }; > }()); > > function toggleNFC(enabled, callback) { Krzysztof said we will do this in a follow-up bug, so it's okay for me the keep the callback arg here. @@ +62,5 @@ > + > + return deferred.promise; > +} > + > +function activateRE0() { There's a function with the same name in test_nfc_peer_sendndef.js. Or you forgot to remove it in test_nfc_peer_sendndef.js?

Comment on attachment 8424076 [details] [diff] [review] test cases for mozNFC.checkP2PRegistration Review of attachment 8424076 [details] [diff] [review]: ----------------------------------------------------------------- Nits, there are some trailing spaces spaces. The test looks okay, but I think you need to wrapDomRequestAsPromise function here. ::: dom/nfc/tests/marionette/test_nfc_checkP2PRegistration.js @@ +19,5 @@ > + > + request.onsuccess = function() { > + log('got on success, result should be false: ' + request.result); > + is(request.result, false, > + 'request.status should be false, onpeerready not registered, no session token'); request.result should be false...

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #14) > This is seems like too much work. As you wrote it's better to have the > emulator working properly and support more commands than generate errors ;). Maybe 'too much' is a bit strong. :) Generating the error would mean to set a flag for certain operations to fail, and then fail on the operation. And it's the only reliably way of testing nfcd's error handling. In the other comment, I was mostly talking about the general case. But if we have a simple case, we can start with that.

I just don't want the emulator to be able to fail for valid and correct NCI commands, because that makes no sense. Errors in the underlying layers are fine to fail on.

(In reply to Yoshi Huang[:allstars.chh] from comment #20) > Krzysztof said we will do this in a follow-up bug, so it's okay for me the > keep the callback arg here. Filed the bug here Bug 1012620. Will take care of it once this will land. > @@ +62,5 @@ > > + > > + return deferred.promise; > > +} > > + > > +function activateRE0() { > > There's a function with the same name in test_nfc_peer_sendndef.js. > Or you forgot to remove it in test_nfc_peer_sendndef.js? Yes, I've seen this but I didn't want to introduce changes to other tests now. I think this method could be used in some other scenarios also, as some of the tests are using a callback version of it. I think we should file another bug to refactor other tests to use this method and in it also remove the activateRE0 from test_nfc_peer_sendndef.js. Is it ok with you?

Or just do the activateRE0 cleanup as part of Bug 1012620 ;).

Comment on attachment 8424077 [details] [diff] [review] test cases for nfc error messages Review of attachment 8424077 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/test_nfc_error_messages.js @@ +1,2 @@ > +'use strict'; > + No license? @@ +1,3 @@ > +'use strict'; > + > + extra line @@ +1,4 @@ > +'use strict'; > + > + > +/* globals log, is, ok, runTests, toggleNFC, runNextTest, trailing ws, and below. @@ +8,5 @@ > +const MARIONETTE_HEAD_JS = 'head.js'; > + > +const MANIFEST_URL = 'app://system.gaiamobile.org/manifest.webapp'; > +const NDEF_MESSAGE = [new MozNDEFRecord(new Uint8Array(0x01), > + new Uint8Array(0x84), align @@ +9,5 @@ > + > +const MANIFEST_URL = 'app://system.gaiamobile.org/manifest.webapp'; > +const NDEF_MESSAGE = [new MozNDEFRecord(new Uint8Array(0x01), > + new Uint8Array(0x84), > + new Uint8Array(0), could use undefined so we don't create extra object. @@ +15,5 @@ > + > +let nfcPeers = []; > +let sessionTokens = []; > + > +/**git stati typo? @@ +18,5 @@ > + > +/**git stati > + * Enables nfc and RE0 then registers onpeerready callback and once > + * it's fired it creates mozNFCPeer and stores it for later. > + * After disabling nfc tries to do mozNFCPeer.sendNdef which should sendNDEF @@ +33,5 @@ > + .then(afterEach); > +} > + > +/** > + * Enables nfc and RE0, register onpeerready callback, once it fires 'fires' at the end seems not neccesary. @@ +36,5 @@ > +/** > + * Enables nfc and RE0, register onpeerready callback, once it fires > + * creates and stores mozNFCPeer. Disables nfc, enables nfc and > + * once again registers and fires new onpeerready callback and stores > + * mozNfcPeer. Than fires sendNdef on the first stored peer which nit, sendNDEF. @@ +54,5 @@ > + .then(()=>toggleNFC(false)) > + .then(afterEach); > +} > + > +function afterEach() { rename it to better naming, like cleanUp(); @@ +75,5 @@ > + request.onsuccess = function() { > + log('onsuccess should have result.status true: ' + request.result); > + is(request.result, true, 'P2P registration result'); > + if(request.result) { > + nfc.notifyUserAcceptedP2P(MANIFEST_URL); deferred.resolve(); is missing here? @@ +98,5 @@ > + > + let req = peer.sendNDEF(NDEF_MESSAGE); > + req.onsuccess = function() { > + ok(false, 'success on sending ndef not possible shoudl get: ' + errorMsg); > + deferred.resolve(); should be deferred.rejected();?

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #24) > > > +function activateRE0() { > > > > There's a function with the same name in test_nfc_peer_sendndef.js. > > Or you forgot to remove it in test_nfc_peer_sendndef.js? > > Yes, I've seen this but I didn't want to introduce changes to other tests > now. I think this method could be used in some other scenarios also, as some > of the tests are using a callback version of it. I think we should file > another bug to refactor other tests to use this method and in it also remove > the activateRE0 from test_nfc_peer_sendndef.js. Is it ok with you? You have modified head.js and this will affect all tests including this, also in this case the naming is the same. So if one of them is buggy it will cost someone several hours to find out. I'd suggest you use a different naming here.

(In reply to Yoshi Huang[:allstars.chh] from comment #26) > @@ +75,5 @@ > > + request.onsuccess = function() { > > + log('onsuccess should have result.status true: ' + request.result); > > + is(request.result, true, 'P2P registration result'); > > + if(request.result) { > > + nfc.notifyUserAcceptedP2P(MANIFEST_URL); > > deferred.resolve(); is missing here? This promise is resolved in onpeerready callback couple of lines above. Here I just trigger gecko to fire the onpeerready event. > @@ +98,5 @@ > > + > > + let req = peer.sendNDEF(NDEF_MESSAGE); > > + req.onsuccess = function() { > > + ok(false, 'success on sending ndef not possible shoudl get: ' + errorMsg); > > + deferred.resolve(); > > should be deferred.rejected();? I don't want to break the promise chain here as it's the last step in the whole test scenario. After this i need to do regular clean up no matter if this fails or not. If i would reject the promise i would need to do the clean up here or in the onRejected handler, and the the code would be same as in onFulfilled handler. So this way i'm not doubling the code and the test scenario function is more readable i think.

(In reply to Yoshi Huang[:allstars.chh] from comment #27) > You have modified head.js and this will affect all tests including this, > also in this case the naming is the same. So if one of them is buggy it will > cost someone several hours to find out. > > I'd suggest you use a different naming here. This is a good point, I agree with you. Will propose a different name. Thank you very much for the review. I'll incorporate your remarks in the code.

I'm reusing message as suggested by Yoshi. Error code to message mapping is done in Nfc.js now, and errorMsg is passed to NfcContentHelper.

Renamed activateRE0 to enableRE0 to avoid mixup and confusion with activateRE0 used in other tests.

Wrapped checkP2PRegistration DOMRequest and rewrote the tests. They are shorter now as expected.

Added one more test case which tests nfcd error NFC_ERROR_DISCONNECT. Added proper promise rejection handling.

Comment on attachment 8427060 [details] [diff] [review] Part 1: Gecko code changes to pass error messages to gaia Review of attachment 8427060 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Nfc.js @@ +481,5 @@ > + if(!(errorCode in NFC.NFC_ERROR_MSG)) { > + errorCode = NFC.NFC_GECKO_ERROR_GENERIC_FAILURE; > + } > + > + return NFC.NFC_ERROR_MSG[errorCode]; no need for the if check. just return NFC.NFC_ERROR_MSG[errorCode] || "defaultErrorString";

Comment on attachment 8427067 [details] [diff] [review] Part 3: Marionette tests for mozNfc.checkP2PRegistrat Review of attachment 8427067 [details] [diff] [review]: ----------------------------------------------------------------- looks good, but you forgot to modify manifest.ini

Comment on attachment 8427070 [details] [diff] [review] Part 4: Marionette tests checking error messages Review of attachment 8427070 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/test_nfc_error_messages.js @@ +35,5 @@ > + .catch(handleRejectedPromise); > +} > + > +/** > + * Enables nfc and RE0, register onpeerready callback, once it fires Previous comment 26 @@ +33,5 @@ > + .then(afterEach); > +} > + > +/** > + * Enables nfc and RE0, register onpeerready callback, once it fires 'fires' at the end seems not neccesary. @@ +38,5 @@ > +/** > + * Enables nfc and RE0, register onpeerready callback, once it fires > + * creates and stores mozNFCPeer. Disables nfc, enables nfc and > + * once again registers and fires new onpeerready callback and stores > + * mozNfcPeer. Than fires sendNdef on the first stored peer which Also asked in Comment 26. s/sendNdef/sendNDEF/

Comment on attachment 8427071 [details] [diff] [review] Part-5: Enabling new tests Review of attachment 8427071 [details] [diff] [review]: ----------------------------------------------------------------- Oh I don't know you put manifest.ini here. This modification should be included the previous part, not as a seperate part.

(In reply to Yoshi Huang[:allstars.chh] from comment #37) > 'fires' at the end seems not neccesary. I'll change this to 'once it's fire it creates' as it is in the previous comment so this will be grammatically correct. > @@ +38,5 @@ > > +/** > > + * Enables nfc and RE0, register onpeerready callback, once it fires > > + * creates and stores mozNFCPeer. Disables nfc, enables nfc and > > + * once again registers and fires new onpeerready callback and stores > > + * mozNfcPeer. Than fires sendNdef on the first stored peer which > > Also asked in Comment 26. > s/sendNdef/sendNDEF/ I'm sorry for this. I've changed this but was rebasing and this changes got lost. Will post new version.

Included suggestion from Comment 35

Combined tests and manifest.ini changes into one patch. Fixed typos in comments.

https://hg.mozilla.org/integration/b2g-inbound/rev/26841f68fc99 https://hg.mozilla.org/integration/b2g-inbound/rev/d74a0d77e9b8 https://hg.mozilla.org/integration/b2g-inbound/rev/f9823c4a6be9 also updated patch subject and added r=me.

https://hg.mozilla.org/mozilla-central/rev/26841f68fc99 https://hg.mozilla.org/mozilla-central/rev/d74a0d77e9b8 https://hg.mozilla.org/mozilla-central/rev/f9823c4a6be9