1222956 - The promise returned from BluetoothAdapter.startLeScan will always be rejected.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

This is a follow-up bug from bug 1220121 comment 22. The promise returned from |BluetoothAdapter.startLeScan|[1] will always be rejected.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#startLeScan.28sequence.3CDOMString.3E_aServiceUuids.29

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.patch

This patch is based on m-c. It might need to be re-based depending on the progress of bug 1220121.

Comment 2

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

Comment on attachment 8685334 [details] [diff] [review]
bug1222956_bluetooth_adapter_startlescan_rejected.patch

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

Thanks for the fix. I'd like to see the UUID issues being cleaned up before it lands.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +595,5 @@
>  
>    BluetoothUuid& operator=(const BluetoothUuid& aRhs) = default;
>  
> +  /* This less-than operator is used for sorted insertion of nsTArray */
> +  friend bool operator <(const BluetoothUuid&, const BluetoothUuid&);

No empty space before '<'. Declare as 'const'.

@@ +670,5 @@
>  
> +/* This less-than operator is used for sorted insertion of nsTArray */
> +inline bool operator <(const BluetoothUuid& aUuidA, const BluetoothUuid& aUuidB)
> +{
> +  return memcmp(aUuidA.mUuid, aUuidB.mUuid, sizeof(aUuidA.mUuid)) < 0;

Please implement this operator inline in |BluetoothUuid|.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +371,5 @@
>        RefPtr<BluetoothVoidReplyRunnable> results =
>          new BluetoothVoidReplyRunnable(nullptr);
> +      nsAutoString uuidStr;
> +      UuidToString(uuid, uuidStr);
> +      bs->StopLeScanInternal(uuidStr, results);

This gets fixed in bug 1220121, patch [07]. I hope this will be reviewed soon.

::: dom/bluetooth/common/webapi/BluetoothDevice.cpp
@@ +391,5 @@
>            } else if (type == GAP_INCOMPLETE_UUID32 ||
>                       type == GAP_COMPLETE_UUID32) {
> +            uint8_t uuid[] = {aAdvData[offset], aAdvData[offset + 1],
> +                              aAdvData[offset + 2], aAdvData[offset + 3]};
> +            mUuids.AppendElement(BluetoothUuid(LittleEndian::readUint32(uuid)));

|LittleEndian::readUint32| means that 'uuid' contains an LE value which is converted to native order, right? The interface is cryptic to me. And 'aAdvData' is always in LE order?

The byte order in the Bluetooth UUIDs is nothing but a mess; everywhere in our code base. There's also code in |StringToUuid| that converts byte orders. We should have a constructor for |BluetoothUuid| that take the 6 values of a UUID in native byte order and writes them into the UUID buffer (in BE order apparently). Could you provide a patch for this as part of this patch set?

@@ +410,5 @@
> +                                               aAdvData[offset + 4],
> +                                               aAdvData[offset + 3],
> +                                               aAdvData[offset + 2],
> +                                               aAdvData[offset + 1],
> +                                               aAdvData[offset]));

Here 'aAdvData' gets reversed. That looks correct when compared to the old code, but it's still cryptic. I think this should be handled in the same UUID constructor as the other cases. It's better to shift the array bytes into values of |uint*_t| and hand them over to the constructor.

Comment 3

[Bruce Sun [:brsun]]

> ::: dom/bluetooth/common/webapi/BluetoothDevice.cpp
> @@ +391,5 @@
> >            } else if (type == GAP_INCOMPLETE_UUID32 ||
> >                       type == GAP_COMPLETE_UUID32) {
> > +            uint8_t uuid[] = {aAdvData[offset], aAdvData[offset + 1],
> > +                              aAdvData[offset + 2], aAdvData[offset + 3]};
> > +            mUuids.AppendElement(BluetoothUuid(LittleEndian::readUint32(uuid)));
> 
> |LittleEndian::readUint32| means that 'uuid' contains an LE value which is
> converted to native order, right? The interface is cryptic to me.
>

The byte order in |uuid[]| is the exactly the same as |aAdvData|, but since |nsTArray| doesn't expose a way/pointer to let others access its internal array from the outside world sequentially, |uuid[]| is a wordaround to make sure we can have a continuous memory which is accessible by the endian utility. There are already one |BluetoothUuid| constructor which takes |uint16_t| value in native order and another one constructor which takes |uint32_t| values in native order. The usage of these contructors probably is straight forward.


> And 'aAdvData' is always in LE order?
> 

I think so.

> The byte order in the Bluetooth UUIDs is nothing but a mess; everywhere in
> our code base.

I think the byte order of Bluetooth UUIDs is confusing because of the spec:

Extended Inquiry Response(EIR) and Advertising Data(AD) in Generic Access Profile(GAP) uses little-endian byte order as stated in Supplement to Bluetooth Core Specification, Part A, Section 1.

Service Discovery Protocol(SDP) uses big-endian byte order as stated in Bluetooth Core Specification, Volume 3, Part B, Section 4.2.

While most of the multiple-byte fields use little-endian in Bluetooth spec, SDP shapes its own style to use big-endian.

> There's also code in |StringToUuid| that converts byte orders.

It seems that both |StringToUuid| and |UuidToString| handle the byte order that |BluetoothUuid| accepts. These two function probably should be isolated from the backend stuffs and should only handle our byte order in |BluetoothUuid|. And they already can do the job properly without extra complexity, right?

> We should have a constructor for |BluetoothUuid| that take the 6
> values of a UUID in native byte order and writes them into the UUID buffer
> (in BE order apparently). Could you provide a patch for this as part of this
> patch set?
> 

Sure. This will be handled in the next version. But probably this is not so related to the native order of |uint16_t| and |uint32_t| versions which are used in the current patch. Maybe it can help on 128-bit values by the way.

> @@ +410,5 @@
> > +                                               aAdvData[offset + 4],
> > +                                               aAdvData[offset + 3],
> > +                                               aAdvData[offset + 2],
> > +                                               aAdvData[offset + 1],
> > +                                               aAdvData[offset]));
> 
> Here 'aAdvData' gets reversed. That looks correct when compared to the old
> code, but it's still cryptic. I think this should be handled in the same
> UUID constructor as the other cases. It's better to shift the array bytes
> into values of |uint*_t| and hand them over to the constructor.

To have one constructor of |BluetoothUuid| with |uint8_t*| array pointer (or |nsTArray<uint8_t>&) would introduce byte order issues in |BluetoothUuid|. Currently |BluetoothUuid| handles native order only, which is good I think. I am not sure whether it is good as well to address other byte order conversion inside |BluetoothUuid|. Maybe using a constructor which takes 6 values in native byte orders is preferred in this case?

Comment 4

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

Hi

(In reply to Bruce Sun [:brsun] from comment #3)
> > ::: dom/bluetooth/common/webapi/BluetoothDevice.cpp
> > @@ +391,5 @@
> > >            } else if (type == GAP_INCOMPLETE_UUID32 ||
> > >                       type == GAP_COMPLETE_UUID32) {
> > > +            uint8_t uuid[] = {aAdvData[offset], aAdvData[offset + 1],
> > > +                              aAdvData[offset + 2], aAdvData[offset + 3]};
> > > +            mUuids.AppendElement(BluetoothUuid(LittleEndian::readUint32(uuid)));
> > 
> > |LittleEndian::readUint32| means that 'uuid' contains an LE value which is
> > converted to native order, right? The interface is cryptic to me.
> >
> 
> The byte order in |uuid[]| is the exactly the same as |aAdvData|, but since
> |nsTArray| doesn't expose a way/pointer to let others access its internal
> array from the outside world sequentially, |uuid[]| is a wordaround to make
> sure we can have a continuous memory which is accessible by the endian
> utility. There are already one |BluetoothUuid| constructor which takes
> |uint16_t| value in native order and another one constructor which takes
> |uint32_t| values in native order. The usage of these contructors probably
> is straight forward.

These constructors handle these cases fine; I was rather taking about the big picture.

I see what uuid[] does, but I'd rather have the values being shifted into a |uint*_t| value. That would remove the need for explicit conversion.

> 
> > And 'aAdvData' is always in LE order?
> > 
> 
> I think so.
> 
> > The byte order in the Bluetooth UUIDs is nothing but a mess; everywhere in
> > our code base.
> 
> I think the byte order of Bluetooth UUIDs is confusing because of the spec:
> 
> Extended Inquiry Response(EIR) and Advertising Data(AD) in Generic Access
> Profile(GAP) uses little-endian byte order as stated in Supplement to
> Bluetooth Core Specification, Part A, Section 1.
> 
> Service Discovery Protocol(SDP) uses big-endian byte order as stated in
> Bluetooth Core Specification, Volume 3, Part B, Section 4.2.
> 
> While most of the multiple-byte fields use little-endian in Bluetooth spec,
> SDP shapes its own style to use big-endian.

Thanks for these pointers.

> > There's also code in |StringToUuid| that converts byte orders.
> 
> It seems that both |StringToUuid| and |UuidToString| handle the byte order
> that |BluetoothUuid| accepts. These two function probably should be isolated
> from the backend stuffs and should only handle our byte order in
> |BluetoothUuid|. And they already can do the job properly without extra
> complexity, right?
> 
> > We should have a constructor for |BluetoothUuid| that take the 6
> > values of a UUID in native byte order and writes them into the UUID buffer
> > (in BE order apparently). Could you provide a patch for this as part of this
> > patch set?
> > 
> 
> Sure. This will be handled in the next version. But probably this is not so
> related to the native order of |uint16_t| and |uint32_t| versions which are
> used in the current patch. Maybe it can help on 128-bit values by the way.

Yes, my comments don't always align correctly with your code.

> > @@ +410,5 @@
> > > +                                               aAdvData[offset + 4],
> > > +                                               aAdvData[offset + 3],
> > > +                                               aAdvData[offset + 2],
> > > +                                               aAdvData[offset + 1],
> > > +                                               aAdvData[offset]));
> > 
> > Here 'aAdvData' gets reversed. That looks correct when compared to the old
> > code, but it's still cryptic. I think this should be handled in the same
> > UUID constructor as the other cases. It's better to shift the array bytes
> > into values of |uint*_t| and hand them over to the constructor.
> 
> To have one constructor of |BluetoothUuid| with |uint8_t*| array pointer (or
> |nsTArray<uint8_t>&) would introduce byte order issues in |BluetoothUuid|.

I didn't mean an uint8_t*-based constructor, but the 6-parameter constructor. But byte order still remains an issue.

> Currently |BluetoothUuid| handles native order only, which is good I think.
> I am not sure whether it is good as well to address other byte order
> conversion inside |BluetoothUuid|. Maybe using a constructor which takes 6
> values in native byte orders is preferred in this case?


How about we remove *all* byte-order conversion from |BluetoothUuid| and expect all method's input and output to be in native byte order every time? On to of that we create a number of static functions in |BluetoothUuid| that do these conversions. For example:

  * |BluetoothUuid::CreateBE(...)| converts to BE before constructing a UUID
  * |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| converts to BE before storing
  * |BluetoothUUID::GetUuid16LE(uuid)| reads the UUID's 16-bit value and converts to LE before returning

Byte-ordering would at least be clear with this API.

Comment 5

[Bruce Sun [:brsun]]

Hi,

> These constructors handle these cases fine; I was rather taking about the
> big picture.
> 
> I see what uuid[] does, but I'd rather have the values being shifted into a
> |uint*_t| value. That would remove the need for explicit conversion.
> 

I am not sure I understand "have the values being shifted into a |uint*_t| value" precisely. Does it refer to the following styles?
 - uint16_t uuid16 = (static_cast<uint16_t>(aAdvData[offset])) | (static_cast<uint16_t>(aAdvData[offset + 1]) << 8); mUuids.AppendElement(BluetoothUuid(uuid16));
 - uint32_t uuid32 = (static_cast<uint32_t>(aAdvData[offset])) | (static_cast<uint32_t>(aAdvData[offset + 1]) << 8) | (static_cast<uint32_t>(aAdvData[offset + 2]) << 16) | (static_cast<uint32_t>(aAdvData[offset + 3]) << 24); mUuids.AppendElement(BluetoothUuid(uuid32));

The shift and concatenation are parts of byte-order conversion. Using endian utilities to explicitly handle this conversion would tend to be more error-proof.


But if we use the following styles:
 - uint16_t uuid16 = (static_cast<uint16_t>(aAdvData[offset]) << 8) | (static_cast<uint16_t>(aAdvData[offset + 1])); mUuids.AppendElement(BluetoothUuid::CreateFromSomeEndian(uuid16));
 - uint32_t uuid32 = (static_cast<uint32_t>(aAdvData[offset]) << 32) | (static_cast<uint32_t>(aAdvData[offset + 1]) << 16) | (static_cast<uint32_t>(aAdvData[offset + 2]) << 8) | (static_cast<uint32_t>(aAdvData[offset + 3])); mUuids.AppendElement(BluetoothUuid::CreateFromSomeEndian(uuid32));

These styles introduce confusions to the native types. The |uint*_t| variables could store values which are not in native byte orders. This is a little bit hacky. And it requires a very clear mind to understand that these variables are parts of byte-order conversion. The stored values in them are used for transition only. Packing all steps into one concrete logic to do byte-order conversion (or simply using the existing endian utilities) probably can make our codes clearer.


Kindly help correct me if I got something wrong.

> > To have one constructor of |BluetoothUuid| with |uint8_t*| array pointer (or
> > |nsTArray<uint8_t>&) would introduce byte order issues in |BluetoothUuid|.
> 
> I didn't mean an uint8_t*-based constructor, but the 6-parameter
> constructor. But byte order still remains an issue.
> 

I would prefer to keep native types simple by always store values with native byte order in it. By doing so we can save burdens to document why and how the values in a native type but not using a native byte order.

I would also prefer to concatenate multiple bytes into a large value only in native byte order. We'd better to use endian utilities to parse the sequential memory instead of shifting and concatenating by our own. Purely dividing memories into different segments by our own is fine to me because the logic won't be affected by the endianness of values.

> > Currently |BluetoothUuid| handles native order only, which is good I think.
> > I am not sure whether it is good as well to address other byte order
> > conversion inside |BluetoothUuid|. Maybe using a constructor which takes 6
> > values in native byte orders is preferred in this case?
> 
> 
> How about we remove *all* byte-order conversion from |BluetoothUuid| and
> expect all method's input and output to be in native byte order every time?

I fully agree on this. But I think the current design of |BluetoothUuid| satisfies the statement already. No?

> On to of that we create a number of static functions in |BluetoothUuid| that
> do these conversions. For example:
> 
>   * |BluetoothUuid::CreateBE(...)| converts to BE before constructing a UUID
>   * |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| converts to BE before storing
>   * |BluetoothUUID::GetUuid16LE(uuid)| reads the UUID's 16-bit value and
> converts to LE before returning
> 
> Byte-ordering would at least be clear with this API.

I am not sure whether it is a good idea to expose interfaces to set/get a multiple-byte value in a native type but formatted in a non-native-byte order. I am a little afraid that using such kind of variables would be error-prone.


Probably my worry is excessive. Does that make sense to you?

Comment 6

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

Hi

(In reply to Bruce Sun [:brsun] from comment #5)
> 
> The shift and concatenation are parts of byte-order conversion. Using endian
> utilities to explicitly handle this conversion would tend to be more
> error-proof.
> 
> These styles introduce confusions to the native types. The |uint*_t|
> variables could store values which are not in native byte orders. This is a
> little bit hacky. And it requires a very clear mind to understand that these
> variables are parts of byte-order conversion. The stored values in them are
> used for transition only. Packing all steps into one concrete logic to do
> byte-order conversion (or simply using the existing endian utilities)
> probably can make our codes clearer.

Your examples were correct. Let me address your points.

 1) The more I think about it, the more sense it makes to always store native byte order in |uint*_t| (and |BluetoothUuid|). There are types like |__be32| for non-native ordering in the Linux kernel. If we ever have to store non-native ordering, we should use such a type as well.

 2) We know the order of the input data. And the shifting and bit-or'ing will guarantee that the results are in native order.

 3) The current interfaces are badly named and not intuitive. For example

  |LittleEndian::readUint32|

  Does this name mean that it's reading a LE value and converting it to native order? Or does it read a native value and than converts it to LE ordering. Does it even convert? Maybe it just reads.

  That's why I don't think that the current interface is more error-proof than shifting. I don't mind using a helper function for these shifts, but I'd prefer an interface that is clearer in its naming. 


> > > To have one constructor of |BluetoothUuid| with |uint8_t*| array pointer (or
> > > |nsTArray<uint8_t>&) would introduce byte order issues in |BluetoothUuid|.
> > 
> > I didn't mean an uint8_t*-based constructor, but the 6-parameter
> > constructor. But byte order still remains an issue.
> > 
> 
> I would prefer to keep native types simple by always store values with
> native byte order in it. By doing so we can save burdens to document why and
> how the values in a native type but not using a native byte order.
> 
> I would also prefer to concatenate multiple bytes into a large value only in
> native byte order. We'd better to use endian utilities to parse the
> sequential memory instead of shifting and concatenating by our own. Purely
> dividing memories into different segments by our own is fine to me because
> the logic won't be affected by the endianness of values.

I'm not sure if I got all of your argument, but I think we agree. As I mentioned above, I don't mind the use of helper function, but the current helpers are completely unintuitive.


> > > Currently |BluetoothUuid| handles native order only, which is good I think.
> > > I am not sure whether it is good as well to address other byte order
> > > conversion inside |BluetoothUuid|. Maybe using a constructor which takes 6
> > > values in native byte orders is preferred in this case?
> > 
> > 
> > How about we remove *all* byte-order conversion from |BluetoothUuid| and
> > expect all method's input and output to be in native byte order every time?
> 
> I fully agree on this. But I think the current design of |BluetoothUuid|
> satisfies the statement already. No?

|BluetoothUuid::SetUuid32| converts the input value to BE. But wouldn't this collide with the all-LE approach of EIR and AD? When such an UUID is send out in a backend, the order of certain bytes is suddenly reversed.

> > On to of that we create a number of static functions in |BluetoothUuid| that
> > do these conversions. For example:
> > 
> >   * |BluetoothUuid::CreateBE(...)| converts to BE before constructing a UUID
> >   * |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| converts to BE before storing
> >   * |BluetoothUUID::GetUuid16LE(uuid)| reads the UUID's 16-bit value and
> > converts to LE before returning
> > 
> > Byte-ordering would at least be clear with this API.
> 
> I am not sure whether it is a good idea to expose interfaces to set/get a
> multiple-byte value in a native type but formatted in a non-native-byte
> order. I am a little afraid that using such kind of variables would be
> error-prone.

Only the protocols (SDP, EIR, AD) specify which ordering to use. The ideas is to store everything in native order and retrieve the values in the correct (BE/LE) order when sending out the UUID to bluetoothd or DOM interfaces.

What do you think?

Comment 7

[Bruce Sun [:brsun]]

> > > On to of that we create a number of static functions in |BluetoothUuid| that
> > > do these conversions. For example:
> > > 
> > >   * |BluetoothUuid::CreateBE(...)| converts to BE before constructing a UUID
> > >   * |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| converts to BE before storing
> > >   * |BluetoothUUID::GetUuid16LE(uuid)| reads the UUID's 16-bit value and
> > > converts to LE before returning
> > > 
> > > Byte-ordering would at least be clear with this API.
> > 

After thinking this issue again and again, I finally found that these static functions do help the byte conversion stuffs and hide the detail of the internal byte ordering of |BluetoothUuid|. I think this is a good idea to have these static functions.

But I still don't feel comfortable to use native types to store values in non-native byte order. I would like to tweak the types of the arguments a little bit to ease this concern.

> > I am not sure whether it is a good idea to expose interfaces to set/get a
> > multiple-byte value in a native type but formatted in a non-native-byte
> > order. I am a little afraid that using such kind of variables would be
> > error-prone.
> 
> Only the protocols (SDP, EIR, AD) specify which ordering to use. The ideas
> is to store everything in native order and retrieve the values in the
> correct (BE/LE) order when sending out the UUID to bluetoothd or DOM
> interfaces.
> 
> What do you think?

How the byte order is managed in |BluetoothUuid| probably doesn't matter, as long as we can have values with the correct order through |BluetoothUuid|'s interfaces.

I would try to address all above in the next patches.

Comment 8

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

Hi

(In reply to Bruce Sun [:brsun] from comment #7)

> But I still don't feel comfortable to use native types to store values in
> non-native byte order. I would like to tweak the types of the arguments a
> little bit to ease this concern.

I don't understand where we are storing values non-native bit order? Can you give an example?

Comment 9

[Bruce Sun [:brsun]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> Hi
> 
> (In reply to Bruce Sun [:brsun] from comment #7)
> 
> > But I still don't feel comfortable to use native types to store values in
> > non-native byte order. I would like to tweak the types of the arguments a
> > little bit to ease this concern.
> 
> I don't understand where we are storing values non-native bit order? Can you
> give an example?

Currently there is no such example in codes, this concern is just for the future work we are going to do.

> > > >   * |BluetoothUuid::CreateBE(...)| converts to BE before constructing a UUID
> > > >   * |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| converts to BE before storing
> > > >   * |BluetoothUUID::GetUuid16LE(uuid)| reads the UUID's 16-bit value and
> > > > converts to LE before returning

For example, if |uuid32| uses a type of |uint32_t|, then |uuid32| in |BluetoothUuid::SetUuuid32BE(uuid, uuid32)| would be the case.

Comment 10

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.rebase.patch

Re-based version of attachment 8685334 [details] [diff] [review] after bug 1220121 has been resolved.

Comment 11

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.rebase_2.patch

Re-base again.

Comment 12

[Chris Lord [:cwiiis]]

Nominating this to block 2.6. Given we're meant to be concentrating on IoT with FxOS as a base, Bluetooth LE ought to be important.

Comment 13

[Ben Tian (inactive)]

Set 2.6+ for broken LE scan functionality.

Comment 14

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.rebase_3.patch

rebase again

Comment 15

[Chris Lord [:cwiiis]]

(In reply to Bruce Sun [:brsun] from comment #14)
> Created attachment 8719413 [details] [diff] [review]
> bug1222956_bluetooth_adapter_startlescan_rejected.rebase_3.patch
> 
> rebase again

This patch isn't up for review - should it be? If not, what's missing?

Comment 16

[Bruce Sun [:brsun]]

Hi Chris,

The patch needs to be refined based on reviewer's comments. I was not working it recently due to other issues, sorry for the delay. And now I am back and going to work on it again.

Comment 17

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.v2.patch

Comment 18

[Bruce Sun [:brsun]]

This patch is focusing on solving the regression only. Some of original discussions are not contained in the patch due to more re-factor/refinement related. Handling the refinement on a separated bug could make the changeset more readable.

Comment 19

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

Comment on attachment 8719716 [details] [diff] [review]
bug1222956_bluetooth_adapter_startlescan_rejected.v2.patch

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

Hi!

::: dom/bluetooth/common/BluetoothCommon.h
@@ +551,5 @@
>  
> +/*
> + * Each profile has its distinct endianness for multi-byte values
> + */
> +enum ProfileEndian {

The name should be prefixed with 'Bluetooth'

@@ +566,5 @@
> + * assignment to often-used, registered purposes. UUID values in the
> + * pre-allocated range have aliases that are represented as 16-bit or 32-bit
> + * values.
> + */
> +enum UuidType {

'Bluetooth' prefix

@@ +675,5 @@
>     *
>     * Below are helpers for accessing these values.
>     */
>  
>    void SetUuid32(uint32_t aUuid32)

Should these helper methods be implemented with |SetUuid<>| and |GetUuid<>|?

@@ +699,5 @@
>    }
> +
> +  template<UuidType T, ProfileEndian E>
> +  size_t SetUuid(const nsTArray<uint8_t>& aUuid,
> +                 nsTArray<uint8_t>::index_type aOffset)

I seems wrong to implement this method as template. Templates are useful to solve a more general problem that does not depend on data types or constants. But here, the template parameters serve the purpose of method parameters.

I think it would be better to either

 - special the template |SetUuid| for the valid parameters, or
 - pass the template parameters as method parameters and hide |SetUuid| behind easier interfaces (e.g., |SetUuid128|)

@@ +704,5 @@
> +  {
> +    MOZ_ASSERT(T == UUID_16_BIT || T == UUID_32_BIT || T == UUID_128_BIT);
> +    MOZ_ASSERT(E == ENDIAN_BIG || E == ENDIAN_LITTLE);
> +
> +    size_t index = (T == UUID_16_BIT) ? 2 : 0;

This branch should be implemented as specialized template method.

@@ +707,5 @@
> +
> +    size_t index = (T == UUID_16_BIT) ? 2 : 0;
> +    size_t length = 0;
> +
> +    if (T == UUID_16_BIT) {

This set of branches should be implemented as specialized template method.

@@ +715,5 @@
> +    } else {
> +      length = MOZ_ARRAY_LENGTH(mUuid);
> +    }
> +
> +    MOZ_ASSERT(aUuid.Length() >= aOffset + length);

Why can the UUID array be longer than the stored data?

@@ +717,5 @@
> +    }
> +
> +    MOZ_ASSERT(aUuid.Length() >= aOffset + length);
> +
> +    operator=(BASE);

Assigning base (or not) depends on the template parameter.

@@ +719,5 @@
> +    MOZ_ASSERT(aUuid.Length() >= aOffset + length);
> +
> +    operator=(BASE);
> +
> +    if (E == ENDIAN_BIG) {

This branch should be implemented as specialized template method.

@@ +729,5 @@
> +        mUuid[index + i] = aUuid[aOffset + length - i - 1];
> +      }
> +    }
> +
> +    return length;

This method should not return 'length', or anything else. The value of 'length' only depends on the template parameter and should be returned by a separate helper method, if any.

@@ +733,5 @@
> +    return length;
> +  }
> +
> +  template<UuidType T, ProfileEndian E>
> +  size_t GetUuid(nsTArray<uint8_t>& aUuid) const

Same comments as for |SetUuid|.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ -372,5 @@
>        RefPtr<BluetoothVoidReplyRunnable> results =
>          new BluetoothVoidReplyRunnable(nullptr);
> -
> -      BluetoothUuid uuid;
> -      if (NS_SUCCEEDED(StringToUuid(uuidStr, uuid))) {

Good to see this being removed :)

::: dom/bluetooth/common/webapi/BluetoothDevice.cpp
@@ +406,2 @@
>            if (type == GAP_INCOMPLETE_UUID16 || type == GAP_COMPLETE_UUID16) {
> +            length = uuid.SetUuid<UUID_16_BIT, ENDIAN_GAP>(aAdvData, offset);

The way you handle 'length' here is problematic, because it mixes up "length of BluetoothUuid" and "length of Bluetooth protocol". Just because they are of the same value doesn't mean that they are the same.

I'd suggest to move the conversion of |nsTArray| to |BluetoothUuid| into a separate helper method of BluetoothDevice. That means moving |BluetoothUuid::SetUuid| here. Then you wouldn't even need |ProfileEndian| and |UuidType| AFAICT.

Comment 20

[Bruce Sun [:brsun]]

Thanks for your instant feedback. Kindly refer to quick questions/comments inline.

> > +enum ProfileEndian {
> 
> The name should be prefixed with 'Bluetooth'
> 

> > +enum UuidType {
> 
> 'Bluetooth' prefix
> 

These enumerations are contained in |mozilla::dom::bluetooth| already. Would |Bluetooth| prefix still be necessary?

> @@ +715,5 @@
> > +    } else {
> > +      length = MOZ_ARRAY_LENGTH(mUuid);
> > +    }
> > +
> > +    MOZ_ASSERT(aUuid.Length() >= aOffset + length);
> 
> Why can the UUID array be longer than the stored data?
> 

Because |nsTArray| might contain lots of data (ex. other Bluetooth UUIDs, or other data), and one single |BluetoothUuid| only needs some part of it.

Comment 21

[Bruce Sun [:brsun]]

> I'd suggest to move the conversion of |nsTArray| to |BluetoothUuid| into a
> separate helper method of BluetoothDevice. That means moving
> |BluetoothUuid::SetUuid| here. Then you wouldn't even need |ProfileEndian|
> and |UuidType| AFAICT.

Conversion of |nsTArray| and |BluetoothUuid| could be used in other places as well. For example, we could use the conversion function to pack/unpack UUIDs with explicit endianness for different profiles. (By simply reading the codes, currently there are no hints to describe about the reason why some UUIDs need reversed packing/unpacking but others don't.) How about putting these conversion functions to some other places outside |BluetoothDevice|?

Comment 22

[Bruce Sun [:brsun]]

Attachment: bug1222956_bluetooth_adapter_startlescan_rejected.v3.patch

Comment 24

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

Hi

> > 'Bluetooth' prefix
> > 
> 
> These enumerations are contained in |mozilla::dom::bluetooth| already. Would
> |Bluetooth| prefix still be necessary?

All the other constants have the prefix. It would make sense to use prefixes here as well.

> > Why can the UUID array be longer than the stored data?
> > 
> 
> Because |nsTArray| might contain lots of data (ex. other Bluetooth UUIDs, or
> other data), and one single |BluetoothUuid| only needs some part of it.

That's what I meant by mixing up UUID and protocol parser. Please see the next reply.

> > I'd suggest to move the conversion of |nsTArray| to |BluetoothUuid| into a
> > separate helper method of BluetoothDevice. That means moving
> > |BluetoothUuid::SetUuid| here. Then you wouldn't even need |ProfileEndian|
> > and |UuidType| AFAICT.

> How about
> putting these conversion functions to some other places outside
> |BluetoothDevice|?

Yes, I support this idea. I'd think of a parser function that takes the array and the respective parameters, and converts them to a |BluetoothUuid|. It could reuse the constants from |BluetoothDevice| unless there are other, unrelated users of this functionality. |BluetoothUuid| would just have trivial getter and setter functions.

Comment 25

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

(In reply to Bruce Sun [:brsun] from comment #22)
> Created attachment 8720148 [details] [diff] [review]
> bug1222956_bluetooth_adapter_startlescan_rejected.v3.patch

I'll take a look tomorrow.

Comment 26

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

Comment on attachment 8720148 [details] [diff] [review]
bug1222956_bluetooth_adapter_startlescan_rejected.v3.patch

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

Hi!

Thanks for the cleanup. I left a few more comments of things I think are worth fixing.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +551,5 @@
>  
> +/*
> + * Each profile has its distinct endianness for multi-byte values
> + */
> +enum ProfileEndian {

Please use 'Bluetooth'.

I also think these enums should be moved to 'BluetoothUtils.h'. 'BluetoothCommon.h' looks like a dump for almost everything. We should probably split it up at some point.

@@ +579,2 @@
>  
> +  uint8_t mUuid[16];  // store 128-bit UUID value in big endian

'in big-endian order'

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +277,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +BytesToUuid(const nsTArray<uint8_t>& aArray,

Rather pass a pointer to the first raw byte. 'nsTArray<>::Elements() + offset' should give you the address.

@@ +278,5 @@
>  }
>  
>  nsresult
> +BytesToUuid(const nsTArray<uint8_t>& aArray,
> +            nsTArray<uint8_t>::index_type aOffset,

Pass the remaining bytes instead of the offset (nsTArray<>::GetLength() - offset).

@@ +288,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +  if (aEndian != ENDIAN_BIG && aEndian != ENDIAN_LITTLE) {
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }

I think an assertion should do here.

@@ +321,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +UuidToBytes(const BluetoothUuid& aUuid,

Same comments as above.

Comment 27

[Bruce Sun [:brsun]]

Hi Thomas,

Thanks for the suggestion.

> > +/*
> > + * Each profile has its distinct endianness for multi-byte values
> > + */
> > +enum ProfileEndian {
> 
> Please use 'Bluetooth'.
> 
> I also think these enums should be moved to 'BluetoothUtils.h'.
> 'BluetoothCommon.h' looks like a dump for almost everything. We should
> probably split it up at some point.
> 

These two enums probably have many potentials to be used in other place, not only in utilities. But I will do as suggested since there are no such use cases for the moment.

> ::: dom/bluetooth/common/BluetoothUtils.cpp
> @@ +277,5 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> > +BytesToUuid(const nsTArray<uint8_t>& aArray,
> 
> Rather pass a pointer to the first raw byte. 'nsTArray<>::Elements() +
> offset' should give you the address.
> 
> @@ +278,5 @@
> >  }
> >  
> >  nsresult
> > +BytesToUuid(const nsTArray<uint8_t>& aArray,
> > +            nsTArray<uint8_t>::index_type aOffset,
> 
> Pass the remaining bytes instead of the offset (nsTArray<>::GetLength() -
> offset).
> 

Passing raw pointers and length values could maximize the flexibility of the function, but the caller should be more responsible to avoid potential risks. If the caller pass an invalid pointer or an invalid length value, probably there are no many things we could do to avoid crashing. Passing the reference of |nsTArray| could avoid such cases by design in general. We can have extra error checking inside the function comparing to passing raw pointers. Conversion utilities in that file seem very high-level and tend to provide such protection by design. Just for double checking, do you prefer passing raw pointers over passing the reference of |nsTArray| regarding to these conversion utilities (bytes <==> UUID)?

Comment 28

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

Hi 

> Just for double checking, do you prefer
> passing raw pointers over passing the reference of |nsTArray| regarding to
> these conversion utilities (bytes <==> UUID)?

It was just an idea to make the code a bit more flexible. |nsTArray| should be fine as well. :)

Comment 29

[Bruce Sun [:brsun]]

> It was just an idea to make the code a bit more flexible. |nsTArray| should
> be fine as well. :)

Got it. Thanks for the comments. :)

Comment 30

[Bruce Sun [:brsun]]

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

Comment 31

[Bruce Sun [:brsun]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/807683706e4ed63cd348a3c748bcc028cb4c3d3e
Bug 1222956: Use |BluetoothUuid| in |BluetoothDiscoveryHandle|, r=tzimmermann

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/807683706e4e