1207649 - Use |BluetoothAddress| throughout Bluetooth backend interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P2)

Description

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

We currently represent most Bluetooth addresses as strings. This has a number of drawbacks: such as memory and CPU overhead, fragile string parsing, and explicit conversions to binary data in the backend code.

This bug is about moving the string-based address representation from the backend into the Bluetooth mid-layer and profile managers. The long-term perspective is to move address strings into the DOM layer and convert them as early as possible to binary representation.

Comment 1

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

Attachment: [01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code

Comment 2

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

Attachment: [02] Bug 1207649: Convert Bluetooth Core backend to |BluetoothAddress|

Comment 3

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

Attachment: [03] Bug 1207649: Convert Bluetooth Socket backend to |BluetoothAddress|

Comment 4

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

Attachment: [04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress|

Comment 5

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

Attachment: [05] Bug 1207649: Convert Bluetooth A2DP backend to |BluetoothAddress|

Comment 6

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

Attachment: [06] Bug 1207649: Convert Bluetooth AVRCP backend to |BluetoothAddress|

Comment 7

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

Attachment: [07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress|

Comment 8

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

Attachment: [08] Bug 1207649: Remove obsolete string/address conversion from Bluetooth backend

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8664946 [details] [diff] [review]
[04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress|

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

Thomas,

What's the reason to put string/address conversion in hfp manager instead of hfp daemon interface?

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +1002,3 @@
>       */
>  #if ANDROID_VERSION >= 21
> +    rv = UnpackPDU( pdu, aArg2);

nit: remove space after (

Comment 10

[Ben Tian (inactive)]

Set ni? per comment 9.

Comment 11

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

Hi Ben

> 
> What's the reason to put string/address conversion in hfp manager instead of
> hfp daemon interface?

I only briefly outlined this in the bug description. Let me give a more complete explanation.

Using strings for storing most of our data (addresses, UUIDs, pins, etc) is results from the early days of Bluetooth, when it was just a wrapper around BlueZ. BlueZ uses strings in it's interfaces, and the Bluetooth code was built around it.

Compared to binary data structures (|BluetoothAddress|, |BluetoothUuid|, etc), strings have a number of drawbacks, such as

  * memory overhead when storing the data, and
  * CPU overhead for comparing, copying, etc.

And there is also the additional problem of conversion between strings and raw representation. If the string is malformed, the conversion fails. This makes parsing fragile and can easily lead to bugs.

We can fix these problems by using binary data in our mid-layer code and profile managers. The strings that come from the DOM API should be converted as early as possible.

The patch set is not yet the full solution. The purpose of the patch set is to introduce |BluetoothAddress| in the Bluetooth mid-layer and profile managers. A later patch set will further change them to use binary data as well, and move the string conversion into the DOM front-end code. This will simplify many of the current conversion in the mid-layer.

My hope is that we might be able to handle all strings in the App process. When the calls a DOM interface, we'd convert the input strings in the App process. For the rest of the call, we'd use binary data. That would restrict the overhead of the strings to the App process; and if the string parsing/conversion fails, it fails in the (un-privilegued) App process.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8664942 [details] [diff] [review]
[01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code

Forward review to new bluetooth module peer Bruce since he's familiar with daemon related code.

Comment 13

[Bruce Sun [:brsun]]

Comment on attachment 8664942 [details] [diff] [review]
[01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code

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

r=me if the following suggestions would be addressed.

Feel free to share your ideas if some of them don't make sense to you.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +310,5 @@
> +struct BluetoothAddress {
> +
> +  static const BluetoothAddress ANY;
> +  static const BluetoothAddress ALL;
> +  static const BluetoothAddress LOCAL;

From the naming of these three static |BluetoothAddress|, it seems we could use them for bit-wise operation such as AND or OR or even XOR. It would be a very interesting use case to dig into the value of each segment of the Bluetooth address, but I am not sure how we might use them in real scenarios in Gecko. I think it is good to have these predefined constants. Just curious about the usage.

I've noticed that |BluetoothAddress::ANY| is used to reset some |BluetoothAddress| instances, would it be good to have another predefined constant named |BluetoothAddress::EMPTY| (or some other naming) to explicitly describe the cleanup usage of this constant? Although |BluetoothAddress::EMPTY| might have the same value of |BluetoothAddress::ANY| in the memory and seems as a duplicate, I would suggest not to use |BluetoothAddress::ANY| for reset/cleanup purpose. So that the code itself would be more self-explanatory. What do you think?

@@ +314,5 @@
> +  static const BluetoothAddress LOCAL;
> +
> +  uint8_t mAddr[6];
> +
> +  BluetoothAddress() = default;

How about initialize |mAddr| with an empty value or some value that represented as an invalid address? So that we can avoid handling |BluetoothAddress| instances with undefined values in any cases.

::: dom/bluetooth/common/BluetoothHashKeys.h
@@ +53,5 @@
> +                         (static_cast<uint64_t>(aKey->mAddr[1]) << 32) |
> +                         (static_cast<uint64_t>(aKey->mAddr[2]) << 24) |
> +                         (static_cast<uint64_t>(aKey->mAddr[3]) << 16) |
> +                         (static_cast<uint64_t>(aKey->mAddr[4]) << 8) |
> +                         (static_cast<uint64_t>(aKey->mAddr[5]) << 0));

By definition[1], |PLDHashNumber| is a 32-bit unsigned integer. During the conversion from 64-bit to 32-bit, information of higher 32-bit would be always dropped. Probably using |HashKnownLength|[2] or |HashBytes|[3] to hash all bytes would be better in this case.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/PLDHashTable.h#26
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/HashFunctions.h?offset=1100#226
[3] https://dxr.mozilla.org/mozilla-central/source/mfbt/HashFunctions.h?offset=1700#313

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +51,5 @@
> +                   &aAddress.mAddr[2],
> +                   &aAddress.mAddr[3],
> +                   &aAddress.mAddr[4],
> +                   &aAddress.mAddr[5]);
> +  if (res < static_cast<ssize_t>(MOZ_ARRAY_LENGTH(aAddress.mAddr))) {

It is possible that |sscanf| returns |EOF| value. Although |EOF| might be the same as -1 in most cases, it would be good to design our logic without this assumption. Suggest explicitly compare the value of |res| with |EOF| in the condition.

Comment 14

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

(In reply to Bruce Sun [:brsun] from comment #13)
> Comment on attachment 8664942 [details] [diff] [review]
> [01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout
> Bluetooth code
> 
> Review of attachment 8664942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if the following suggestions would be addressed.
> 
> Feel free to share your ideas if some of them don't make sense to you.
> 
> ::: dom/bluetooth/common/BluetoothCommon.h
> @@ +310,5 @@
> > +struct BluetoothAddress {
> > +
> > +  static const BluetoothAddress ANY;
> > +  static const BluetoothAddress ALL;
> > +  static const BluetoothAddress LOCAL;
> 
> From the naming of these three static |BluetoothAddress|, it seems we could
> use them for bit-wise operation such as AND or OR or even XOR. It would be a
> very interesting use case to dig into the value of each segment of the
> Bluetooth address, but I am not sure how we might use them in real scenarios
> in Gecko. I think it is good to have these predefined constants. Just
> curious about the usage.

These constants are defined by BlueZ headers at [1]. We obviously use ANY and I also took the others as I was working on it anyway.

I tried to find the definition in a spec. Bluetooth 4.2 [2] referred to IEEE 802-2001, sec 9.2 [3]. So they are just Ethernet addresses, sort of. I'd welcome pointers to the exact definition of these constants.

> I've noticed that |BluetoothAddress::ANY| is used to reset some
> |BluetoothAddress| instances, would it be good to have another predefined
> constant named |BluetoothAddress::EMPTY| (or some other naming) to
> explicitly describe the cleanup usage of this constant? Although
> |BluetoothAddress::EMPTY| might have the same value of
> |BluetoothAddress::ANY| in the memory and seems as a duplicate, I would
> suggest not to use |BluetoothAddress::ANY| for reset/cleanup purpose. So
> that the code itself would be more self-explanatory. What do you think?

I'd say we should not introduce more constants that are not covered by any specification or related work.

I'm also skeptical about defining bit operators, because one usually cannot apply bit patterns onto network addresses and expect to get meaningful results.

But let's add some helper methods to |BluetoothAddress|. |Clear| could set the address to ANY. We can also add getters and setters for the LAP, UAP and NAP fields of the address. OK?

> 
> @@ +314,5 @@
> > +  static const BluetoothAddress LOCAL;
> > +
> > +  uint8_t mAddr[6];
> > +
> > +  BluetoothAddress() = default;
> 
> How about initialize |mAddr| with an empty value or some value that
> represented as an invalid address? So that we can avoid handling
> |BluetoothAddress| instances with undefined values in any cases.

Makes sense to me.

> ::: dom/bluetooth/common/BluetoothHashKeys.h
> @@ +53,5 @@
> > +                         (static_cast<uint64_t>(aKey->mAddr[1]) << 32) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[2]) << 24) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[3]) << 16) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[4]) << 8) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[5]) << 0));
> 
> By definition[1], |PLDHashNumber| is a 32-bit unsigned integer. During the
> conversion from 64-bit to 32-bit, information of higher 32-bit would be
> always dropped. Probably using |HashKnownLength|[2] or |HashBytes|[3] to
> hash all bytes would be better in this case.

Sure. I wasn't aware of that.

> ::: dom/bluetooth/common/BluetoothUtils.cpp
> @@ +51,5 @@
> > +                   &aAddress.mAddr[2],
> > +                   &aAddress.mAddr[3],
> > +                   &aAddress.mAddr[4],
> > +                   &aAddress.mAddr[5]);
> > +  if (res < static_cast<ssize_t>(MOZ_ARRAY_LENGTH(aAddress.mAddr))) {
> 
> It is possible that |sscanf| returns |EOF| value. Although |EOF| might be
> the same as -1 in most cases, it would be good to design our logic without
> this assumption. Suggest explicitly compare the value of |res| with |EOF| in
> the condition.

Makes sense to me.

[1] https://android.googlesource.com/platform/external/bluetooth/bluez/+/master/lib/bluetooth/bluetooth.h#134
[2] https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=286439
[3] http://www.ieee802.org/secmail/pdfYD89wBpRqH.pdf
[4] https://msdn.microsoft.com/en-us/library/cc510863.aspx

Comment 15

[Bruce Sun [:brsun]]

> I'd say we should not introduce more constants that are not covered by any
> specification or related work.
> 
> I'm also skeptical about defining bit operators, because one usually cannot
> apply bit patterns onto network addresses and expect to get meaningful
> results.
> 
> But let's add some helper methods to |BluetoothAddress|. |Clear| could set
> the address to ANY. We can also add getters and setters for the LAP, UAP and
> NAP fields of the address. OK?

It sounds good.

Comment 16

[Bruce Sun [:brsun]]

Trying to describe more for comment 15.

> |Clear| could set the address to ANY.
This does match what was suggested. Thanks.

> We can also add getters and setters for the LAP, UAP and NAP fields of the address. OK?
It is good to have these implementation in this bug, but it's also OK for me if these are planed to be addressed on other bug.

Comment 17

[Bruce Sun [:brsun]]

Comment on attachment 8664943 [details] [diff] [review]
[02] Bug 1207649: Convert Bluetooth Core backend to |BluetoothAddress|

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

Suppose |BluetoothAddress| would be used entirely within Gecko in the near future, the overhead of |StringToAddress()| and |AddressToAddress()| and the corresponding error handling could be saved while all the refactor stuffs are done. But at this moment, some trivial error handling might still be needed due to the refactor stuffs are still in progress. Hope we can eliminate those overhead soon. :)

r=me if the following suggestions would be addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +730,5 @@
> +
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(addressString, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +765,5 @@
> +
> +    BluetoothAddress address;
> +    nsresult rv = StringToAddress(aDeviceAddress[i], address);
> +    if (NS_FAILED(rv)) {
> +      return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +833,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1095,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1116,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1160,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1217,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1701,4 @@
>      NS_ENSURE_TRUE_VOID(bs);
>  
>      // Cleanup static adapter properties and notify adapter.
> +    mBdAddress = BluetoothAddress::ANY;

As discussed on comment 14, assigning the value of |BluetoothAddress::ANY| to a |BluetoothAddress| instance seems not self-explanatory enough to the logic of this statement. Suppose this would be modified based on the refined version of |BluetoothAddress|.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +383,2 @@
>    /* PROPERTY_BDNAME
>       PROPERTY_BDADDR

Suppose the corresponding value of |PROPERTY_BDADDR| would be contained in |mBdAddress|, so probably we could remove the wording (i.e. |PROPERTY_BDADDR|) from the comments of |mString|.

Comment 18

[Bruce Sun [:brsun]]

Comment on attachment 8664945 [details] [diff] [review]
[03] Bug 1207649: Convert Bluetooth Socket backend to |BluetoothAddress|

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

r=me if the following questions has been clarified.

::: dom/bluetooth/bluedroid/BluetoothSocketMessageWatcher.cpp
@@ +142,2 @@
>    ReadBdAddress(OFF_BDADDRESS, bdAddress);
> +  return BluetoothAddress(static_cast<const BluetoothAddress&>(bdAddress));

Since we have copy constructor already for |BluetoothAddress|, could we simply return |bdAddress| here?

Comment 19

[Jocelyn Liu [:jocelyn] [:joliu]]

[Blocking Requested - why for this release]:

nominate as 2.5 blocker since there's a 2.5+ dependent bug.

Comment 20

[Bruce Sun [:brsun]]

Comment on attachment 8664946 [details] [diff] [review]
[04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress|

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

r=me if the following question would be clarified.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1171,4 @@
>    }
>  
>    if (mAudioState != HFP_AUDIO_STATE_DISCONNECTED) {
> +    BluetoothAddress deviceAddress;

How about moving the conversion between string to |BluetoothAddress| in the beginning of this function? So that the same logic won't duplicate in both |mConnectionState != HFP_CONNECTION_STATE_DISCONNECTED| case and |mAudioState != HFP_AUDIO_STATE_DISCONNECTED| case.

Comment 21

[Bruce Sun [:brsun]]

Comment on attachment 8664950 [details] [diff] [review]
[08] Bug 1207649: Remove obsolete string/address conversion from Bluetooth backend

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

Looks good to me. :)

Comment 22

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

Comment on attachment 8664948 [details] [diff] [review]
[06] Bug 1207649: Convert Bluetooth AVRCP backend to |BluetoothAddress|

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

LGTM

Comment 23

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

Attachment: [01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code (v2)

Changes since v1:

  - initialize addresses to ANY by default 
  - added getter/setter for address parts
  - added |BluetoothAddress::Clear|
  - test for EOF result in address conversion
  - use |HashBytes| for generating hash key

Comment 24

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

Attachment: [02] Bug 1207649: Convert Bluetooth Core backend to |BluetoothAddress| (v2)

Changes since v1:

  - dispatch error runnables when address conversion fails
  - use |Clear| to clear addresses
  - fix comment in |BluetoothProperty|

Comment 25

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

Attachment: [03] Bug 1207649: Convert Bluetooth Socket backend to |BluetoothAddress| (v2)

Changes since v1:

  - return address from socket-setup message directly

Comment 26

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

Attachment: [04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress| (v2)

Change since v1:

  - only convert address once in |HandleBackendError|

Comment 27

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

Attachment: [07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v2)

Changes since v1:

  - rebased onto latest m-c

Comment 28

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

Attachment: [08] Bug 1207649: Remove obsolete string/address conversion from Bluetooth backend (v2)

Changes since v1:

  - rebased onto latest m-c

Comment 29

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

The patches at v2 should fix everything that has been mentioned so far.

Comment 30

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8665978 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v2)

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

Hi Thomas,

I have few questions regarding to BluetoothGattManager.cpp.

1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.
2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in BluetoothGattManager.cpp, how about we call StringToAddress and check the result as early as possible, since it's checking the validity of the argument?

Haven't finish the review of GattMgr.cpp, I would like to know the answers of these questions to review the rest part in GattMgr.cpp.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +1601,4 @@
>        return rv;
>      }
>      /* Read address */
> +    rv = UnpackPDU(pdu, aArg4);

We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf, right?

Same thing applies to ClientReadRemoteRssiInitOp, ServerRequestReadInitOp, ServerRequestExecuteWriteInitOp below.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +819,5 @@
> +        BluetoothValue(false)); // Disconnected
> +
> +      // Reject the connect request
> +      DispatchReplyError(client->mConnectRunnable,
> +                         NS_LITERAL_STRING("Connect failed"));

I would suggest to revise as
|DispatchReplyError(client->mConnectRunnable, STATUS_PARM_INVALID);|
Applies to other |DispatchReplyError| calls below.

@@ +828,2 @@
>      sBluetoothGattInterface->Connect(client->mClientIf,
> +                                           deviceAddr,

Would you mind to help to fix the alignment here?

@@ +899,5 @@
> +  if (NS_FAILED(rv)) {
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Notify BluetoothGatt for client disconnected

Notify BluetoothGatt that client remains connected

@@ +903,5 @@
> +    // Notify BluetoothGatt for client disconnected
> +    bs->DistributeSignal(
> +      NS_LITERAL_STRING(GATT_CONNECTION_STATE_CHANGED_ID),
> +      client->mAppUuid,
> +      BluetoothValue(true)); // Disconnected

the comment should be Connected

@@ +1031,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(aDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mReadRemoteRssiRunnable);

redundant here.

@@ +1119,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(client->mDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mRegisterNotificationsRunnable);

redundant

@@ +1205,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(client->mDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mDeregisterNotificationsRunnable);

DITTO.

@@ +1671,5 @@
>    if (server->mServerIf > 0) {
> +    BluetoothAddress address;
> +    nsresult rv = StringToAddress(aAddress, address);
> +    if (NS_FAILED(rv)) {
> +      MOZ_ASSERT(server->mConnectPeripheralRunnable);

DITTO.

Comment 31

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

Hi

> 
> I have few questions regarding to BluetoothGattManager.cpp.
> 
> 1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by
> BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.

Shall I add these changes?

I simply didn't do this because the patch set was already large and I focused on the Bluetooth backend interfaces.

> 2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in
> BluetoothGattManager.cpp, how about we call StringToAddress and check the
> result as early as possible, since it's checking the validity of the
> argument?

Makes sense. I simply tried to stay close to the original code.

> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> @@ +1601,4 @@
> >        return rv;
> >      }
> >      /* Read address */
> > +    rv = UnpackPDU(pdu, aArg4);
> 
> We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf,
> right?

With all the explicit conversions gone, there are a lot more places that can use |UnpackPDUInitOp|. I wanted to do these changes in another bug.

Comment 32

[Mahendra Potharaju [:mahe]]

Fixing this bug will close other issue too, bug 1200495. 

Blocks 2.5 with a priority P2.

Comment 33

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #31)
> Hi
> 
> > 
> > I have few questions regarding to BluetoothGattManager.cpp.
> > 
> > 1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by
> > BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.
> 
> Shall I add these changes?
> 
> I simply didn't do this because the patch set was already large and I
> focused on the Bluetooth backend interfaces.
> 
I originally thought the scope here also covers profile managers, but found that's not true when revisiting Comment 1.
Please ignore my comment, let's do it when cleaning profile managers.
Sorry.

> > 2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in
> > BluetoothGattManager.cpp, how about we call StringToAddress and check the
> > result as early as possible, since it's checking the validity of the
> > argument?
> 
> Makes sense. I simply tried to stay close to the original code.
> 
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> > @@ +1601,4 @@
> > >        return rv;
> > >      }
> > >      /* Read address */
> > > +    rv = UnpackPDU(pdu, aArg4);
> > 
> > We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf,
> > right?
> 
> With all the explicit conversions gone, there are a lot more places that can
> use |UnpackPDUInitOp|. I wanted to do these changes in another bug.

Ok, got it.

Comment 34

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8665978 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v2)

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

Almost there, but I would like to double check the logic after moving argument check, thanks.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +905,5 @@
> +      NS_LITERAL_STRING(GATT_CONNECTION_STATE_CHANGED_ID),
> +      client->mAppUuid,
> +      BluetoothValue(true)); // Disconnected
> +
> +    // Reject the connect request

disconnect request here and mDisconnectRunnable below.

Comment 35

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

Attachment: [07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v3)

Changes since v2:

  - convert address strings as early as possible
  - typo and indention fixes

Comment 36

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8667131 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v3)

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

r=me, thanks for the effort.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +888,5 @@
> +  if (NS_FAILED(rv)) {
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Notify BluetoothGatt for client connected

nit: Notify BluetoothGatt that client remains connected

Comment 37

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

Attachment: [07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v4)

Changes since v3:

  - changed comment as advised in review

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a336f5041c27
https://hg.mozilla.org/integration/b2g-inbound/rev/9c647b56f37c
https://hg.mozilla.org/integration/b2g-inbound/rev/f03f71a3de8a
https://hg.mozilla.org/integration/b2g-inbound/rev/b906ed92f33a
https://hg.mozilla.org/integration/b2g-inbound/rev/02928720b1a0
https://hg.mozilla.org/integration/b2g-inbound/rev/d63567af5fa2
https://hg.mozilla.org/integration/b2g-inbound/rev/fcbf5852361f
https://hg.mozilla.org/integration/b2g-inbound/rev/df09a48144a5

Comment 39

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

Thanks everyone!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=df09a48144a5

Comment 40

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2889789&repo=b2g-inbound

Comment 41

[Pulsebot]

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

Comment 42

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

Testing a fix on try:

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

Comment 43

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

Attachment: [01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code (v3)

Changes since v2:

  - removed move c'tor/assignment from |BluetoothAddress| to build on Windows

We don't need move here anyway.

Comment 44

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

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3c17c0c67491

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/2159a5ce2b1a
https://hg.mozilla.org/integration/b2g-inbound/rev/09dbe50829ec
https://hg.mozilla.org/integration/b2g-inbound/rev/1bbf292813c9
https://hg.mozilla.org/integration/b2g-inbound/rev/f26250d6c260
https://hg.mozilla.org/integration/b2g-inbound/rev/5590a7ecf08a
https://hg.mozilla.org/integration/b2g-inbound/rev/b6d66b7aa7fb
https://hg.mozilla.org/integration/b2g-inbound/rev/7e7354e2baf5

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/2159a5ce2b1a
https://hg.mozilla.org/mozilla-central/rev/09dbe50829ec
https://hg.mozilla.org/mozilla-central/rev/1bbf292813c9
https://hg.mozilla.org/mozilla-central/rev/f26250d6c260
https://hg.mozilla.org/mozilla-central/rev/5590a7ecf08a
https://hg.mozilla.org/mozilla-central/rev/b6d66b7aa7fb
https://hg.mozilla.org/mozilla-central/rev/7e7354e2baf5
https://hg.mozilla.org/mozilla-central/rev/3c17c0c67491