1186331 - [Aries-l] Pairing device name sometimes has extra character at the end

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Attachment: extra-device-name-character-pairing-screen

+++ This bug was initially created as a clone of Bug #1181827 +++

Sometimes the displayed pairing device name has extra character at the end (as attached picture). Note the bug is reproduced with bug 1181827 fix.

Repro step:
  1) device A (Aries-L) initiates pairing to device B.
  2) confirm pairing on both devices, and devices A & B are paired.
  3) unpair each other on both devices A & B
  4) device B initiates pairing to device A
  5) device A would see extra character at the end of device B's name on pairing screen

Repro rate:
  High, ~80%

Attachment:
  symptom picture. The actual device name is "Nexus 4 xyz".

Comment 1

[Ben Tian (inactive)]

Attachment: remote-name.log

Attached log file shows there's always a non-zero character at the end of |BluetoothRemoteName.mName| from bluetoothd. The character appears on pairing screen if it's display-able.

Thomas, do you have any idea on this bug? Note I'm unsure whether this is L-only or a general bug.

===
I GeckoBluetooth: Convert: BluetoothRemoteName[0] = [78](N)
I GeckoBluetooth: Convert: BluetoothRemoteName[1] = [101](e)
I GeckoBluetooth: Convert: BluetoothRemoteName[2] = [120](x)
I GeckoBluetooth: Convert: BluetoothRemoteName[3] = [117](u)
I GeckoBluetooth: Convert: BluetoothRemoteName[4] = [115](s)
I GeckoBluetooth: Convert: BluetoothRemoteName[5] = [32]( )
I GeckoBluetooth: Convert: BluetoothRemoteName[6] = [52](4)
I GeckoBluetooth: Convert: BluetoothRemoteName[7] = [32]( )
I GeckoBluetooth: Convert: BluetoothRemoteName[8] = [120](x)
I GeckoBluetooth: Convert: BluetoothRemoteName[9] = [121](y)
I GeckoBluetooth: Convert: BluetoothRemoteName[10] = [122](z)
I GeckoBluetooth: Convert: BluetoothRemoteName[248] = [45](-)
I GeckoBluetooth: SspRequestNotification: name [Nexus 4 xyz]

Comment 2

[Ben Tian (inactive)]

Attachment: remotename.patch

Also attach the log patch I use.

Comment 3

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

The extra character in the example is '-', right?

Maybe there's a problem with the encoded message from bluetoothd. I'll check...

Comment 4

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

Strange. I have not yet seen this problem, not even on nexus-5-l.

The name's length is always 249 characters (as specified by Android headers). Bluetoothd copies all bytes into the messages and Gecko copies them out. If the name is shorter than 249 characters, there has to be an \0 character.

To me this looks like an off-by-one error in the Bluedroid driver on Aries, where the \0 character comes one byte to late. I'll create an Aries build and investigate a bit further.

Comment 5

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> The extra character in the example is '-', right?
> 
> Maybe there's a problem with the encoded message from bluetoothd. I'll
> check...

It's random character. You can see it's 'N' in comment 0 picture and 194 in the log file as well.

Comment 6

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> To me this looks like an off-by-one error in the Bluedroid driver on Aries,
> where the \0 character comes one byte to late. I'll create an Aries build
> and investigate a bit further.

I see. Thanks for help.

Note I'm still fixing bug 1181827 so the device name is always "Unknown Device", but you can still observe the string from logs.

Comment 7

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #6)
> Note I'm still fixing bug 1181827 so the device name is always "Unknown

'the device name shown on pairing screen'

Comment 8

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> The name's length is always 249 characters (as specified by Android
> headers). Bluetoothd copies all bytes into the messages and Gecko copies
> them out. If the name is shorter than 249 characters, there has to be an \0
> character.

The extra character always appears at index 248 of the string. Take comment 1 logs as example,

index [0 ~ 10]: real device name "Nexus 5 xyz"
index [11 ~ 247]: \0 character
index [248]: the random extra character

===
I GeckoBluetooth: Convert: BluetoothRemoteName[0] = [78](N)
I GeckoBluetooth: Convert: BluetoothRemoteName[1] = [101](e)
I GeckoBluetooth: Convert: BluetoothRemoteName[2] = [120](x)
I GeckoBluetooth: Convert: BluetoothRemoteName[3] = [117](u)
I GeckoBluetooth: Convert: BluetoothRemoteName[4] = [115](s)
I GeckoBluetooth: Convert: BluetoothRemoteName[5] = [32]( )
I GeckoBluetooth: Convert: BluetoothRemoteName[6] = [52](4)
I GeckoBluetooth: Convert: BluetoothRemoteName[7] = [32]( )
I GeckoBluetooth: Convert: BluetoothRemoteName[8] = [120](x)
I GeckoBluetooth: Convert: BluetoothRemoteName[9] = [121](y)
I GeckoBluetooth: Convert: BluetoothRemoteName[10] = [122](z)
I GeckoBluetooth: Convert: BluetoothRemoteName[248] = [45](-)

Comment 9

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

Right, I didn't notice the index. I'm surprised that the intermediate \0 characters get removed or ignored.

We can certainly fix this by searching explicitly for \0 in the Convert method at [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#954

Comment 10

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

Attachment: [01] Bug 1186331: Check for \0 when parsing Bluetooth device names

This took a bit longer, because I had to setup the aries-l build first.

I cannot even pair devices without this patch, because |NS_ConvertUTF8toUTF16| complains about the input not being UTF-8. The patch fixes this.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8637882 [details] [diff] [review]
[01] Bug 1186331: Check for \0 when parsing Bluetooth device names

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

LGTM. Thanks for the help.

Comment 12

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

Thanks!

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

Comment 13

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

Comment on attachment 8637882 [details] [diff] [review]
[01] Bug 1186331: Check for \0 when parsing Bluetooth device names

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

  Bluetooth daemon that was introduced in v2.2

User impact if declined: 

  Bluetooth pairing might not work. (Depends on HW and drivers.)

Testing completed: 

  Tested locally on aries-l. Device pairing only works with the patch.

Risk to taking this patch (and alternatives if risky): 

  Very low. The patch makes a simple change to a string-handling function within BT.

String or UUID changes made by this patch:

  None.

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/5d1ccf26d9e1

Comment 15

[Josh Cheng [:josh]]

Hi Norry,
Please verify on master before approving for 2.2. Thanks!

Comment 16

[Shally Li（leave from Mozilla）]

Hi Josh,

    This bug has been verified as "pass" on the latest nightly build of Nexus-L-5 master by the STR in comment 0.

Actual results: When pairing device, there is no any extra character at the end of the name on the Nexus-L-5 master.
See attachment: verified_Nexus5-L-Master.3gp (left is Nexus-L-5 master and right is Nexus-L-5 2.2)
Reproduce rate: 0/15


Device: Nexus-L-5 master (Pass)
Gaia-Rev        862f0895f3f5a97200601542d99a152a46385a0b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/833403badc41
Build-ID        20150728150207
Version         42.0a1
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150728.182857
FW-Date         Tue Jul 28 18:29:15 EDT 2015
Bootloader      HHZ12f

Note:
The bug can't be repro on Flame kk v2.2&master, so we don't verify it on Flame kk master.

----------------------------------------------
Leaving 'verifyme' for firefox42 verification.

Comment 17

[Shally Li（leave from Mozilla）]

Attachment: verified_Nexus5-L-Master.3gp

Comment 18

[Josh Cheng [:josh]]

Hi Gerry, Teri
Could you please help to check per our offline discussion?
Thank you!

Comment 19

[Teri Wen [:twen]]

Thanks Ben for providing the build. After running about 20x of pairing and renaming both devices, I did not find any extra character.

Gaia-Rev        ef50518c87bde1e005ac0154ef58e8c2770950f1
Gecko-Rev       
Build-ID        20150730153811
Version         42.0a1
Device-Name     aries
FW-Release      5.1
FW-Incremental  eng.bentian.20150722.120123
FW-Date         三  7月 22 12:01:35 CST 2015
Bootloader      s1

Comment 20

[Josh Cheng [:josh]]

Comment on attachment 8637882 [details] [diff] [review]
[01] Bug 1186331: Check for \0 when parsing Bluetooth device names

Thanks, Teri.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f15bd4bdff6e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/f15bd4bdff6e

Comment 23

[Sxean Lee-David [:sxean]]

Requires an Aries-L to verify.