[Bluetooth][Daemon] Correct the data type of |Channel| with |int32_t| in Bluetooth Socket HAL.

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.2 S5 (6feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
This bug is originated from [1]. The BlueZ doc says |Channel| is 2 octets, but it is |int32_t| inside source codes of BlueZ. See also: [2][3].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c15
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c17
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c18
Assignee

Comment 1

4 years ago
Attachment #8558975 - Flags: review?(tzimmermann)
Comment on attachment 8558975 [details] [diff] [review]
bug1129257_bluetoothd_socket_channel_int32_t.patch

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

r+ with the comments addressed.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +72,5 @@
>  
>  nsresult
> +Convert(int aIn, int32_t& aOut)
> +{
> +  if (NS_WARN_IF(!mozilla::detail::IsInRange<int32_t>(aIn))) {

The rest of the file uses C++' 'std::limit'. If there's no coding style requirement, please use limit as well.

@@ +81,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +Convert(int aIn, uint32_t& aOut)

Please remove this function after changing BluetoothDaemonSocketInterface.

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +35,5 @@
>    nsresult rv = PackPDU(
>      aType,
>      PackConversion<nsAString, BluetoothServiceName>(aServiceName),
>      PackArray<uint8_t>(aServiceUuid, 16),
> +    PackConversion<int, uint32_t>(aChannel),

This should be |int32_t| as well. Using 'unsigned' is a bug in the current code.
Attachment #8558975 - Flags: review?(tzimmermann) → review+
Assignee

Comment 5

4 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Comment on attachment 8558975 [details] [diff] [review]
> bug1129257_bluetoothd_socket_channel_int32_t.patch
> 
> Review of attachment 8558975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comments addressed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +72,5 @@
> >  
> >  nsresult
> > +Convert(int aIn, int32_t& aOut)
> > +{
> > +  if (NS_WARN_IF(!mozilla::detail::IsInRange<int32_t>(aIn))) {
> 
> The rest of the file uses C++' 'std::limit'. If there's no coding style
> requirement, please use limit as well.
> 

There will be one compile warning while comparing a signed integer value with a unsigned integer value by using original coding style, and this warning will result a compile error if warning-as-error is on.
Assignee

Comment 6

4 years ago
(In reply to Bruce Sun [:brsun] from comment #5)
> There will be one compile warning while comparing a signed integer value
> with a unsigned integer value by using original coding style, and this
> warning will result a compile error if warning-as-error is on.

BTW, suppose we can avoid this issue if we don't convert between |int| and |uint32_t|. I'll give it a try later.
Assignee

Comment 7

4 years ago
Address suggestions on comment 3.
Attachment #8558975 - Attachment is obsolete: true
Attachment #8559015 - Flags: review+
Assignee

Updated

4 years ago
blocking-b2g: --- → 2.2?
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 9

4 years ago
(In reply to Bruce Sun [:brsun] from comment #8)
> Waiting for the result of TBPL:
>  - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2525ee84e226
>  - https://tbpl.mozilla.org/?tree=Try&rev=2525ee84e226

TBPL results seem good.
Bruce,

are you aware that all your fixes need to go into bluetooth2/ as well?
Assignee

Comment 11

4 years ago
Yes, I know that...I just don't have the courage to handle those stuffs yet...

By the way, I would suggest to handle bluetooth2 after bluetooth1 is stable and has been verified properly. We have no ways to make sure the porting tasks from bluetooth1 to bluetooth2 are properly handled in the current stage.
Assignee

Updated

4 years ago
Blocks: 1129846
(In reply to Bruce Sun [:brsun] from comment #11)
> Yes, I know that...I just don't have the courage to handle those stuffs
> yet...
> 
> By the way, I would suggest to handle bluetooth2 after bluetooth1 is stable
> and has been verified properly. We have no ways to make sure the porting
> tasks from bluetooth1 to bluetooth2 are properly handled in the current
> stage.

Yeah, I know. :( The situation around bluetooth2 is dire and I expect regressions left and right, once it gets merged officially.

I wrote all of the current backend code for HAL and bluetoothd and have been porting it 'blindly' to bluetooth2 by renaming file paths in the patch files and compiling the results. I'm glad that the backend code doesn't conflict much with the rest of the bluetooth2 changes.

And there is no way to really test anything in bluetooth2.
Assignee

Updated

4 years ago
Duplicate of this bug: 1124556

Comment 15

4 years ago
Triage: blocking
blocking-b2g: 2.2? → 2.2+
Assignee

Comment 16

4 years ago
Comment on attachment 8559015 [details] [diff] [review]
bug1129257_bluetoothd_socket_channel_int32_t.commit.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1124556
User impact if declined: The behavior of device connection triggered by NFC is wrong.
Testing completed: Manually test locally.
Risk to taking this patch (and alternatives if risky): Low. But it's worth noting that |libxul.so| and |bluetoothd| are needed to be updated concurrently. File transfer functions will break if only one side (gecko or bluetoothd) has been updated without corresponding update on the other side (bluetoothd or gecko).
String or UUID changes made by this patch: N/A
Attachment #8559015 - Flags: approval-mozilla-b2g37?
Attachment #8558976 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559015 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
This was merged a few days ago and got missed.

https://hg.mozilla.org/mozilla-central/rev/38d3cf148195
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.