If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[NFC][Bluetooth] NFC headset auto connect always failed when user manually disconnect already connected NFC headset

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Alison Shiue, Assigned: arno)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S10 (17apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8582168 [details]
flame_22.log

Gaia-Rev        e54c4ed1cc188f70ddf1156534d364005dc45490
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
Build-ID        20150323162503
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150323.200543
FW-Date         Mon Mar 23 20:05:54 EDT 2015
Bootloader      L1TC100118D0

STR:
1. Enable NFC on test device
2. Tap NFC headset on device
3. Show connect confirmed dialog and device click yes to connect
4. After NFC headset paired and connected, user manually disconnect NFC headset 
5. Tap NFC headset on device again 
6. Show connect confirmed dialog and device click yes to connect

Expected result:
NFC headset auto connect again.

Actual result:
Could not auto connect to NFC headset.
Shawn has some finding on bug 1141899 Comment 16.
(Reporter)

Updated

3 years ago
blocking-b2g: --- → 2.2?
QA Whiteboard: [COM=NFC]
On flame-kk, can be paired and connect with the remote device at the first
time. If the device get paired previously but disconnected, re-connection
trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
trying to do pairing again even device is in the paired list and I don't see
NfcHandoverManager creates connection.
ni? for Arno for NfcHandoverManager
Blocks: 933640
Flags: needinfo?(arno)
triage: function not working
blocking-b2g: 2.2? → 2.2+
(Assignee)

Comment 4

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #1)
> On flame-kk, can be paired and connect with the remote device at the first
> time. If the device get paired previously but disconnected, re-connection
> trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
> trying to do pairing again even device is in the paired list and I don't see
> NfcHandoverManager creates connection.

NfcHandoverManager will not check if the device is already paired. This has been the behavior since the first version and the pre-KK BT stack seemed to be lenient about this. However, NfcHandoverManager will always connect after pairing:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L337

If you don't see the connect(), onsuccess() of the pair() request was not called. Will onerror() be called if the device is already paired? That would explain the missing connect().

Since this worked pre-KK, how do you want to handle this? Do you want to guard against multiple pairing on the BT layer or do you want me to check for this in NfcHandoverManager? If the latter, I guess I still need to do a connect() for already-paired devices, right?
Flags: needinfo?(arno) → needinfo?(shuang)
(In reply to arno from comment #4)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #1)
> > On flame-kk, can be paired and connect with the remote device at the first
> > time. If the device get paired previously but disconnected, re-connection
> > trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
> > trying to do pairing again even device is in the paired list and I don't see
> > NfcHandoverManager creates connection.
> 
> NfcHandoverManager will not check if the device is already paired. This has
> been the behavior since the first version and the pre-KK BT stack seemed to
> be lenient about this. However, NfcHandoverManager will always connect after
> pairing:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> nfc_handover_manager.js#L337
> 
> If you don't see the connect(), onsuccess() of the pair() request was not
> called. Will onerror() be called if the device is already paired? That would
> explain the missing connect().
> 
> Since this worked pre-KK, how do you want to handle this? Do you want to
> guard against multiple pairing on the BT layer or do you want me to check
> for this in NfcHandoverManager? If the latter, I guess I still need to do a
> connect() for already-paired devices, right?

I think you can check paired devices before you re-pair again. Similar code: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L532

I have not got much time to dig into the real problem yet. I expect i will do it on Friday.
Flags: needinfo?(shuang)
A soft reminder that feature complete day will be April6. 
Let's try to fix before that.
Flags: needinfo?(shuang)
Flags: needinfo?(arno)

Comment 7

3 years ago
Created attachment 8585817 [details] [review]
[gaia] svic:Bug_1146744_v2.2 > mozilla-b2g:v2.2
(Assignee)

Comment 8

3 years ago
Created attachment 8585818 [details] [diff] [review]
Don't call pair() on an already paired BT device (v2.2)
Flags: needinfo?(arno)
Attachment #8585818 - Flags: review?(alive)
Hi arno, can you take the bug since you have submitted a patch for review?
Flags: needinfo?(arno)
(Assignee)

Updated

3 years ago
Assignee: nobody → arno
Flags: needinfo?(arno)
Attachment #8585818 - Attachment is patch: true
Attachment #8585818 - Attachment mime type: text/x-github-pull-request → text/plain
Flags: needinfo?(shuang)
See Also: → bug 1146741
See Also: bug 1146741bug 1141899
Comment on attachment 8585818 [details] [diff] [review]
Don't call pair() on an already paired BT device (v2.2)

See nits before you land.
Attachment #8585818 - Flags: review?(alive) → review+
(Assignee)

Comment 11

3 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> Comment on attachment 8585818 [details] [diff] [review]
> Don't call pair() on an already paired BT device (v2.2)
> 
> See nits before you land.

Thanks, Alive. I'll use promises for the master-branch patch.
(Assignee)

Comment 12

3 years ago
Created attachment 8586116 [details] [review]
Don't call pair() on an already paired BT device (v2.2) (r=alive)

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: there is a hard JS error if the user tries to pair an already paired device.
[Testing completed]: Tests on Flame device and added new unit tests
[Risk to taking this patch] (and alternatives if risky): Minimal
[String changes made]: none
Attachment #8585817 - Attachment is obsolete: true
Attachment #8585818 - Attachment is obsolete: true
Attachment #8586116 - Flags: approval-gaia-v2.2?
waiting for mater landing before considering the 2.2 approval request.

Comment 14

3 years ago
Created attachment 8586536 [details] [review]
[gaia] svic:Bug_1146744 > mozilla-b2g:master
(Assignee)

Comment 15

3 years ago
Created attachment 8586545 [details] [review]
Don't call pair() on an already paired BT device (master)

Alive: here is the patch for master. As you will see, I didn't use Promise. I do have a version with Promise that works fine on a device, but I was not able to get the unit tests to work. Unfortunately I am running out of time. The sheriff will only accept the critical v2.2 patch when the patch for master has landed. If you agree, I will open a new bug to use Promises in NfcHandoverManager that I will work on when I have more time.
Attachment #8586545 - Flags: review?(alive)
Comment on attachment 8586545 [details] [review]
Don't call pair() on an already paired BT device (master)

Ya, you will have unit test problem :)

The keypoint is you will need to test the result in the returning another promise:
test('find paired device', function(done) {
  subject.findPairedDevice(address).then(function() {
    // test success stuff
  });
});

And test the one who call it by mock promise:
test('something called paired device', function() {
  this.sinon.stub(subject, 'findPairedDevice').returns(MockPromise);
  subject.someMethodWouldCallGetPairedDevice();
  MockPromise.mResolve();
  // Test the content in success callback is done
  MockPromise.mReject();
  // Test the content in fail callback is done
});

If this does not work, maybe we should return another problem in the method who calls findPairedDevice and test it using done().
subject.someMethodWouldCallGetPairedDevice().then(function() {
  // assert
  done();
});
Attachment #8586545 - Flags: review?(alive) → review+
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Comment 17

3 years ago
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
(Assignee)

Comment 18

3 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #16)
>
> Ya, you will have unit test problem :)

Thanks for the suggestions. Introducing that one Promise broke over half of the unit tests for NfcHandoverManager because findPairedDevice() was on the execution path. I was trying to find a way without MockPromise but ultimately failed. I guess your suggestion of stubbing findPairedDevice() is the only way, although this will get quickly very painful if more promises are used. It is too bad that internal implementation details under test need to be exposed in the test case itself. Might it be possible to do something similar to sinon.seFakeTimers() that makes handling of Promises more deterministic?
alive/arno can you help per : https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c13
Flags: needinfo?(arno)
Flags: needinfo?(alive)
(Assignee)

Comment 20

3 years ago
Vincent, Wesley: what needs to be done here? The autolander rejects the CI flag.
Flags: needinfo?(whuang)
Flags: needinfo?(vchang)
Flags: needinfo?(arno)
I think the problem in bug 1146220 had already being resolved. Clear ni.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> I think the problem in bug 1146220 had already being resolved. Clear ni.

we still need to land this in master.
similar case as https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c19
I'm changing the component to gaia:system
Component: NFC → Gaia::System
Flags: needinfo?(whuang)
Flags: needinfo?(vchang)

Comment 24

3 years ago
Created attachment 8589853 [details] [review]
[gaia] KevinGrandon:svic-Bug_1146744 > mozilla-b2g:master
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #23)
> similar case as https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c19
> I'm changing the component to gaia:system

Yes, this should work with the new component set, but unfortunately the linters are failing.

I've submitted a new PR to rollup a linter fix with the original patch. Assuming the linter and tests pass, I think we can land this. Thanks!
Comment on attachment 8586545 [details] [review]
Don't call pair() on an already paired BT device (master)

Obsoleting in favor of: https://github.com/mozilla-b2g/gaia/pull/29401
Attachment #8586545 - Attachment is obsolete: true
Comment on attachment 8586536 [details] [review]
[gaia] svic:Bug_1146744 > mozilla-b2g:master

Obsoleting in favor of: https://github.com/mozilla-b2g/gaia/pull/29401
Attachment #8586536 - Attachment is obsolete: true
Comment on attachment 8589853 [details] [review]
[gaia] KevinGrandon:svic-Bug_1146744 > mozilla-b2g:master

Carrying review from Alive earlier.
Attachment #8589853 - Flags: review+
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Comment 29

3 years ago
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/f2226cc3002f26bd481190f73c970e34af2d534c

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Attachment #8586116 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/0bfb78bb850cd2f47e97a6107d2ace5d21efbac3
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S10 (17apr)
(Reporter)

Updated

3 years ago
Blocks: 1154136
(Reporter)

Updated

3 years ago
QA Whiteboard: [COM=NFC]
See Also: → bug 1154136
See Also: → bug 1090799
You need to log in before you can comment on or make changes to this bug.