Closed Bug 1183496 Opened 7 years ago Closed 7 years ago

[nfc handover] adapter not found when pairing

Categories

(Firefox OS Graveyard :: Gaia::Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.5+, b2g-master fixed)

VERIFIED FIXED
FxOS-S4 (07Aug)
blocking-b2g 2.5+
Tracking Status
b2g-master --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

Attachments

(2 files)

on flame L & flame kk, restart the device, use nfc headset to pair with device will have highly chance to monitor `(bluetooth) adapter not found` error log and no pairing dialog is shown.

After disable/enable bluetooth, the dialog works normally.

I think it might be a 2.5 regression of bug 1094759, which might make nfc core modules loaded before bluetooth modules.
Blocks: 1172823
Currently we only save the adapter when we receive `bluetooth-enabled` event. But with current bootstrap procedure, the NFC handover module usually start after bluetooth module, and the event is missed.

If the root cause is right, `adapter not found` will only happens when NFC & Bluetooth are default enabled after reboot. 

If we toggle Bluetooth enabled or make Bluetooth enabled automatically, the issue will not happen.
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
[Blocking Requested - why for this release]: It breaks the NFC transfer
blocking-b2g: --- → 2.5?
QA Whiteboard: [COM=NFC]
marked as 2.5+
blocking-b2g: 2.5? → 2.5+
Comment on attachment 8633996 [details] [review]
[gaia] gasolin:issue-1183496 > mozilla-b2g:master

@tauzen could you help review it?
Attachment #8633996 - Flags: review?(kmioduszewski)
Comment on attachment 8633996 [details] [review]
[gaia] gasolin:issue-1183496 > mozilla-b2g:master

Hi Fred, this looks good to me. I've left some small comments on github. 
I'm setting f+ and forwarding the review to Tim. I'm leaving the project, so I think I shouldn't review this.
Attachment #8633996 - Flags: review?(timdream)
Attachment #8633996 - Flags: review?(kmioduszewski)
Attachment #8633996 - Flags: feedback+
Thanks for feedback and very appreciate for your contribution! 

I've addressed your comment and would ask Tim to review this issue.
Comment on attachment 8633996 [details] [review]
[gaia] gasolin:issue-1183496 > mozilla-b2g:master

I don't believe this is a correct fix. This behaves correctly just because of the behavior of Bluetooth#adapter() method.

Could we instead always make sure there is an adopter when handling incoming NFC events? The current design is very fragile against state disparity and races like this.

If you can't do it or if I don't read the code correctly, I am happy to review this band-aid patch again.
Attachment #8633996 - Flags: review?(timdream) → review-
Fred do you still need a regression-window for this issue?  You seem to have an idea of what caused this and what is needed to fix it.

Also am I correct in understanding that when this issue occurs the NFC sharing screen will close to the homescreen?
Flags: needinfo?(gasolin)
Thanks, no need regression-window here. 

The issue occurs after shrinking UI, the transfer notification will not triggered due to no bluetooth adapter.
Flags: needinfo?(gasolin)
Comment on attachment 8636386 [details] [review]
[gaia] gasolin:issue-1183496-2 > mozilla-b2g:master

Always make sure there is an adopter when handling incoming NFC events. Now all _doXxxx methods are triggered after _doAction, which will request the BT adapter before execute those methods.

Test with BT on/off after restart and works well.
Attachment #8636386 - Flags: review?(timdream)
Removing the regression window tag per comment 10.
QA Whiteboard: [COM=NFC] → [COM=NFC][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [COM=NFC][QAnalyst-Triage?] → [COM=NFC][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment on attachment 8636386 [details] [review]
[gaia] gasolin:issue-1183496-2 > mozilla-b2g:master

Since you are changing the behavior of _doAction, please ensure this is what other methods are expecting as well.
Attachment #8636386 - Flags: review?(timdream) → review+
Thanks for review. I'll ask Alison for help to make sure origin operations going well in case I miss anything.
Alison found some issues after applying the patch. I'll trace it later after fixed other issues.
yoshi, during testing NFC on 8/5 flame pvt img,

Sending file When NFC on and BT on or off, sometimes I got logs (monitored via `adb logcat | grep "Nfc"` command):

D/nfcd    ( 3145): void NfcIpcSocket::Loop(const char*, bool):  116 of bytes to be sent... data=0x0 ret=0
D/NfcNci  ( 3145): void RouteDataSet::DeleteDatabase(): default db size=0; sec elem db size=0

After these logs, the device will turn off the screen and auto reboot. Is it a known issue?


Here is the more detailed log:

I/BrcmNfcNfa( 3145): LLCP_SendData () Local SAP:0x20, Remote SAP:0x10
D/NfcNci  ( 3145): static void PeerToPeer::NfaClientCallback(tNFA_P2P_EVT, tNFA_P2P_EVT_DATA*): enter; event=7
D/NfcNci  ( 3145): static void PeerToPeer::NfaClientCallback(tNFA_P2P_EVT, tNFA_P2P_EVT_DATA*): NFA_P2P_DATA_EVT; h=0x581; remote sap=0x10
I/BrcmNfcNfa( 3145): NFA_P2pReadData (): handle:0x581
I/BrcmNfcNfa( 3145): LLCP_ReadDataLinkData () Local SAP:0x20, Remote SAP:0x10
D/NfcNci  ( 3145): bool PeerToPeer::Receive(unsigned int, uint8_t*, uint16_t, uint16_t&): exit; nfa h: 0x581  ok: 1  actual len: 60
D/nfcd    ( 3145): void MessageHandler::ProcessResponse(NfcResponseType, NfcErrorCode, void*): enter response=2, error=0
D/nfcd    ( 3145): void NfcIpcSocket::WriteToOutgoingQueue(uint8_t*, size_t): enter, data=0xb76e8ec8, dataLen=16
D/nfcd    ( 3145): void NfcIpcSocket::WriteToOutgoingQueue(uint8_t*, size_t): Writing 16 bytes to gecko
D/nfcd    ( 3145): void* NfcService::EventLoop(): NFCService msg=14
D/nfcd    ( 3145): void MessageHandler::ProcessNotification(NfcNotificationType, void*): processNotificaton notification=4
D/nfcd    ( 3145): void NfcIpcSocket::WriteToOutgoingQueue(uint8_t*, size_t): enter, data=0xb76e8eb8, dataLen=112
D/nfcd    ( 3145): void NfcIpcSocket::WriteToOutgoingQueue(uint8_t*, size_t): Writing 112 bytes to gecko
D/nfcd    ( 3145): void NfcIpcSocket::Loop(const char*, bool):  116 of bytes to be sent... data=0x0 ret=0
D/NfcNci  ( 3145): void RouteDataSet::DeleteDatabase(): default db size=0; sec elem db size=0
Flags: needinfo?(allstars.chh)
@alison I've re-testing between android z3c and 8/5 flame pvt img (Base img is 18Dv4),
with NFC on + BT on/off, for receive and send cases. 

Everything works except above crash (It happens during transferring so it might not related to gaia). 
Could you help verify again?


BTW, Shawn Huang told me that we should flash full pvt img after flash the Base img, so the nfc/bluetooth gonk & gecko daemon will match with each other.
Flags: needinfo?(ashiue)
See Also: → 1191715
Flags: needinfo?(allstars.chh)
comment 17 is tracked in bug 1191715
I tried with bt on/off combinations, file transfer works fine.

test build:

[Flame]
Build ID               20150805150207
Gaia Revision          581de383687dc441a878d2c91a0167c6ec688fef
Gaia Date              2015-08-05 01:48:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/b12a261ee32e
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150805.195702
Firmware Date          Wed Aug  5 19:57:12 EDT 2015
Bootloader             L1TC000118D0

[Z3C]
Build ID               20150806003241
Gaia Revision          581de383687dc441a878d2c91a0167c6ec688fef
Gaia Date              2015-08-05 01:48:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9801f91760d9
Gecko Version          42.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150805.235801
Firmware Date          Wed Aug  5 23:58:08 UTC 2015
Bootloader             s1
Flags: needinfo?(ashiue)
Thanks alison,

merged to master since this patch does solve the problem https://github.com/mozilla-b2g/gaia/commit/cfbfe8e2df1ecd7710d41125bd0d2ba7f36db3af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
According to Comment 20, change the status to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.