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)
Tracking
(blocking-b2g:2.0+, 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.
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 2.0?
Updated•10 years ago
|
Flags: needinfo?(arno)
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)
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.
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [COM=NFC]
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
Comment on attachment 8458282 [details] 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)
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
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] > 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 16•10 years ago
|
||
Comment on attachment 8458811 [details]
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)
Comment 17•10 years ago
|
||
(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 18•10 years ago
|
||
Comment on attachment 8458811 [details]
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)
Comment 19•10 years ago
|
||
Yoshi, I wonder if we could handle this bug in gecko?
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(alive)
Assignee | ||
Comment 22•10 years ago
|
||
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.
Updated•10 years ago
|
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.
Assignee | ||
Comment 25•10 years ago
|
||
(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
I have a patch to fix the throwing callback will always reside in the actionQueue.
Attachment #8460057 -
Flags: review?(alive)
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh] from comment #26) > Created attachment 8460057 [details] > 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] > > 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.
Assignee | ||
Comment 29•10 years ago
|
||
(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] > > > 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.
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8458282 [details]
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)
Updated•10 years ago
|
Attachment #8458811 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8458282 -
Flags: review?(alive) → review+
Keywords: checkin-needed
Comment 33•10 years ago
|
||
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
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)
Reporter | ||
Comment 35•10 years ago
|
||
Verify on Gaia 91986777d0942b63e37fbfeec19d69d6176d6d74 Gecko https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/392054b2e899 BuildID 20140723160206 Version 32.0
Status: RESOLVED → VERIFIED
Blocks: 1043856
Comment 36•10 years ago
|
||
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)
Comment 37•10 years ago
|
||
Comment 38•10 years ago
|
||
Updated•10 years ago
|
Updated•10 years ago
|
Comment 39•10 years ago
|
||
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.
Description
•