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

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: iliu@mozilla.com, ianliu.moz@gmail.com, Assigned: jaliu)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webbt-api])

Attachments

(1 attachment, 4 obsolete attachments)

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.
============= 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)

Updated

3 years ago
Whiteboard: [webbt-api]

Comment 2

3 years ago
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
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 3

3 years ago
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.
Blocks: 1093084
(Assignee)

Comment 4

3 years ago
(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.

Updated

3 years ago
No longer blocks: 1093084
(Assignee)

Comment 5

3 years ago
Created attachment 8551118 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v1)
Attachment #8551118 - Flags: review?(btian)

Comment 6

3 years ago
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.
Attachment #8551118 - Flags: review?(btian)
(Assignee)

Updated

3 years ago
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM

Comment 7

3 years ago
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]:
(Assignee)

Comment 8

3 years ago
Created attachment 8551635 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v2)
Attachment #8551118 - Attachment is obsolete: true
Attachment #8551635 - Flags: review?(btian)
(Assignee)

Comment 9

3 years ago
(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

3 years ago
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.
Attachment #8551635 - Flags: review?(btian)
(Assignee)

Comment 11

3 years ago
Created attachment 8554137 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v3)
Attachment #8551635 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8554137 - Flags: review?(btian)

Comment 12

3 years ago
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
Attachment #8554137 - Flags: review?(btian)
(Assignee)

Comment 13

3 years ago
Created attachment 8556311 [details] [diff] [review]
Cache addresses of bonded devices and notify adapter properly. (v4)
Attachment #8554137 - Attachment is obsolete: true
Attachment #8556311 - Flags: review?(btian)

Comment 14

3 years ago
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>'
Attachment #8556311 - Flags: review?(btian) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8556334 [details] [diff] [review]
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.
Attachment #8556311 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/a72324bb5f94
https://hg.mozilla.org/mozilla-central/rev/a72324bb5f94
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.