In case of techDiscoverd and techLost message from nfc worker we do not have a status property. This causes adding the generic errorMsg to the message, before sending it to upper layers. This should be removed.

Dimi if it's ok with you, I will reenable the missing marionette tests case, as I will be needing them in this bug.

No problem, sorry for removing it accidentally before :(

Tested on gecko f297dd29f28bb736dfcfd374dfc62c02ed4f871c and works fine. Retest needed once latest emulator will be fixed. Should be applied after Bug 1019436 to have better performance.

Comment on attachment 8435090 [details] [diff] [review] 0001-Bug-1019668-NFC-Nfc.js-do-not-add-errorMsg-to-techDi.patch Review of attachment 8435090 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Nfc.js @@ +499,2 @@ > message.errorMsg = this.getErrorMessage(message.status); > } Move this if clause to messages handlers with the 'status' property, or if (message.status) or if (message.status !== undefined && message.status !== NFC.NFC_SUCCESS) if you want to make it more clear.

Comment on attachment 8435090 [details] [diff] [review] 0001-Bug-1019668-NFC-Nfc.js-do-not-add-errorMsg-to-techDi.patch Review of attachment 8435090 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/manifest.ini @@ +9,5 @@ > [test_nfc_peer.js] > [test_nfc_peer_sendndef.js] > [test_nfc_tag.js] > +[test_nfc_checkP2PRegistration.js] > +[test_nfc_error_messages.js] Dimi said he removed these by accident before, so I'd suggest move this to another patch or bug.

white spaces removed

Same here, white spaces removed.

Comment on attachment 8436898 [details] [diff] [review] Part:1 Not adding errorMsg if status undefined Review of attachment 8436898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/test_nfc_error_messages.js @@ +78,5 @@ > > +/** > + * Enables nfc and RE0, registers tech-discovered msg handler, once it's > + * fired set tech-lost handler and disables nfc. In both handlers checks > + * if error message is not present. nit. trailing ws. @@ +172,5 @@ > > +function setTechDiscoveredHandler() { > + let deferred = Promise.defer(); > + > + var techDiscoveredHandler = function(msg) { s/var/let/ @@ +189,5 @@ > + > +function setAndFireTechLostHandler() { > + let deferred = Promise.defer(); > + > + var techLostHandler = function(msg) { ditto @@ +200,5 @@ > + }; > + > + window.navigator.mozSetMessageHandler('nfc-manager-tech-lost', > + techLostHandler); > + toggleNFC(false); This should be done outside, not inside this function.

Fixed s/var/let, removed white space. As agreed on IRC left toggleNFC(false), should be refactored in followup bug when we will have support for tech-lost triggering in emulator (Bug 1023079).

Submitting once more to obsolete previous patch.

https://hg.mozilla.org/integration/b2g-inbound/rev/3e7908638c2a https://hg.mozilla.org/integration/b2g-inbound/rev/9d7ae79baf16

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #12) > Submitting once more to obsolete previous patch. This can be done retrospectively, without needing to upload another patch, by going to the details page (eg https://bugzilla.mozilla.org/attachment.cgi?id=8437523&action=edit) -> "edit details" -> check the "obsolete" input field.

Backed out for timeouts/crashes/...: 03:04:41 INFO - TEST-START test_nfc_error_messages.js 03:04:45 INFO - /builds/slave/test/build/tests/marionette/tests/dom/nfc/tests/marionette/test_nfc_error_messages.js, runTest (marionette_test.MarionetteJSTestCase) ... ERROR 03:04:47 INFO - START LOG: 03:04:47 ERROR - Error getting log: Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors. 03:04:47 INFO - END LOG: 03:04:47 INFO - ====================================================================== 03:04:47 INFO - ERROR: None 03:04:47 INFO - ---------------------------------------------------------------------- 03:04:47 INFO - Traceback (most recent call last): 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette_test.py", line 170, in run 03:04:47 INFO - testMethod() 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette_test.py", line 497, in runTest 03:04:47 INFO - filename=os.path.basename(self.jsFile)) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 1070, in execute_js_script 03:04:47 INFO - line=None) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/decorators.py", line 35, in _ 03:04:47 INFO - return func(*args, **kwargs) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 621, in _send_message 03:04:47 INFO - response = self.client.send(message) 03:04:47 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_transport/transport.py", line 100, in send 03:04:47 INFO - response = self.receive() 03:04:47 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_transport/transport.py", line 57, in receive 03:04:47 INFO - raise IOError(self.connection_lost_msg) 03:04:47 ERROR - TEST-UNEXPECTED-FAIL | test_nfc_error_messages.js | IOError: Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors. eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=41428664&tree=B2g-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41433939&tree=B2g-Inbound remote: https://hg.mozilla.org/integration/b2g-inbound/rev/4f11ff882c7c remote: https://hg.mozilla.org/integration/b2g-inbound/rev/f23a77c2ddb1

This the same error which i'm getting in all nfc tests on latest gecko, after repo sync (Bug 1021180). Nobody else was reporting this. On gecko f297dd29f28bb736dfcfd374dfc62c02ed4f871c everything works fine. Will try to land this once 1021180 is solved.

One remark here. In the detailed log, it's visible that all the other NFC tests are being skipped because there is no NFC hardware on the device.

test_nfc_error_messages.js was failing because of seg fault in MozNDEFRecord. More details can be seen here https://bugzilla.mozilla.org/show_bug.cgi?id=1021180#c14. The problem with MozNDEFRecord is being solved in Bug 1020034. Once it lands I'll retest this once more.

Bug 1020034 has landed. I've retested everything and it works fine. Added one small thing in the patch which was pointed out by Thomas. First argument in MozNDEFRecord constructor is not Uint8Array: >-const NDEF_MESSAGE = [new MozNDEFRecord(new Uint8Array(0x01), >+const NDEF_MESSAGE = [new MozNDEFRecord(0x01, Apart from that everything is the same.

https://hg.mozilla.org/integration/b2g-inbound/rev/6e455ae7ac2f https://hg.mozilla.org/integration/b2g-inbound/rev/716bf10db437

https://hg.mozilla.org/mozilla-central/rev/6e455ae7ac2f https://hg.mozilla.org/mozilla-central/rev/716bf10db437

[Blocking Requested - why for this release]: Request to uplift this to 2.0 as 2.0 branch also has this problem. There will be an extra property "errorMsg" contained in the system message.

This patch applies directly to gecko 2.0. I'm doing 2.0 flame build to check if everything is fine. After this I'll request the uplift. I was trying to run 2.0 marionette web-api tests but I cannot start them (see error log below). Will rebuild emulator once more if this persists I'll file a separate bug. >Error running mach: > ['marionette-webapi', 'gecko/dom/nfc/tests/marionette/manifest.ini'] >The error occurred in code that was called by the mach command. This is either >a bug in the called code itself or in the way that mach is calling it. >You should consider filing a bug for this issue. >If filing a bug, please include the full output of mach, including this error >message. >The details of the failure are as follows: >ImportError: cannot import name uses_marionette > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/mach_commands.py", line 90, in run_marionette_webapi > testtype=testtype, topsrcdir=self.topsrcdir, address=None) > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/mach_commands.py", line 30, in run_marionette > from marionette.runtests import ( > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/__init__.py", line 5, in <module> > from gestures import smooth_scroll, pinch > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/gestures.py", line 5, in <module> > from marionette import MultiActions, Actions > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/marionette.py", line 29, in <module> > import geckoinstance > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/geckoinstance.py", line 9, in <module> > from mozrunner import Runner > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/__init__.py", line 6, in <module> > from .local import * > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/local.py", line 20, in <module> > from .base import Runner > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/base/__init__.py", line 2, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/base/device.py", line 15, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/devices/__init__.py", line 5, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/devices/emulator.py", line 20, in <module>>

Tested on flame with 2.0, works ok. As commented above I've filed Bug 1051762 for problems with marionette test.

Comment on attachment 8440710 [details] [diff] [review] Part:1 Not adding errorMsg if status undefined, applied Yoshis suggestions , applied Thomas remark Requesting for uplift as stated in comment 22. Tested on flame with 2.0 build.

Comment on attachment 8436899 [details] [diff] [review] Part 2: Reenabling missing marionette tests Requesting for uplift as stated in comment 22. Marionette tests enabled, does not affect the user.

Comment on attachment 8436899 [details] [diff] [review] Part 2: Reenabling missing marionette tests testonly changes approving for uplift and clearing the blocking flag here.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/89003c7eb4c4 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f5f52be48ea4

Backed out from b2g32 for marionette-webapi permafail. https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6495a7bd61ed https://tbpl.mozilla.org/php/getParsedLog.php?id=45701741&tree=Mozilla-B2g32-v2.0

Ah, I'm guessing that's bug 1021180. Looks like some deps need uplift here.

Try run with some of the other deps on this bug included. If it's green, I'll go ahead and push it. https://tbpl.mozilla.org/?tree=Try&rev=507608719f53

We could try to uplift Bug 1020034, which solves this. But I think we might run into some more problems, so I would advise not to do this. The error is caused by the usage of MozNDEFRecord constructor in test_nfc_error_messages.js. I can prepare a 2.0 version of the patch which tests the introduced changes without using MozNDEFRecord.

Problematic MozNDEFRecord usage removed. Cannot run marionette tests locally for v2.0 due to Bug 1051762, same solution works fine for master so this should clear the errors.

Comment on attachment 8471430 [details] [diff] [review] Part 1: b2g32_v2_0 Gecko version Discussed this with tauzen and we decided to go with the approach from the Try push (which was green) to keep the code uniform across the branches.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e7b49fdb07d6 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f100dd10a6f3