1042651 - B2G NFC: getNFCPeer and getNFCTag should not throw error

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

This is a followup to the discussion in Bug 1039239. 
Currently getNFCPeer can throw an error when the session token passed as an argument is not valid. This can happen when other device is quickly moved away after the onpeerready callback was fired. Error throwing was introduced in this Bug 976402. Gaia apps which are using sharing (contacts, browser, gallery, music) do not have a try-catch block and are handling getNFCPeer inconsistently. 

In this bug we should discuss how to fix getNFCPeer API. Currently there are two propositions:
1. Removing the error throwing in getNFCPeer and return null instead - quicker needs small changes in gecko, needs small changes in current gaia apps.
2. Remove getNFCPeer and reimplement onpeerready to be compatible with W3C onpeerfound[1]. This would require more effort, but would simplify the API usage considerably and solve our problem in an elegant way (rejected promise when the other device is out of reach). Also we would not expose the session token to app developer. 

http://www.w3.org/TR/nfc/#widl-NFCManager-onpeerfound

Comment 1

[arno]

I suggest to make this bug more generic: getNFCTag() also throws an error in an error case.

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

Yoshi is currently reimplementing onpeerready handling in Bug 1046554, after this change it will be possible to obtain MozNFCPeer directly in the callback. Once he'll land this we will need to:
1. Fix gaia apps (contacts, gallery, music, system browser) to not use getNFCPeer
2. Remove getNFCPeer completely

This will solve the initial problem. I'm renaming the bug so the title will be more appropriate.

Comment 3

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

The problem is in nfc_handover_manager.js still uses getNFCPeer to doing handover,
which I think it's still a long way to go to completely remove getNFCPeer.

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

Right, I didn't think this all through, sorry for that. As Yoshi pointed out we still need to figure out how to handle this in NfcHandoverManager which is not listening for onpeerready.

Additionally, Sid performed a couple of tests and it's not possible to return null or undefined in getNFCPeer as it results in the following error:
>JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object.

Should getNFCPeer return a DOMRequest or a Promise then?

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #4)
> Additionally, Sid performed a couple of tests and it's not possible to
> return null or undefined in getNFCPeer as it results in the following error:
> >JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object.

Where does this error come from? Isn't it because the user of mozNFC.getNFCPeer expect it is an object and use it right away?

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> Where does this error come from? Isn't it because the user of
> mozNFC.getNFCPeer expect it is an object and use it right away?
The app does not use it right away, but the error is visible.

This is a more detailed log from a test which I've done:
>I/Gecko   ( 3391): -*- NfcContentHelper: Message received: {"target":{},"name":"NFC:DOMEvent","sync":false,"json":{"event":1,"sessionToken":"{c9778234-d3af-4fc7-a0c6-471013d66b05}","data":null},"data":{"event":1,"sessionToken":"{c9778234-d3af-4fc7-a0c6-471013d66b05}","data":null},"objects":{}}
>E/GeckoConsole( 3391): [JavaScript Error: "Return value of MozNFC.getNFCPeer is not an object."]
>I/Gonk    ( 3391): Setting nice for pid 3552 to 1
>I/Gonk    ( 3391): Changed nice for pid 3552 from 18 to 1.
>E/GeckoConsole( 3391): Content JS LOG at app://browser.gaiamobile.org/js/nfc.js:69 in nfc_handlePeerConnectvity: Browser: trying to getNFCPeer

I've checked Browser app and added there additional logging before and after call to MozNFC.getNFCPeer. It just retrieves MozNFCPeer. I'm not sure why "Return value of MozNFC.getNFCPeer is not an object." is printed out first, but no log after call to getNFCPeer is visible.

Comment 7

[Krzysztof Mioduszewski[:tauzen]]

Ok, I've modified MozNFC.webidl, so MozNFCPeer can be nullable. It's possible to remove error throwing and just return null. I'll prepare a patch with some tests and ask for a review.

Comment 8

[psiddh]

This is nice. We can then remove the 'throwing of exception' from webidl

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable

Hi Yoshi, can you review this? Should I ask somebody for the feedback on WebIDL changes?

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Gaia changes, NfcHandoverManger try-catch removal

I will ask for review if Gecko part will get r+.

Comment 11

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

Comment on attachment 8466074 [details] [diff] [review]
Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable

Review of attachment 8466074 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/nsNfc.js
@@ +195,5 @@
>      obj.initialize(this._window, sessionToken);
>      if (this._nfcContentHelper.setSessionToken(sessionToken)) {
>        return this._window.MozNFCPeer._create(this._window, obj);
>      }
> +    return null;

rebase here.

::: dom/nfc/tests/marionette/test_nfc_peer.js
@@ +91,1 @@
>    }

shouldn't use try-catch anymore.

@@ +102,5 @@
> +    let tag = nfc.getNFCTag("fakeSessionToken");
> +    is(tag, null, "NFCTag should be null on wrong session token");
> +  } catch (ex) {
> +    ok(false, "Exception should not be thrown");
> +  }

ditto

::: dom/webidl/MozNFC.webidl
@@ +45,5 @@
>  [JSImplementation="@mozilla.org/navigatorNfc;1",
>   NavigatorProperty="mozNfc",
>   Func="Navigator::HasNFCSupport"]
>  interface MozNFC : EventTarget {
> +   MozNFCTag? getNFCTag(DOMString sessionId);

Should add comments on this.
Also rename sessionId to session.

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable, v 1.1

Hi Yoshi, thank you for the review. I've addressed all your comments. Could you take a look on the new version?

Comment 13

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

Comment on attachment 8467714 [details] [diff] [review]
Gecko part - not throwing, MozNFCPeer and MozNFCTag nullable, v 1.1

Review of attachment 8467714 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thank you.

Forward r? to smaug for the WebIDL change.

Comment 14

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8466983 [details] [review]
Gaia changes, NfcHandoverManger try-catch removal

Hi Alive, since Gecko changes are r+ could you take a look at NfcHandoverManager patch? Try-catch was removed as you requested and I've updated the test cases.

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Hi Yoshi could you push the gecko part for try build?

Comment 16

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

If you have level 1 access you can do it by yourself, other you need to get Level 1 access first.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8466983 [details] [review]
Gaia changes, NfcHandoverManger try-catch removal

Nice.

Comment 18

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

Attachment: Gecko Patch. v1.2

rebase

Comment 19

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

b2g-inbound is close now, add checkin-needed

Comment 20

[Krzysztof Mioduszewski[:tauzen]]

Important note, Gecko and Gaia part need to land together.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b8c53e689955

Master: https://github.com/mozilla-b2g/gaia/commit/09365cfc38e1381c81a0a86b4ef22bcd16babe98

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8468311 [details] [diff] [review]
Gecko Patch. v1.2

v1.2 was rebased on top of bug 1046554, which was backed out on m-c. I backed out v1.2, merged m-c to b-i, then re-landed the v1.1 patch here.

https://hg.mozilla.org/integration/b2g-inbound/rev/d265bcfec5d8

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d265bcfec5d8