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

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ben.tian, Assigned: tzimmermann)

Tracking

({verifyme})

unspecified
FxOS-S3 (24Jul)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox40 wontfix, firefox41 wontfix, firefox42 fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master verified)

Details

Attachments

(5 attachments)

(Reporter)

Description

4 years ago
+++ 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".
(Reporter)

Updated

4 years ago
Keywords: regression
Whiteboard: [2.5-Daily-Testing], [Spark], [aries-l-support]
(Reporter)

Comment 1

4 years ago
Posted file 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]
Flags: needinfo?(tzimmermann)
(Reporter)

Comment 2

4 years ago
Also attach the log patch I use.
The extra character in the example is '-', right?

Maybe there's a problem with the encoded message from bluetoothd. I'll check...
Flags: needinfo?(tzimmermann)
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.
(Reporter)

Comment 5

4 years ago
(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.
(Reporter)

Comment 6

4 years ago
(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.
(Reporter)

Comment 7

4 years ago
(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'
(Reporter)

Comment 8

4 years ago
(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](-)
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
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
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.
Attachment #8637882 - Flags: review?(btian)
(Reporter)

Comment 11

4 years ago
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.
Attachment #8637882 - Flags: review?(btian) → review+
blocking-b2g: --- → 2.2?
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.
Attachment #8637882 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/5d1ccf26d9e1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Hi Norry,
Please verify on master before approving for 2.2. Thanks!
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(fan.luo)
Keywords: verifyme
Target Milestone: --- → FxOS-S3 (24Jul)
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.
Flags: needinfo?(fan.luo)
Flags: needinfo?(jocheng)
QA Whiteboard: [MGSEI-Triage+]
Hi Gerry, Teri
Could you please help to check per our offline discussion?
Thank you!
Flags: needinfo?(twen)
Flags: needinfo?(jocheng)
Flags: needinfo?(gchang)

Comment 19

4 years ago
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
Flags: needinfo?(twen)
Flags: needinfo?(gchang)
Comment on attachment 8637882 [details] [diff] [review]
[01] Bug 1186331: Check for \0 when parsing Bluetooth device names

Thanks, Teri.
Attachment #8637882 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Requires an Aries-L to verify.
QA Whiteboard: [MGSEI-Triage+] → [MGSEI-Triage+][QAExclude]
Flags: needinfo?(jmercado)
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.