Closed Bug 1039239 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.1 S1 (1aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: ashiue, Assigned: arno)

References

Details

Attachments

(4 files, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #1031993 +++
Hi Arno,

I tried on
Gaia      5f8b1b8a2da9e3b531eee817a669f57fa4d9b9c6
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/913827496f65
BuildID   20140716000201
Version   32.0a2

the following problems still occur:

When quick leave 2 phones after swipe shrinking UI, sender does not show error  notification.
And then share again after turn off BT on both sides, the receiver side always throw "[JavaScript Error: "Unable to create NFCPeer object" error unless reboot device.
blocking-b2g: --- → 2.0?
Flags: needinfo?(arno)
Assignee: nobody → arno
Flags: needinfo?(arno)
Attached patch Add try-block for getNFCPeer() (obsolete) — Splinter Review
That is a little embarrassing, but there is a second location in the handover manager that calls getNFCPeer() that is not protected by a try-block. That should be the problem you encounter. Allison: very nice job on the testing! I'm currently traveling and don't have my Flame device with me, however, I did create a patch. If you have the cycles, perhaps you can run your test with this patch considering time is short. I will be back to SF tomorrow and will create a PR.
Flags: needinfo?(ashiue)
Attachment #8456879 - Attachment is obsolete: true
Arno, QA cannot apply your patch directly.

Due to this is going to be a 2.0+ bug,
I'll help to send a PR and send r? to Alive for Arno.

Arno, when you see this please obselte mine and create your own PR.
Flags: needinfo?(ashiue)
I wrote a test case as well in the afternoon. I'll create a PR once I'm back at the hotel. 

Yoshi: since I don't have a Flame device with me, could you please flash a version based on the patch I attached so that Allison can do some testing even before I create the PR? TIA
I just sent a PR now and Alison could help to verify this on master.
https://github.com/mozilla-b2g/gaia/pull/21845
I've seen several errors like this._debug is not defined.
Arno can you check this?
I created a PR that includes a unit test. The errors with this._debug() is probably because of a missing bind() (which I fixed in this PR).
Attachment #8457642 - Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
with your latest PR, there's always a dialog pop up on receiver side, 
"File could not be received" before the receiving BT File request.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh] from comment #8)
> with your latest PR, there's always a dialog pop up on receiver side, 
> "File could not be received" before the receiving BT File request.
The sender side also has the "File could not be sent" dialog.
QA Whiteboard: [COM=NFC]
blocking-b2g: 2.0? → 2.0+
I managed to test on the Flame device and fixed the issues that Yoshi mentioned. Also added more unit tests.
Attachment #8457779 - Attachment is obsolete: true
Attachment #8458282 - Flags: review?(alive)
Comment on attachment 8458282 [details] [review]
Add try-block for getNFCPeer() and set timeouts

Please don't use timer in test.

After visiting the js file again, to be honest, I have no idea how to review it.
* Many function is defined again and again inside function scope but unnecessary.
* Some function should be bind to self/this but it didn't. I really suspect how it works.
https://github.com/svic/gaia/blob/Bug_1039239/apps/system/js/nfc_handover_manager.js#L540
in self._doPairing, this is not self.

The readability is really sad so I could not approve it. Please ask evelyn to review it.
Attachment #8458282 - Flags: review?(alive)
Also I really wonder why we need to use try catch here.
Why not use dom request to implement it?

An error being thrown means something really bad happens.
There is no denying that the NfcHandoverManager needs a makeover. Regarding Alive's comments made in comment 11 and comment 12:

* we will remote setTimeout in the test.
* the correct function context will be used when receiving the bluetooth-adapter-event:
  https://github.com/svic/gaia/blob/Bug_1039239/apps/system/js/nfc_handover_manager.js#L174
  That should be more resilient than expecting callers to call bind()
* getNFCPeer() throws an exception in case of an error. The API was designed (and presumably r+'ed) this way.
Attached file pull-request-1039239-v2.txt (obsolete) —
Arno is currently travelling. I'm attaching a 2.0 version of the patch, this is his code with minor fixes for v2.0 branch made by me. I have removed the tests using setTimeout. The main goal of this patch is to solve the problem raised by Alison. Issues listed by Alive in comment 11 we will address in future bugs in master branch.

Travis failed in unrelated test. Evelyn could you review this?
Attachment #8458811 - Flags: review?(ehung)
(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #14)
> Created attachment 8458811 [details] [review]
> pull-request-1039239-v2.txt
> 
> Arno is currently travelling. I'm attaching a 2.0 version of the patch, 

You need to create a patch for master first, once it's r+, then create another PR for 2.0 branch.
Comment on attachment 8458811 [details] [review]
pull-request-1039239-v2.txt

After talking with Evelyn off line, this patch should be reviewed by Alive.
Attachment #8458811 - Flags: review?(ehung) → review?(alive)
(In reply to arno from comment #13)
> There is no denying that the NfcHandoverManager needs a makeover. Regarding
> Alive's comments made in comment 11 and comment 12:
> 
> * we will remote setTimeout in the test.
> * the correct function context will be used when receiving the
> bluetooth-adapter-event:
>  
> https://github.com/svic/gaia/blob/Bug_1039239/apps/system/js/
> nfc_handover_manager.js#L174
>   That should be more resilient than expecting callers to call bind()
> * getNFCPeer() throws an exception in case of an error. The API was designed
> (and presumably r+'ed) this way.

Could you tell me who reviews it?
If any error needs to be thrown then website could easily blocks the phone.

After reading the whole I really this is gecko issue. Please fix it in gecko side.
Do not throw error unless it's really something wrong.
Comment on attachment 8458811 [details] [review]
pull-request-1039239-v2.txt

Sorry not reviewing this because I am not convinced why gaia should always give a try-catch in order not to block the phone. We should not design the error handling API like this.
Attachment #8458811 - Flags: review?(alive)
Yoshi, I wonder if we could handle this bug in gecko?
Flags: needinfo?(allstars.chh)
(In reply to Ken Chang[:ken] from comment #19)
> Yoshi, I wonder if we could handle this bug in gecko?

A couple of quick comments: getNFCPeer() needs to be able to report an error condition back to the caller and can't be handled transparently in Gecko. The caller (in this case NfcHandoverManager) needs to be able to do its own cleanup (which is what this bug is all about). Also note that getNFCPeer() is an API used by any sharing app. If the API changes, most if not all sharing apps would need to be adapted. The easiest way to get rid of the exception is to have getNFCPeer() return null. The other alternative is a DOMRequest but that feels a bit like overkill and would require even more pervasive changes to all sharing apps.
No, this is a Gaia bug as I've mentioned clearly in https://bugzilla.mozilla.org/show_bug.cgi?id=1031993#c3.

The problem of this bug and why it's 2.0+ is if user turned off the BT then did the file sharing through NFC, it will NEVER succeed.

Whether the getNFCPeer call should throw or not should be another question I'd suggest we disucss this in another bug.

And if the reviewer thinks Gaia shouldn't do try-catch here I'd also suggest refactor the patch in this bug and the already landed patch in Bug 1031993 to avoid the try-catch pattern.
Flags: needinfo?(allstars.chh)
Flags: needinfo?(alive)
Yes, of course it is a Gaia problem because NfcHandoverManager didn't do cleanup. That is the root cause of Bug 1031993 as well as this one. But I can only handle the error condition if we agree on how getNFCPeer() returns an error. I'm just pointing out that if we change the API, all sharing apps will most likely to be changed as well.
Flags: needinfo?(alive)
(In reply to arno from comment #22)
> That is the root cause of Bug 1031993 as well as this one. 

I don't think that's the root-cause.
I think the problem should be in 

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L157
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L158

for (var i = 0; i < self.actionQueue.length; i++) {
  var action = self.actionQueue[i];
  action.callback.apply(self, action.args);
}

if action.callback throws, why is it still left in the queue ?
(In reply to Yoshi Huang[:allstars.chh] from comment #23)
> (In reply to arno from comment #22)
> > That is the root cause of Bug 1031993 as well as this one. 
> 
> I don't think that's the root-cause.
What I meant here is even the two phones are close and both are in proximinty, however if BT is off, this problem will still occur 100%, which I think is the major problem.
(In reply to Yoshi Huang[:allstars.chh] from comment #23)
> (In reply to arno from comment #22)
> > That is the root cause of Bug 1031993 as well as this one. 
> 
> I don't think that's the root-cause.
> I think the problem should be in 
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> nfc_handover_manager.js#L157
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> nfc_handover_manager.js#L158
> 
> for (var i = 0; i < self.actionQueue.length; i++) {
>   var action = self.actionQueue[i];
>   action.callback.apply(self, action.args);
> }
> 
> if action.callback throws, why is it still left in the queue ?

I explain this here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1031993#c7
Attached file PR to fix the queue problem (obsolete) —
I have a patch to fix the throwing callback will always reside in the actionQueue.
Attachment #8460057 - Flags: review?(alive)
(In reply to Yoshi Huang[:allstars.chh] from comment #26)
> Created attachment 8460057 [details] [review]
> PR to fix the queue problem
> 
> I have a patch to fix the throwing callback will always reside in the
> actionQueue.

A quick comment on your patch: all the actions that get executed in the for-loop expect BT to be on (the whole point of actionQueue is to queue up actions while BT is being turned on). In the catch-block of your patch you restore the BT status (possibly turning BT off) but then continue the for-loop. This will disrupt whatever actions will execute next.
(In reply to arno from comment #27)
> (In reply to Yoshi Huang[:allstars.chh] from comment #26)
> > Created attachment 8460057 [details] [review]
> > PR to fix the queue problem
> > 
> > I have a patch to fix the throwing callback will always reside in the
> > actionQueue.
> 
> A quick comment on your patch: all the actions that get executed in the
> for-loop expect BT to be on (the whole point of actionQueue is to queue up
> actions while BT is being turned on). In the catch-block of your patch you
> restore the BT status (possibly turning BT off) but then continue the
> for-loop. This will disrupt whatever actions will execute next.

Thanks, I just moved the try-block out of the while loop.
(In reply to Yoshi Huang[:allstars.chh] from comment #28)
> (In reply to arno from comment #27)
> > (In reply to Yoshi Huang[:allstars.chh] from comment #26)
> > > Created attachment 8460057 [details] [review]
> > > PR to fix the queue problem
> > > 
> > > I have a patch to fix the throwing callback will always reside in the
> > > actionQueue.
> > 
> > A quick comment on your patch: all the actions that get executed in the
> > for-loop expect BT to be on (the whole point of actionQueue is to queue up
> > actions while BT is being turned on). In the catch-block of your patch you
> > restore the BT status (possibly turning BT off) but then continue the
> > for-loop. This will disrupt whatever actions will execute next.
> 
> Thanks, I just moved the try-block out of the while loop.

Then you should also keep the actionQueue = [] at the end otherwise you have some orphaned actions. Quite frankly, if an exception ever makes it to that level, it will wreak havoc on the internal state of NfcHandoverManager. Besides turning off BT there are other cleanups that need to happen depending on the context.
Comment on attachment 8458282 [details] [review]
Add try-block for getNFCPeer() and set timeouts

Fixed the timer in the unit tests.
Attachment #8458282 - Flags: review?(alive)
Attachment #8460057 - Attachment is obsolete: true
Attachment #8460057 - Flags: review?(alive)
Attachment #8458811 - Attachment is obsolete: true
Attachment #8458282 - Flags: review?(alive) → review+
Travis failure is unrelated to this PR.
Keywords: checkin-needed
Attached file PR for 2.0
Travis build is still in progress. Not sure if it's needed.
Master: https://github.com/mozilla-b2g/gaia/commit/5910d8e3f2d64a8a23806c1fbc8ba51e735b3424
v2.0:   https://github.com/mozilla-b2g/gaia/commit/77386dfd41a2126713d3dc45d8ed298ebf2a4b3e
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)
Verify on
Gaia      91986777d0942b63e37fbfeec19d69d6176d6d74
Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/392054b2e899
BuildID   20140723160206
Version   32.0
Status: RESOLVED → VERIFIED
This bug has been verified to fail on Flame 2.1.
Issue steps:
Premise:Two device have turned on NFC: device A and device B.
1.Launch Gallery from device A.
2.Tap an image to view.
3.Put Device A‘s back close to Device B's back. 
4.When quick leave 2 phones after swipe shrinking UI.

Actual result:
4.The device A can't pop up the " File could not be sent" prompt box.

Expect result:
4.The device A should pop up the " File could not be sent" prompt box.

Note:And then share again after turn off BT on both sides, the receiver receive the file successfully, and open the file normally.

See attachment: 1452.MP4 and logcat_1452.txt
Reproducing rate: 2/5

Flame 2.1 version:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880
Flags: needinfo?(mlien)
Attached video 1452.MP4
Verified again with today's v2.1 gaia/gecko, it cannot be reproduced

Gaia-Rev        c226db212db4d824c09617cd6dc407b2d4258d9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cf8bebfa4703
Build-ID        20141209170126
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141209.212104
FW-Date         Tue Dec  9 21:21:15 EST 2014
Bootloader      L1TC10011880
Flags: needinfo?(mlien)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: