1127665 - [Bluetooth][API v2] The property 'name' of a paired device is not existed while this device just be paired.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[[e-mail], [e-mail]]

STD:

1. Launch settings app and go into Bluetooth panel.
2. Turn Bluetooth on and try to pair with a remote device.
3. After pairing process completely, checking the name on the paired device list.

Expected:
The name of paired device should be show normally. Or the name should be updated quickly.

Actual:
It shows 'Unknown device'.

Comment 1

[[e-mail], [e-mail]]

====== Log =======:
I/Settings( 1932):     at btc__refreshPairedDevicesInfo (app://settings.gaiamobile.org/js/modules/bluetooth/bluetooth_context.js:418:10)

Comment 2

[Ben Tian (inactive)]

Jamin will take a look first. Assign owner to him.

Comment 3

[Ben Tian (inactive)]

Current implementation return |adapter.getPairedDevices| with adapter's cached devices array that may contain no device name. bluetooth1 adopts DOMRequest to retrieve the latest properties of remote devices from stack.

Possible fix is to 1) make |adapter.getPairedDevices| async call as bluetooth1 or 2) revise update of adapter's cached devices array to get the latest properties.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Append name of paired device to 'DevicePaired' BT signal (v1)

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8569748 [details] [diff] [review]
Append name of paired device to 'DevicePaired' BT signal (v1)

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

Please see my comment below.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1674,5 @@
>  
> +  // Retrieve device name from hash table of pairing device name and append it
> +  // to the properties array.
> +  if (bonded) {
> +    nsString deviceName;

Move this section into https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#1642

--
Also suggest to revise as following:

  nsString deviceName = EmptyString();
  if (/* hashtable.get() */) {
    ...
  }
  BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);

@@ +1679,5 @@
> +    if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
> +      if (aStatus == STATUS_SUCCESS) {
> +        BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
> +      }
> +      sPairingNameTable.Remove(aRemoteBdAddr);

When to remove the entry if pairing fails (i.e., bonded = false)?

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Append name of paired device to 'DevicePaired' BT signal (v2)

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Append name of paired device to 'DevicePaired' BT signal (v3)

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 8569748 [details] [diff] [review]
> Append name of paired device to 'DevicePaired' BT signal (v1)
> 
> Review of attachment 8569748 [details] [diff] [review]:
> @@ +1679,5 @@
> > +    if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
> > +      if (aStatus == STATUS_SUCCESS) {
> > +        BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
> > +      }
> > +      sPairingNameTable.Remove(aRemoteBdAddr);
> 
> When to remove the entry if pairing fails (i.e., bonded = false)?

#attachment 8569748 [details] [diff] [review] can't handle pairing fails correctly.
Thank you for pointing it out to me.

I've uploaded a new patch #attachment 8571228 [details] [diff] [review] to fix this issue.
Thanks.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8571228 [details] [diff] [review]
Append name of paired device to 'DevicePaired' BT signal (v3)

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

Please see my comment below.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1665,5 @@
>    BT_APPEND_NAMED_VALUE(propertiesArray, "Address", nsString(aRemoteBdAddr));
>    BT_APPEND_NAMED_VALUE(propertiesArray, "Paired", bonded);
>  
> +  // Retrieve device name from hash table of pairing device name.
> +  nsString deviceName = EmptyString();

Remove the empty string assignment since |deviceName| is used only when it exists in hash table.

@@ +1670,5 @@
> +  if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
> +    sPairingNameTable.Remove(aRemoteBdAddr);
> +    if (bonded) {
> +      // Append BD name to signal properties if the device is paired.
> +      BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);

We should also append device name into device's "PropertyChanged" signal in case the device created in "DeviceFound" doesn't get name before pairing. We may revise appending order to reuse the property array.

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Append name of paired device to 'DevicePaired' and 'PropertyChanged' BT signal (v4)

- Refine previous patch based on comment 9.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8571709 [details] [diff] [review]
Append name of paired device to 'DevicePaired' and 'PropertyChanged' BT signal (v4)

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

r=me with comment addressed.

::: dom/bluetooth2/BluetoothCommon.h
@@ +88,5 @@
> + * Wrap literal name and value into a BluetoothNamedValue
> + * and insert it to the array.
> + */
> +#define BT_INSERT_NAMED_VALUE(array, index, name, value)               \
> +  array.InsertElementAt(index, BluetoothNamedValue(NS_LITERAL_STRING(name),   \

nit: align the two '\'

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1651,5 @@
>    } else {
>      sAdapterBondedAddressArray.RemoveElement(remoteBdAddr);
>    }
>  
>    // Update attribute BluetoothDevice.paired

Revise comment:
  // Update BluetoothDevice attributes: paired and name

@@ +1657,5 @@
>    BT_APPEND_NAMED_VALUE(propertiesArray, "Paired", bonded);
>  
> +  // Retrieve device name from hash table of pairing device name.
> +  nsString deviceName;
> +  if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {

Replace with MOZ_ASSERT here since |BluetoothAdatper::HandleDevicePaired| does so.

==
  bool nameExists = sPairingNameTable.Get(aRemoteBdAddr, &deviceName);
  MOZ_ASSERT(nameExists);

  sPairingNameTable.Remove(aRemoteBdAddr);
  if (bonded) {
    // Append BD name to signal properties if the device is paired.
    BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
  }

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Append name of paired device to 'DevicePaired' and 'PropertyChanged' BT signal (v5)

- Refine previous patch based on #comment 11

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

No try server link since this patch is made for BlueDroid and tinderbox use BlueZ as hardware stack.

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

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

Thank Ben for reviewing the patch.

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

There is a defect in #attachment 8571821 [details] [diff] [review].
I'll upload another patch to fix it today.

Sorry for the inconvenience caused

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v1)

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8571858 [details] [diff] [review]
Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v1)

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

r=me with comment addressed.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1659,3 @@
>    // Retrieve device name from hash table of pairing device name.
>    nsString deviceName = EmptyString();
>    bool nameExists = sPairingNameTable.Get(aRemoteBdAddr, &deviceName);

Remove |nameExists| as following:

  nsString deviceName = EmptyString();
  if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
    sPairingNameTable.Remove(aRemoteBdAddr);
  }

  if (bonded && aStatus == STATUS_SUCCESS) {
    MOZ_ASSERT(!deviceName.IsEmpty());
    BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
  }

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83f27620ef8e

Comment 19

[Ben Tian (inactive)]

One more thing: please also assert deviceName is not empty in [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#900

(In reply to Ben Tian [:btian] from comment #17)
> Remove |nameExists| as following:
> 
>   nsString deviceName = EmptyString();
>   if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
>     sPairingNameTable.Remove(aRemoteBdAddr);
>   }
> 
>   if (bonded && aStatus == STATUS_SUCCESS) {
>     MOZ_ASSERT(!deviceName.IsEmpty());
>     BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
>   }

Comment 20

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #19)
> One more thing: please also assert deviceName is not empty in [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/
> BluetoothAdapter.cpp#900
> 
> (In reply to Ben Tian [:btian] from comment #17)
> > Remove |nameExists| as following:
> > 
> >   nsString deviceName = EmptyString();
> >   if (sPairingNameTable.Get(aRemoteBdAddr, &deviceName)) {
> >     sPairingNameTable.Remove(aRemoteBdAddr);
> >   }
> > 
> >   if (bonded && aStatus == STATUS_SUCCESS) {
> >     MOZ_ASSERT(!deviceName.IsEmpty());
> >     BT_APPEND_NAMED_VALUE(propertiesArray, "Name", deviceName);
> >   }

A valid Bluetooth name is a UTF-8 encoding string which is up to 248 bytes in length.
Some Bluetooth devices might allow users to set device name to an empty string.

Therefore, I prefer to remove the MOZ_ASSERT statement in BluetoothAdapter::HandleDevicePaired().
I'll upload a new patch, please feel free to leave comments.

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v2)

Comment 22

[Ben Tian (inactive)]

Comment on attachment 8572488 [details] [diff] [review]
Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v2)

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ -896,5 @@
>    MOZ_ASSERT(arr.Length() == 3 &&
>               arr[0].value().type() == BluetoothValue::TnsString && // Address
>               arr[1].value().type() == BluetoothValue::Tbool &&     // Paired
>               arr[2].value().type() == BluetoothValue::TnsString);  // Name
> -  MOZ_ASSERT(!arr[0].value().get_nsString().IsEmpty() &&

Keep this assertion since it checks not name but address.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1668,5 @@
>      MOZ_ASSERT(nameExists);
> +
> +    // According to Bluetooth Core Spec. v3.0 - Sec. 6.22, a valid Bluetooth
> +    // name is a UTF-8 encoding string which is up to 248 bytes in length.
> +    // e.g. An empty string is also a valid name.

Revise comment to mention first thing first.

  // We don't assert |!deviceName.IsEmpty()| here since empty string is also a valid name.
  // According to ...

Comment 23

[Ben Tian (inactive)]

Comment on attachment 8572488 [details] [diff] [review]
Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v2)

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

r=me with comment 22 addressed.

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the condition of if-statement of name updating when the Bluetoooth device paired. (v3), r=btian

- revise previous patch based on #comment 23

Comment 25

[Jamin Liu [:jaliu][:Jamin]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32369c2ab5c5

Comment 27

[Jamin Liu [:jaliu][:Jamin]]

- Remove the 'leave-open' keyword I added before.
- Mark as RESOLVED FIXED since all patches have landed.