1043186 - B2G NFC: refine Peer event registration.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

Right now I spotted some problems for NFC peer events.

1. Every call for getNFCPeer will create a new MozNFCPeer object, even the sessions are the same.
2. Even we already received a peerlost event, the getNFCPeer and other NFCPeer API calls will still go across process boundary to check the validity of the session.
3. To receive the peerlost event mentioned above, child side has to register onpeerlost callback, otherwise peerlost will never be fired to child side.

And because there's no support for taglost event now so I won't handle *NFCTag* case in this bug.

Comment 1

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

Attachment: WIP. Part 1: Add a nsINfcPeerEventListener

Comment 2

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

Attachment: WIP. Part 2: refine onpeerlost

Comment 3

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

Attachment: WIP. Part 3: onpeerlost will invalidate MozNFCPeer.

Comment 4

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

Comment on attachment 8462474 [details] [diff] [review]
WIP. Part 3: onpeerlost will invalidate MozNFCPeer.

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

Hi, smaug
Once the notifyPeerLost is called, it will invalidate the MozNFCPeer object by setting the session to null.
Then calling peer.sendNDEF or peer.sendFile should fail.

I'd like to request for your feedback here,
once the MozNFCPeer is invalidated, should we do a silent error here (as I do now),
or should we throw exception?

Note that there is already some discussions for getNFCPeer in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1039239#c17
https://bugzilla.mozilla.org/show_bug.cgi?id=1039239#c18 
and https://bugzilla.mozilla.org/show_bug.cgi?id=1042651

So I'd like to make sure whether we should throw or not.

Thanks

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8462474 [details] [diff] [review]
WIP. Part 3: onpeerlost will invalidate MozNFCPeer.

I think exception would make sense here. The caller should know that
the send* call has failed.

Comment 6

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

Attachment: Part 1: Add nsINfcPeerEventListener

Comment 7

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

Attachment: Part 2: peerlost should be fired to correct target.

Comment 8

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

Attachment: Part 3: return cached NFCPeer if session matches.

Comment 9

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

Attachment: Part 4: Add [Throw] in MozNFCPeer.

Comment 10

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

Comment on attachment 8465186 [details] [diff] [review]
Part 1: Add nsINfcPeerEventListener

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

This patch does the followings:
- Add a nsINfcPeerEventListener
- Seperate peerready and peerlost callbacks in nsNfc.js, as peerlost shouldn't have parameter.

Comment 11

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

Comment on attachment 8465187 [details] [diff] [review]
Part 2: peerlost should be fired to correct target.

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

This patch is trying to fix
1. Right now onpeerlost is only notified to Content Side only if the content has registered onpeerlost. We'd like to notify peerlost to Content DOM side even the web app doesn't register this, this could help us to clear some resource residing in DOM part.

2. peerlost should be notified to the app which received onpeerready before. Image the following use case
2-1. In WebApp-Foo, User taps another device, and Shrinking UI pops up.
2-2. User swipes to share the content, and shrinking UI finishes.
2.3. User switch to another WebApp-Bar, which could have also registered onpeerlost
2.4. Now take the other phone away
Right now Gecko will send a peerlost to the later foreground app, WebApp-Bar, which is wrong here because,
it's the former foreground app, WebApp-Foo, received the onpeerready, so onpeerlost should be notified to the same app which received onpeerready before to clear the resources.

3. By the same reasoning in (2), take the later app, WebApp-Foo for example, if the webapp didn't receive onpeerready before, it shouldn't receive onpeerlost later, either.

Comment 12

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

Comment on attachment 8465188 [details] [diff] [review]
Part 3: return cached NFCPeer if session matches.

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

Use a cached object so when session matches we return the cached one.

Comment 13

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

Comment on attachment 8465189 [details] [diff] [review]
Part 4: Add [Throw] in MozNFCPeer.

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

If content side received the onpeerlost, it should invalidate the object,
and calling any API on that object later should throw.

Will send another r? to smaug for the WebIDL change once Dimi has reviewed this.

Comment 14

[Dimi Lee [OOO 19.Oct - 23.Oct]]

Comment on attachment 8465188 [details] [diff] [review]
Part 3: return cached NFCPeer if session matches.

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

::: dom/nfc/nsNfc.js
@@ +206,5 @@
> +      obj.initialize(this._window, sessionToken);
> +      this.nfcObject = obj;
> +      this.nfcObject.contentObject = this._window.MozNFCPeer._create(this._window, obj);
> +    }
> +

Is it possible we already have an nfcObject but the seesionToken is different ?
If yes, how do we handle this situation ?

Comment 15

[Dimi Lee [OOO 19.Oct - 23.Oct]]

Comment on attachment 8465187 [details] [diff] [review]
Part 2: peerlost should be fired to correct target.

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

Looks good to me, thanks

::: dom/nfc/tests/marionette/test_nfc_peer.js
@@ +103,5 @@
> +  toggleNFC(true)
> +    .then(() => NCI.activateRE(emulator.P2P_RE_INDEX_0))
> +    .then(NCI.deactivate)
> +    .then(() => toggleNFC(false))
> +

Nits. ;

::: dom/system/gonk/Nfc.js
@@ +249,1 @@
>        }

if (appInfo.appId in this.peerTargetsMap) {
  // Remove the target from the list of registered targets
  delete this.peerTargetsMap[appInfo.appId]  
}
You can also keep original style if you think it is better for reading

Comment 16

[Dimi Lee [OOO 19.Oct - 23.Oct]]

Comment on attachment 8465186 [details] [diff] [review]
Part 1: Add nsINfcPeerEventListener

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

::: dom/system/gonk/NfcContentHelper.js
@@ +283,1 @@
>  

Do we have to set  this.peerEventListener to null here?

Comment 17

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

Comment on attachment 8465188 [details] [diff] [review]
Part 3: return cached NFCPeer if session matches.

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

::: dom/nfc/nsNfc.js
@@ +206,5 @@
> +      obj.initialize(this._window, sessionToken);
> +      this.nfcObject = obj;
> +      this.nfcObject.contentObject = this._window.MozNFCPeer._create(this._window, obj);
> +    }
> +

Should be not.
this.nfcObject.session != sessionToken check is not neccesary.

@@ +275,5 @@
>        dump("Unpaired session for notifyPeerLost." + sessionToken);
>        return;
>      }
>  
> +    if (this.nfcObject.session == sessionToken) {

I'll also add a check here in case this.nfcObject is null.

Comment 18

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

Comment on attachment 8465186 [details] [diff] [review]
Part 1: Add nsINfcPeerEventListener

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

::: dom/system/gonk/NfcContentHelper.js
@@ +283,1 @@
>  

No, unregsiterTargetForPeerEvent is called when app calls
mozNfc.onpeerready = null;

If we set listener to null here there will be no way to add it back.

Comment 19

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

Comment on attachment 8465187 [details] [diff] [review]
Part 2: peerlost should be fired to correct target.

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

::: dom/system/gonk/Nfc.js
@@ +249,1 @@
>        }

Many functions here use the same naming rule here, so either change all or nothing, 
I'll file another bug for renaming these.

Comment 20

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

Attachment: Part 3: return cached NFCPeer if session matches. v2.

Addressed Comment 17.

Comment 21

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

Attachment: Part 4: Add [Throw] in MozNFCPeer. v2

Comment 22

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

Comment on attachment 8465292 [details] [diff] [review]
Part 4: Add [Throw] in MozNFCPeer. v2

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

r? to Smaug for adding [Throws] in MozNFCPeer.webidl

Comment 23

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

https://hg.mozilla.org/integration/b2g-inbound/rev/e437dd9b22c8
https://hg.mozilla.org/integration/b2g-inbound/rev/eb1fc82c5aec
https://hg.mozilla.org/integration/b2g-inbound/rev/776133b60472
https://hg.mozilla.org/integration/b2g-inbound/rev/922414e40855

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e437dd9b22c8
https://hg.mozilla.org/mozilla-central/rev/eb1fc82c5aec
https://hg.mozilla.org/mozilla-central/rev/776133b60472
https://hg.mozilla.org/mozilla-central/rev/922414e40855