Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Description • 10 years ago

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

	Ken Chang[:kenkai]
	Comment 1 • 10 years ago

Hi Arno, We need your help for this bug.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 3 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 4 • 10 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 5 • 10 years ago

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

	arno Assignee
	Comment 6 • 10 years ago

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

	arno Assignee
	Comment 7 • 10 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 8 • 10 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 9 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 10 • 10 years ago

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

	arno Assignee
	Comment 11 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 12 • 10 years ago

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

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

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 13 • 10 years ago

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

See github, and ask gweng@mozilla.com to review again.
Thank you.

	arno Assignee
	Comment 14 • 10 years ago

Fixed Alive's github comments.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 15 • 10 years ago

Comment on attachment 8453856 [details]
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.

	arno Assignee
	Comment 16 • 10 years ago

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

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 17 • 10 years ago

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

	arno Assignee
	Comment 18 • 10 years ago

Make lint happy.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 19 • 10 years ago

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

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 20 • 10 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 21 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

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