1031993 - NFC - JavaScript Error: "Unable to create NFCPeer object"

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

STR:
1. Turn off BT in receiver side.
2. On sender side, using Shrinking-UI to do the file transfer.
3. Take two devices away.

Expected Result:
If the Handover-Request/Select is interrupted (the other phone is taken away), we should pop up the error to notify the user.

Actual Result:
When the Handover is interrupted, no error notification will be shown.


The receiver side tries to respond Handover Select, but the sender side is already taken away, so when calling nfc.getNFCPeer(session), it will throw "JavaScript Error: "Unable to create NFCPeer object", but the error is not shown so user cannot know the handover procedure is actually failed.

See the log from 
https://bugzilla.mozilla.org/show_bug.cgi?id=1029303#c4

Comment 1

[Ken Chang[:kenkai]]

Hi Arno, We need your help for this bug.

Comment 2

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

Attachment: sender.log

Comment 3

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

In Comment 2 I uploaded the log captured from the sender with debug enabled from Gecko NFC part, nfc_manager.js, and nfc_handover_manager.js.

STR:
Sender device disables BT and enables NFC.
Tapping another device
After swiping, take the two phones away immediately. 

From the log, line 6556
E/GeckoConsole(  290): [JavaScript Error: "Unable to create NFCPeer object, Reason:  Bad SessionToken {58e7f7a8-beba-4dac-af9f-719850b3101c}" {file: "jar:file:///system/b2g/omni.ja!/components/nsNfc.js" line: 200}]

This seems normal since after line 6383 the session is already invalid.

However now disable the BT again, and tap the two phones together.
New session starts from line 6687, with session id {9b8f6924-291e-46c2-adab-506d5283ace6}

Then do the swipe the share the data,
in line 8065, NFCPeer.sendFile is called, then the message is broadcasted to nfc_handover_manager.js again, and it will try to enable BT again.

line 9054 the BT is enabled, now nfc_handover_manager will call initiateFileTransfer.
However the arguments for this function, is WRONG.

From log line 9096, we will see it tries to getNFCPeer with old session info.
I/Gecko   (  290): -*- Nfc: Received 'NFC:SetSessionToken' message from content process
I/Gecko   (  290): -*- Nfc: Received invalid Session Token: {58e7f7a8-beba-4dac-af9f-719850b3101c} - Do not register this target
E/GeckoConsole(  290): [JavaScript Error: "Unable to create NFCPeer object, Reason:  Bad SessionToken {58e7f7a8-beba-4dac-af9f-719850b3101c}" {file: "jar:file:///system/b2g/omni.ja!/components/nsNfc.js" line: 200}]
E/GeckoConsole(  290): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "app://system.gaiamobile.org/js/nfc_handover_manager.js" line: 249}]

Note the session now should be {9b8f6924-291e-46c2-adab-506d5283ace6}, instead of {58e7f7a8-beba-4dac-af9f-719850b3101c}.

I think there's a bug in maintaining the 'actionQueue', if one action throws error, it's never deleted, so when the actionQueue is popping up again, always the oldest one will be executed, and throwing error again.

Nominate this to 2.0+ for this also happens on 2.0 branch.

Comment 4

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

I think there should be two problems here,
1. We should notify the user if the other device is already away, otherwise user will keep waiting however nothing will happen, as mentioned Comment 0.
2. Fix the actionQueue bug mentioned in Comment 3

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: 2.0+ because this breaks user experience. user would get confused what goes wrong.

Comment 6

[arno]

(In reply to Yoshi Huang[:allstars.chh] from comment #4)
> I think there should be two problems here,
> 1. We should notify the user if the other device is already away, otherwise
> user will keep waiting however nothing will happen, as mentioned Comment 0.

I left a NI for Juwei in bug 894320 to clarify what the UI should look like.

> 2. Fix the actionQueue bug mentioned in Comment 3

I will look into this.

Comment 7

[arno]

(In reply to Yoshi Huang[:allstars.chh] from comment #4)
> 2. Fix the actionQueue bug mentioned in Comment 3

The problem is not with actionQueue but sendFileQueue. What happened is that NfcHandoverManager uses getNFCPeer(). If the peer already went away (e.g., because you immediately remove the other device) it will throw an exception. Since there is no try-block to catch that exception, no proper cleanup happens and for that reason BT is not turned off again and there is an orphaned request in sendFileQueue. I will fix it and the notification but since the patch depends on Bug 998175 I will wait for the latter to land before submitting a patch for review.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to arno from comment #6)
> (In reply to Yoshi Huang[:allstars.chh] from comment #4)
> > I think there should be two problems here,
> > 1. We should notify the user if the other device is already away, otherwise
> > user will keep waiting however nothing will happen, as mentioned Comment 0.
> 
> I left a NI for Juwei in bug 894320 to clarify what the UI should look like.
> 
Reply by Juwei is here:
https://bugzilla.mozilla.org/show_bug.cgi?id=894320#c35
> > 2. Fix the actionQueue bug mentioned in Comment 3
> 
> I will look into this.

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Regarding schedule, 2.0 blocker should be fixed before end of last sprint.

Comment 10

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

(In reply to Yoshi Huang[:allstars.chh] from comment #4)
> I think there should be two problems here,
> 1. We should notify the user if the other device is already away, otherwise
> user will keep waiting however nothing will happen, as mentioned Comment 0.
> 2. Fix the actionQueue bug mentioned in Comment 3

Comment 4 is made before this bug is nominated to 2.0+,
given that this is a 2.0+ bug now, we could fix the 2nd problem (actionQueue) first, and file another bug for the first.

Comment 11

[arno]

Attachment: Fix getNFCPeer() bug and show notification when transfer fails

Yoshi: I was already sitting on a patch that addresses both issues you have raised. Let me know what you think.

Comment 12

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

Comment on attachment 8453083 [details] [review]
Fix getNFCPeer() bug and show notification when transfer fails

It's in gaia part so forward r? to Alive.

Comment 13

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

Comment on attachment 8453083 [details] [review]
Fix getNFCPeer() bug and show notification when transfer fails

See github, and ask [e-mail] to review again.
Thank you.

Comment 14

[arno]

Attachment: Fix getNFCPeer() bug and show notification when transfer fails

Fixed Alive's github comments.

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8453856 [details] [review]
Fix getNFCPeer() bug and show notification when transfer fails

It's without what Alive's addressed. Unfortunately, the linter error occurs. Please fix it and set the flag again.

Comment 16

[arno]

I just double checked and I did address all of Alive's comments. Can you please tell me which one I did not fix?

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, sorry, typo. What I mean is you fixed all that Alive commented, but it failed to pass linter check:

https://tbpl.mozilla.org/?rev=16dd81f7c1c345a567783582c0e7701eaf9c7f87&tree=Gaia-Try

and Travis linter check:

https://travis-ci.org/mozilla-b2g/gaia/builds/29617692

Comment 18

[arno]

Attachment: Fix getNFCPeer() bug and show notification when transfer fails

Make lint happy.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8455305 [details] [review]
Fix getNFCPeer() bug and show notification when transfer fails

Okay, it passed all tests and code is okay now, thanks.

Comment 20

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

https://github.com/mozilla-b2g/gaia/commit/c5167d0b1964fb055ad0b985b281f76b9e305219

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

just a reminder, besides patch for master we'll need the patch for 2.0 as this is 2.0+.

Comment 22

[arno]

Attachment: PR for the v2.0 branch

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/391258012d09cc1dde9a6f3f9a2078739043d0d6