1181827 - [Aries-l][Bluetooth] Pairing to another device will show 'Unknown Device' on the pair request screen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150708_1540.txt

Description:
When utilizing an Aries flashed to a Lollipop Spark, the user will observe that pairing two phones together through bluetooth will display 'Unknown Device' as the opposite devices name. This will pair the device titled as 'Unknown Device' under the "Paired Devices" tab. When the user attempts a bluetooth transfer, the true name will be revealed.

PreReq:
* another phone
Repro Steps:
1) Update a Aries to 20150707205829 [Lollipop]
2) Enable bluetooth
3) Change bluetooth name to "pirate" : opposite device to "viking"
4) Set both to visible, and set to pair both phones to eachother through bluetooth.
5) Observe pair request form and device name attempting to pair

Actual:
Name displays as 'Unknown Device'

Expected:
Name displays as 'viking' [what device was set in step 3]


Environmental Variables:
-------------------------------

Device: Aries 2.5 [Lollipop]
Build ID: 20150707205829
Gaia: 553e4e281d11500bea702918841f011bfb5405f7
Gecko: 0246d86e619670bf381d45d587e8ab036e7a8db5
Version: 42.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0
*****************

Issue DOES NOT REPRODUCE on Aries Spark on Kitkat devices
Results: Name displays as 'viking' [what device was set in step 3]

Device: Aries 2.5 [Kitkat]
BuildID: 20150708014729
Gaia: 938375d0abafd92d78b3913db1735e49f46a6598
Gecko: d6fb92164021
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0
------------------------------------

Repro frequency: 9/10
See attached: 
video- https://youtu.be/HOjb2pt9gJo
logcat

Comment 1

[Oliver Nelson [:oliverthor]]

This occurs on the earliest Lollipop, but not on KitKat. So this is a regression from the transition from Kitkat to Lollipop.

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
UX regression that is of a security concern (knowing who you're pairing with).

Comment 3

[Ben Tian (inactive)]

Oliver,

Can you specify task ID of the build you use? I cannot find your build on task cluster [1] based on your info.

[1] https://tools.taskcluster.net/index/#gecko.taskcluster/gecko.taskcluster

Comment 4

[Oliver Nelson [:oliverthor]]

This was a custom build given to my team for testing Lollipop on the Aries device, provided by Naoki.

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It's not a taskcluster build.  I didn't have the time to get the taskcluster to have Aries-L build as we were to do this task in 2 days.

I made the build locally.  What information do you need?
The build can be found here : https://drive.google.com/a/mozilla.com/file/d/0B_0LdM1CVycIeUxEOUxCYjVickk/view?usp=sharing
[Mozilla Only Share]

Ben are you taking it?  Or is Jocelyn?

Comment 6

[Ben Tian (inactive)]

I'll take this bug.

I can reproduce this bug with the latest m-c. Logs show stack passes device name to pair from |RemoveDevicePropertiesNotification| instead of |SspRequestNotification|. Will revise the flow accordingly and do some tests.

Comment 7

[Ben Tian (inactive)]

Update:

I'm revising pairing flow to adapt the L change, but problem in bug 1163550 comment 76 keeps disturbing me even I've applied patch in bug 1163550 comment 86. Still working to fix the problem for development.

Comment 8

[Aaron Wu]

mark as 2.5+

Comment 9

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Store all devices' address and name mapping for pairing

The cause is that bluedroid on aries-L notifies pairing device name from |RemoveDevicePropertiesNotification| instead of |SspRequestNotification|.

The patch fixes the bug by remembering the latest address and name mapping of all devices, and query device name during pairing/paired/unpaired. Note the whole mapping is only cleared when bluetooth is disabled.

Specifically, the mapping is updated in
- |DeviceFoundNotifcation|
- |RemoteDevicePropertiesNotification|
- |SspRequestNotification|/|PinRequestNotifcation|
  (if notification passes non-empty device name)

And device name is queried in
- |SspRequestNotification|/|PinRequestNotifcation|
  (if notification passes empty device name)
- |BondStateChangedNotification|.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The code in BluetoothServiceBluedroid.cpp is already hairy and the name is just one of several properties. Could this problem be handled in the Settings app instead?

Comment 11

[Ben Tian (inactive)]

Thomas,

It's do-able but we have to revise API since current API defines to carry device name in pairing event [1].

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#deviceName

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Ben,

I see. Thanks for the info. Just some more general thoughts:

Maybe such fields should generally be optional (i.e, null if not available). I remember that we had a similar problem with device-scanning back in the BlueZ 4 days on ICS and we sometimes had to update the name later on. In such cases, there's not much we can do.

I think we need to find a way to simplify the code of the BluetoothService. The current approach of global variables and arrays doesn't scale. Some of the notification handlers are getting unmaintainable.

Maybe a better integration of adapters and devices could help. For example, BluetoothService could have a list of adapters, each with a list of devices. When we receive a message from Bluedroid, we'd look for the appropriate receiver and let it handle the message's content.

Comment 13

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> Maybe such fields should generally be optional (i.e, null if not available).
> I remember that we had a similar problem with device-scanning back in the
> BlueZ 4 days on ICS and we sometimes had to update the name later on. In
> such cases, there's not much we can do.
Yes, it's fine if BluetoothService hasn't got it. But in pairing cases BluetoothService always receives the update before notifying gaia.
 
> Maybe a better integration of adapters and devices could help. For example,
> BluetoothService could have a list of adapters,
Any possible usage for this list? I haven't thought of purpose for it.  

> each with a list of devices.
It's already there in current code, the adapter.mDevices member variable. The way to append/remove BluetoothDevice to/from the array is in [1]
[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.h#383

> When we receive a message from Bluedroid, we'd look for the appropriate
> receiver and let it handle the message's content.
It works if the adapter and device are both created at the time BluetoothServervice receives device name update. However in fact the adapter may not be created then, or the BluetoothDevice hasn't been created by adapter to receive property change (e.g., incoming pairing instead of outgoing pairing triggered on discovery screen).

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

My point was just that we should give some more structure to the stored data, instead of putting everything into global variables. A hierarchy of adapters and devices might be helpful. I don't have a full proposal for such a change, though.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8637020 [details] [diff] [review]
Patch 1 (v1): Store all devices' address and name mapping for pairing

Review of attachment 8637020 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this bug. Overall the idea is good to go. However I have one question regarding update mapping from DeviceFoundNotification.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2424,5 @@
>      }
>    }
>  
> +  // Update <address, name> mapping
> +  sDeviceNameMap.Remove(bdAddr);

Check index bdAddr in|sDeviceNameMap| first before removing item.

@@ +2425,5 @@
>    }
>  
> +  // Update <address, name> mapping
> +  sDeviceNameMap.Remove(bdAddr);
> +  sDeviceNameMap.Put(bdAddr, bdName);

Is it possible that DeviceFound reports empty name? Like we used to see 'Unknown device'? Shall we validate bdName (I have a bit concern to update empty name again)?

@@ +2526,5 @@
> +  nsString bdName(aBdName);
> +  if (bdName.IsEmpty()) {
> +    sDeviceNameMap.Get(bdAddr, &bdName);
> +  } else {
> +    sDeviceNameMap.Remove(bdAddr);

Ditto.

Comment 16

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> My point was just that we should give some more structure to the stored
> data, instead of putting everything into global variables. A hierarchy of
> adapters and devices might be helpful. I don't have a full proposal for such
> a change, though.

Agree. Just have to think thoroughly for all possible cases, especially because we have no control on Bluedroid that changes the flow sometimes.

Comment 17

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +2424,5 @@
> >      }
> >    }
> >  
> > +  // Update <address, name> mapping
> > +  sDeviceNameMap.Remove(bdAddr);
> 
> Check index bdAddr in|sDeviceNameMap| first before removing item.

The |Remove| does nothing if the entry of key |bdAddr| doesn't exist [4] so I didn't check it. Note [4] is called from [1]->[2]->[3]->[4]. 

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsBaseHashtable.h#148
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTHashtable.h#172
[3] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.cpp#720
[4] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.cpp#678

> @@ +2425,5 @@
> >    }
> >  
> > +  // Update <address, name> mapping
> > +  sDeviceNameMap.Remove(bdAddr);
> > +  sDeviceNameMap.Put(bdAddr, bdName);
> 
> Is it possible that DeviceFound reports empty name? Like we used to see
> 'Unknown device'? Shall we validate bdName (I have a bit concern to update
> empty name again)?

We can validate but cannot tell whether the latest device name is really empty string or not. I prefer to remember the latest name from stack, no matter what it is.

Comment 18

[Ben Tian (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c109dd1e6ce

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a3274d86bf00

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3274d86bf00

Comment 21

[John Thomas [:Johnt]]

This issue is verified fixed for Aries Master.
Result: Paired devices shows the correct name of device accordingly.

Environmental Variables:
Device: Aries 2.5
BuildID: 20151029161147
Gaia: 91cac94948094cfdcd00cba5c6483e27e80cb3b0
Gecko: ac68828c5e3e2ac4fabcfde859440a749e0f5fbf
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 45.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Comment 22

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue was for Aries Lolipop.  Resetting it to resolved instead of verified.