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

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.

Comment on attachment 8558976 [details] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/19 r+ with the 'unsigned' fixed.

(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.

(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.

Address suggestions on comment 3.

Waiting for the result of TBPL: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2525ee84e226 - https://tbpl.mozilla.org/?tree=Try&rev=2525ee84e226

(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?

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.

(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.

https://hg.mozilla.org/integration/b2g-inbound/rev/38d3cf148195 Master: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/798a1844533b05b652674e738a00304dfe786603

Triage: blocking

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

Comment on attachment 8558976 [details] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/19 The same as comment 16. Thanks.

This was merged a few days ago and got missed. https://hg.mozilla.org/mozilla-central/rev/38d3cf148195

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e80722a87475 v2.2: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/29dcf992a6ebac3c49248a73c59a4086c84b75b6