1019668 - [NFC] Nfc.js - do not add errorMsg to techDiscovered and techLost, reenable marionette tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 2

[Dimi Lee [:dimi]]

No problem, sorry for removing it accidentally before :(

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 0001-Bug-1019668-NFC-Nfc.js-do-not-add-errorMsg-to-techDi.patch

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 4

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

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 5

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

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.

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 1: Not adding errorMsg if status undefined

Comment 7

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 2 Reenabling missing marionette tests

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part:1 Not adding errorMsg if status undefined

white spaces removed

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 2: Reenabling missing marionette tests

Same here, white spaces removed.

Comment 10

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

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.

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part:1 Not adding errorMsg if status undefined, applied Yoshis suggestions

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

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part:1 Not adding errorMsg if status undefined, applied Yoshis suggestions

Submitting once more to obsolete previous patch.

Comment 13

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

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

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 18

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 19

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part:1 Not adding errorMsg if status undefined, applied Yoshis suggestions , applied Thomas remark

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

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

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

Comment 23

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 24

[Krzysztof Mioduszewski[:tauzen]]

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

Comment 25

[Krzysztof Mioduszewski[:tauzen]]

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 26

[Krzysztof Mioduszewski[:tauzen]]

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 27

[bhavana bajaj [:bajaj]]

Comment on attachment 8436899 [details] [diff] [review]
Part 2: Reenabling missing marionette tests

testonly changes approving for uplift and clearing the blocking flag here.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Krzysztof Mioduszewski[:tauzen]]

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.

Comment 33

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 1: b2g32_v2_0 Gecko version

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 34

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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