1121404 - [Bluetooth][API v2] API adapter.getPairedDevices() return incorrect number of paired devices.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Since implementation of pairing process via WIP of bug 1119734, I meet a bug about incorrect number of paired device from API adapter.getPairedDevices().

STR:

0. There is no paired device.
1. Open settings
2. Pair with one remote device
3. Adapter.getPairedDevices() will return two paired devices.

4. Close settings app
5. Open settings app
6. Adapter.getPairedDevices() will return no paired device.

Comment 1

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

============= log ==============

/Settings( 2594): Content JS LOG: --> pairedDevices[0].address = b8:d9:ce:a1:46:e5 
I/Settings( 2594):     at btc__refreshPairedDevicesInfo/< (app://settings.gaiamobile.org/js/modules/bluetooth/bluetooth_context.js:450:0)
I/Settings( 2594): Content JS LOG: --> pairedDevices[1].address = b8:d9:ce:a1:46:e5 
I/Settings( 2594):     at btc__refreshPairedDevicesInfo/< (app://settings.gaiamobile.org/js/modules/bluetooth/bluetooth_context.js:450:0)

Comment 2

[Ben Tian (inactive)]

Update:

BluetoothAdapter doesn't filter out device with duplicate address in |mDevices| array [1]. I'm checking the correctness of using BluetoothDevice operator == [2].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#893
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothDevice.h#100

Comment 3

[[e-mail]]

I found a reproducible step might related to this issue: 

* open uitest app/HW/bluetooth
* enable the bluetooth button

Actual:
Can't get bluetooth address

Expect:
can get bluetooth address

The above step was worked with APIv2.

Comment 4

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

(In reply to Fred Lin [:gasolin] from comment #3)
> I found a reproducible step might related to this issue: 
> 
> * open uitest app/HW/bluetooth
> * enable the bluetooth button
> 
> Actual:
> Can't get bluetooth address
Thank you for reporting this issue.
I'll trace this issue with high priority.

Comment 5

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

Attachment: Cache addresses of bonded devices and notify adapter properly. (v1)

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8551118 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v1)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothDevice.h
@@ +191,5 @@
> + * Explicit Specialization of Function Templates
> + *
> + * Allows customizing the template code for a given set of template arguments.
> + * With this function template, nsTArray can handle comparison of
> + * '<nsRefPtr<BluetoothDevice>' properly, including IndexOf() and Contains();

nit: extra <

@@ +194,5 @@
> + * With this function template, nsTArray can handle comparison of
> + * '<nsRefPtr<BluetoothDevice>' properly, including IndexOf() and Contains();
> + */
> +template <>
> +class nsDefaultComparator <nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>,

Suggest to move this comparator into BluetoothAdapter since it's only used there. Also remove operator== of BluetoothDevice since it's unused.

@@ +196,5 @@
> + */
> +template <>
> +class nsDefaultComparator <nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>,
> +                           nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>> {
> +  public:

nit: no indent for |public:| and only two-space indention required for the following.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +62,5 @@
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
> +
> +// InfallibleTArray is an alias for nsTArray.
> +static InfallibleTArray<nsString> sAdapterBondedAddressArray;

nit: suggest to move the two lines after

  static bool sAdapterEnabled(false);

to keep original grouping.

@@ +1195,5 @@
>      if (sAdapterDiscovering) {
>        sAdapterDiscovering = false;
>        BT_APPEND_NAMED_VALUE(props, "Discovering", false);
>      }
>  

nit: remove newline here.

@@ +1196,5 @@
>        sAdapterDiscovering = false;
>        BT_APPEND_NAMED_VALUE(props, "Discovering", false);
>      }
>  
> +    if (!sAdapterBondedAddressArray.Length()) {

Should be |!sAdapterBondedAddressArray.IsEmpty()|.

@@ +1197,5 @@
>        BT_APPEND_NAMED_VALUE(props, "Discovering", false);
>      }
>  
> +    if (!sAdapterBondedAddressArray.Length()) {
> +      BT_APPEND_NAMED_VALUE(props, "PairedDevices", InfallibleTArray<nsString>());

nit: this line exceeds 80 characters.

@@ +1290,5 @@
>                p.mStringArray.Length());
>  
> +      // Whenever reloading paired devices, force refresh
> +      sAdapterBondedAddressArray.Clear();
> +

nit: remove newline here.

@@ +1297,3 @@
>        }
>  
> +      BT_APPEND_NAMED_VALUE(propertiesArray, "PairedDevices", sAdapterBondedAddressArray);

nit: this line exceeds 80 characters.

@@ +1604,5 @@
>      DispatchBluetoothReply(sBondingRunnableArray[0],
>                             BluetoothValue(true), EmptyString());
>      sBondingRunnableArray.RemoveElementAt(0);
>  
> +    if (!sAdapterBondedAddressArray.Contains(remoteBdAddr)) {

Update |sAdapterBondedAddressArray| no matter |sBondingRunnableArray| is empty or not.

Comment 7

[Ben Tian (inactive)]

Forgot to mention one thing: is it better to compare BluetoothDevice and address instead of two BluetoothDevices? We may save the creation of the temporary BluetoothDevice object for comparison.

(In reply to Ben Tian [:btian] from comment #6)
> Comment on attachment 8551118 [details] [diff] [review]
> Cache addresses of bonded devices and notify adapter properly. (v1)
> 
> Review of attachment 8551118 [details] [diff] [review]:

Comment 8

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

Attachment: Cache addresses of bonded devices and notify adapter properly. (v2)

Comment 9

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

(In reply to Ben Tian [:btian] from comment #6)
> Comment on attachment 8551118 [details] [diff] [review]
> Cache addresses of bonded devices and notify adapter properly. (v1)
> 
> @@ +194,5 @@
> > + * With this function template, nsTArray can handle comparison of
> > + * '<nsRefPtr<BluetoothDevice>' properly, including IndexOf() and Contains();
> > + */
> > +template <>
> > +class nsDefaultComparator <nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>,
> 
> Suggest to move this comparator into BluetoothAdapter since it's only used
> there.
Personally, I prefer to put the specialization of function template in BluetoothDevice.h since it might be used in other file.

The customized template affects nsDefaultComparator only if developer includes 'BluetoothDevice.h' and tries to compare two nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>. In that case, it reasonable to use customized template rather than the original nsDefaultComparator.

Would it be OK for you to keep function template in BluetoothDevice.h ? Or do you feel it's improper ?
These choices are open to be discussed. :)
Please feel few to leave more comments.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8551635 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v2)

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

Please see my comment below. Also please add device paired & unpaired cases into https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.h#336

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +280,5 @@
>        = value.get_ArrayOfnsString();
>  
>      for (uint32_t i = 0; i < pairedDeviceAddresses.Length(); i++) {
> +      // Use customized function template of nsDefaultComparator to check
> +      // whether or not the address exists in mDevices.

Minor suggestion: Reorder comment to mention first what's doing here. Also nsDefaultComparator can be omitted since it's irrelevant to code flow here.

  // Check whether or not the address exists in mDevices

and all the following.

@@ +840,5 @@
>    MOZ_ASSERT(mDiscoveryHandleInUse);
>    MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
>  
> +  const nsString& deviceAddress = RetrieveAddressFromBtValue(aValue);
> +  nsRefPtr<BluetoothDevice> discoveredDevice = nullptr;

Add NS_ENSURE_FALSE_VOID(deviceAddress.IsEmpty()) to handle empty device address.

@@ +849,2 @@
>    if (index == mDevices.NoIndex) {
>      // New device, append it to adapter's device array

Revise comment:

  // Create a new device and append it to adapter's device array

@@ +893,5 @@
>      return;
>    }
>  
> +  const nsString& deviceAddress = RetrieveAddressFromBtValue(aValue);
> +  nsRefPtr<BluetoothDevice> pairedDevice = nullptr;

Add NS_ENSURE_FALSE_VOID(deviceAddress.IsEmpty()) to handle empty device address.

@@ +902,2 @@
>    if (index == mDevices.NoIndex) {
> +    // New device, append it to adapter's device array

Ditto. Revise comment.

@@ +923,5 @@
>      BT_WARNING("HandleDeviceUnpaired() is called when adapter isn't enabled.");
>      return;
>    }
>  
> +  const nsString& deviceAddress = RetrieveAddressFromBtValue(aValue);

Add NS_ENSURE_FALSE_VOID(deviceAddress.IsEmpty()) to handle empty device address.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +290,5 @@
> +  /**
> +   * Retrieve BD address from a BluetoothValue which contains named value
> +   * "Address".
> +   *
> +   * The main purpose of this function is to retrieve addres from bluetooth

typo: addres's'

@@ +295,5 @@
> +   * signal "DeviceFound", DEVICE_PAIRED_ID and DEVICE_UNPAIRED_ID.
> +   *
> +   * @return address of remote Bluetooth device.
> +   */
> +  nsString RetrieveAddressFromBtValue(const BluetoothValue& aValue);

Suggest to remove this function and assign address directly as [1].
[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothPairingListener.cpp#99

::: dom/bluetooth2/BluetoothDevice.h
@@ +181,5 @@
> + * Explicit Specialization of Function Templates
> + *
> + * Allows customizing the template code for a given set of template arguments.
> + * With this function template, nsTArray can handle comparison between
> + * '<nsRefPtr<BluetoothDevice>>' and nsString properly, including IndexOf() and

Remove outer <>. Should be 'nsRefPtr<BluetoothDevice>'.

@@ +188,5 @@
> +template <>
> +class nsDefaultComparator <nsRefPtr<mozilla::dom::bluetooth::BluetoothDevice>,
> +                           nsString> {
> +public:
> +

nit: remove this newline.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +60,5 @@
>  USING_BLUETOOTH_NAMESPACE
>  
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
> +

nit: remove this newline.

@@ +65,4 @@
>  static bool sAdapterDiscoverable(false);
>  static bool sAdapterDiscovering(false);
>  static bool sAdapterEnabled(false);
>  

nit: remove this newline.

Comment 11

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

Attachment: Cache addresses of bonded devices and notify adapter properly. (v3)

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8554137 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v3)

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

Please see my comment below.

* Again please add device paired & unpaired cases into https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.h#336 *

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +294,5 @@
>        // Create paired device with 'address' and 'paired' attributes
>        nsRefPtr<BluetoothDevice> pairedDevice =
>          BluetoothDevice::Create(GetOwner(), BluetoothValue(props));
>  
>        // Append to adapter's device array if the device hasn't been created

Remove 'if the device hasn't been created' from comment.

@@ +838,5 @@
>  {
>    MOZ_ASSERT(mDiscoveryHandleInUse);
>    MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
>  
> +  // Retrieve remote BD address from BluetoothValue

Add another default comparator for two nsRefPtr<BluetoothDevice> to save the search. In this case comparator of two devices seems more convenient. I think we can event leverage comparator of nsRefPtr<BluetoothDevice> and nsString in the additional comparator.

@@ +912,4 @@
>  
> +  nsString deviceAddress = arr[0].value().get_nsString();
> +
> +  NS_ENSURE_FALSE_VOID(deviceAddress.IsEmpty());

After consideration I think making the check MOZ_ASSRT is more reasonable since it shouldn't happen. Also revise in |HandleDeviceUnpaired|.

  MOZ_ASSERT(arr.Length() == 2 &&
             arr[0].value().type() == BluetoothValue::TnsString && // Address
             arr[1].value().type() == BluetoothValue::Tbool);      // Paired
  MOZ_ASSERT(!arr[0].value().get_nsString().IsEmpty() &&
             arr[1].value().get_bool());

  nsString deviceAddress = arr[0].value().get_nsString();

@@ +951,3 @@
>  
> +  nsString deviceAddress = arr[0].value().get_nsString();
> +

nit: remove this newline.

@@ +955,5 @@
> +
> +  // Use customized function template of nsDefaultComparator to check
> +  // whether or not the address exists in mDevices.
> +  size_t index = mDevices.IndexOf(deviceAddress);
> +  if (index != mDevices.NoIndex) {

Call |mDevices.RemoveElement(deviceAddress)| directly. |RemoveElement| would check the existence of |deviceAddress|.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1416

Comment 13

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

Attachment: Cache addresses of bonded devices and notify adapter properly. (v4)

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8556311 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v4)

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

r=me with comment addressed.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +941,4 @@
>  
> +  nsString deviceAddress = arr[0].value().get_nsString();
> +
> +  // Remove the device with same address

'the' same address

::: dom/bluetooth2/BluetoothDevice.h
@@ +181,5 @@
> + * Explicit Specialization of Function Templates
> + *
> + * Allows customizing the template code for a given set of template arguments.
> + * With this function template, nsTArray can handle comparison of
> + * '<nsRefPtr<BluetoothDevice>' properly, including IndexOf() and Contains();

comparison of 'nsRefPtr<BluetoothDevice>'

Comment 15

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

Attachment: Cache addresses of bonded devices and notify adapter properly. (v5), r=btian

- Revise previous patch based on #comment 14
- Add reviewer's name to commit message

Thank Ben for reviewing the patch.

Comment 16

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

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

Comment 17

[Carsten Book [:Tomcat]]

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