1055960 - B2G NFC: onpeerlost won't be called if peerready is reset

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

By definition onpeerlost should be called when the peer got in the peerready is lost.
So considering following case:

nfc.onpeerready = function () {
 ...
 nfc.onpeeready = null;
};

nfc.onpeerlost = function () {

};

inside onpeerready callback, it reset the handler to null so onpeerlost won't be fired.

Comment 1

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

[Blocking Requested - why for this release]:
regression from Bug 1043186

Comment 2

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

I'll fix three problems in this bug
1. Comment 0
2. only needs notifyUserAcceptedP2P to trigger onpeerready.
3. notifyPeerLost will check IsPeerReadyCalled, however it's buggy for if the 1st time onpeerready/onpeerlost both have been called, in the 2nd tech-discovered, we didn't do the Swipe (not calling notifyUserAcceptedP2P), and in the 2nd tech-lost, onpeerlost will still be called.

Comment 3

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

Attachment: Patch

Comment 4

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

Comment on attachment 8481231 [details] [diff] [review]
Patch

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

::: dom/nfc/gonk/Nfc.js
@@ +226,5 @@
> +
> +      // For peerlost, the message is delievered to the target which
> +      // onpeerready has been called before.
> +      this.notifyPeerEvent(this.currentPeer, NFC.NFC_PEER_EVENT_LOST, sessionToken);
> +      this.currentPeer = null;

this.currentPeer should be also set to null if the target is killed.

Comment 5

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

Attachment: Part 1: fix onpeerlost.

- remove this.currentPee in removeTarget
- remove function isPeerReadyTarget

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8482061 [details] [diff] [review]
Part 1: fix onpeerlost.

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

Looks good, thanks

::: dom/nfc/gonk/Nfc.js
@@ +176,5 @@
>      },
>  
> +    notifyPeerEvent: function notifyPeerEvent(target, event, sessionToken) {
> +      if (!target) {
> +        dump("invalid target");

debug

Comment 7

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> > +    notifyPeerEvent: function notifyPeerEvent(target, event, sessionToken) {
> > +      if (!target) {
> > +        dump("invalid target");
> 
> debug

It is my intention to use dump since callers have checked 'target' before calling this.
However if target is still null/undefined when this function is called, error will be thrown.

Comment 8

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #7)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> > > +    notifyPeerEvent: function notifyPeerEvent(target, event, sessionToken) {
> > > +      if (!target) {
> > > +        dump("invalid target");
> > 
> > debug
> 
> It is my intention to use dump since callers have checked 'target' before
> calling this.
> However if target is still null/undefined when this function is called,
> error will be thrown.

ok!

Comment 9

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

Attachment: Part 2: add removeTarget

When fixing removePeerTarget also found targetsByRequestId also has this problem

Comment 10

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

Try:
https://tbpl.mozilla.org/?tree=Try&rev=9f85a646153c

Comment 11

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

add checkin-needed for b2g-inbound is close.

Comment 12

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

Attachment: Part 3: fix tests case.

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8482653 [details] [diff] [review]
Part 3: fix tests case.

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

::: dom/nfc/tests/marionette/test_nfc_error_messages.js
@@ +101,5 @@
>      nfc.onpeerready = null;
>      deferred.resolve();
>    };
>  
> +  nfc.notifyUserAcceptedP2P(MANIFEST_URL);

Just curious about removing checkP2PRegistration here because of |this.currentPeerAppId| is removed 
so we don't have to call checkP2PRegistration anymore ?

@@ +106,4 @@
>    return deferred.promise;
>  }
>  
> +function sendNDEFExpectError(peer) {

Nits. remove caller with errorMsg parameter

Comment 14

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #13)
> Comment on attachment 8482653 [details] [diff] [review]
> Part 3: fix tests case.
> 
> Review of attachment 8482653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/tests/marionette/test_nfc_error_messages.js
> @@ +101,5 @@
> >      nfc.onpeerready = null;
> >      deferred.resolve();
> >    };
> >  
> > +  nfc.notifyUserAcceptedP2P(MANIFEST_URL);
> 
> Just curious about removing checkP2PRegistration here because of
> |this.currentPeerAppId| is removed 
> so we don't have to call checkP2PRegistration anymore ?

Yeah, remove checkP2PRegistration since now this.currentPeer is set in notifyUserAcceptedP2P.

In the original code you will find that the onpeerlost will be notified only if checkP2PRegistration is called because this.currentPeerAppId is set in checkP2PRegistration.

In this test file we want onpeeready to be called, checkP2PRegitraion should be tests in the file test_nfc_checkP2PRegistation.js

Comment 15

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

Attachment: Part 3: fix tests case. v2

Comment 16

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

https://hg.mozilla.org/integration/b2g-inbound/rev/d7c70397b4e1
https://hg.mozilla.org/integration/b2g-inbound/rev/864b7828b156
https://hg.mozilla.org/integration/b2g-inbound/rev/a834ac4ed741


Try: https://tbpl.mozilla.org/?tree=Try&rev=31b2e3037045

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/d7c70397b4e1
https://hg.mozilla.org/mozilla-central/rev/864b7828b156
https://hg.mozilla.org/mozilla-central/rev/a834ac4ed741

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Note that due to recent policy changes, all patches must have approval for uplift regardless of blocking status. Please request aurora approval on these patches when you get a chance. Sorry for the inconvenience :(

Comment 19

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

Comment on attachment 8482061 [details] [diff] [review]
Part 1: fix onpeerlost.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1043186

[User impact if declined]:
onpeerlost can not be notified, so Gaia apps cannot know the MozNFCPeer is invalid so the object cannot be GCed.

[Describe test coverage new/current, TBPL]:
Marionette-webapi

[Risks and why]: 
No

[String/UUID change made/needed]:
No

Comment 20

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

Comment on attachment 8482102 [details] [diff] [review]
Part 2: add removeTarget

Approval Request Comment
Same with Part 1.

Comment 21

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

Comment on attachment 8483932 [details] [diff] [review]
Part 3: fix tests case. v2

Approval Request Comment
Same with Part1.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34b9f8cfa6aa
https://hg.mozilla.org/releases/mozilla-aurora/rev/16c12e16d4be
https://hg.mozilla.org/releases/mozilla-aurora/rev/885be57b98b7