1168343 - [bluetooth2] Assertion failure 'MOZ_ASSERT(nameExists)' occurs at BluetoothServiceBluedroid.cpp during Bluetooth legacy pairing.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

BondStateChangedNotification() would hit a MOZ_ASSERT failure during legacy pairing in B2G DEBUG build. 

Program received signal SIGSEGV, Segmentation fault.
0xb46f810a in mozilla::dom::bluetooth::BluetoothServiceBluedroid::BondStateChangedNotification (this=0xb023f200, aStatus=<optimized out>, aRemoteBdAddr=..., aState=<optimized out>)
    at ../../../dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp:3002
3002	    MOZ_ASSERT(nameExists);
(gdb) bt
#0  0xb46f810a in mozilla::dom::bluetooth::BluetoothServiceBluedroid::BondStateChangedNotification (this=0xb023f200, aStatus=<optimized out>, aRemoteBdAddr=..., aState=<optimized out>)
    at ../../../dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp:3002
#1  0xb46ec7ae in mozilla::dom::bluetooth::BluetoothNotificationHALRunnable3<mozilla::dom::bluetooth::BluetoothCallback::NotificationHandlerWrapper, void, mozilla::dom::bluetooth::BluetoothStatus, nsString, mozilla::dom::bluetooth::BluetoothBondState, mozilla::dom::bluetooth::BluetoothStatus, nsAString_internal const&, mozilla::dom::bluetooth::BluetoothBondState>::Run (this=0xa9e5e520)
    at ../../../dom/bluetooth/bluedroid/BluetoothHALHelpers.h:1384
#2  0xb3fe7be0 in nsThread::ProcessNextEvent (this=0xb6a50320, aMayWait=<optimized out>, aResult=0xbecfe80f) at ../../../xpcom/threads/nsThread.cpp:866
#3  0xb3ffe278 in NS_ProcessNextEvent (aThread=0xb6a50320, aMayWait=aMayWait@entry=true) at /home/jamin/Projects/B2G_Flame/gecko/xpcom/glue/nsThreadUtils.cpp:265
#4  0xb41ba826 in mozilla::ipc::MessagePump::Run (this=0xb6a67190, aDelegate=0xb6a811a0) at ../../../ipc/glue/MessagePump.cpp:127
#5  0xb41a16cc in MessageLoop::RunInternal (this=this@entry=0xb6a811a0) at ../../../ipc/chromium/src/base/message_loop.cc:233

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Remove the improper MOZ_ASSERT in |BluetoothServiceBluedroid::BondStateChangedNotification| (v1)

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8610528 [details] [diff] [review]
Remove the improper MOZ_ASSERT in |BluetoothServiceBluedroid::BondStateChangedNotification| (v1)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2881,5 @@
>      // "a valid Bluetooth name is a UTF-8 encoding string which is up to 248
>      // bytes in length."
> +    // Furthermore, we don't assert |nameExists| here since it's expected to be
> +    // 'false' if remote device only supports legacy pairing.
> +

Hmm,
 bool nameExists = sPairingNameTable.Get(aRemoteBdAddr, &deviceName);
I don't under why this is related to legacy pairing. Can you explain further in the bug?

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> Comment on attachment 8610528 [details] [diff] [review]
> Remove the improper MOZ_ASSERT in
> |BluetoothServiceBluedroid::BondStateChangedNotification| (v1)
> 
> Review of attachment 8610528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +2881,5 @@
> >      // "a valid Bluetooth name is a UTF-8 encoding string which is up to 248
> >      // bytes in length."
> > +    // Furthermore, we don't assert |nameExists| here since it's expected to be
> > +    // 'false' if remote device only supports legacy pairing.
> > +
> 
> Hmm,
>  bool nameExists = sPairingNameTable.Get(aRemoteBdAddr, &deviceName);
> I don't under why this is related to legacy pairing. Can you explain further
> in the bug?

Oh, I see. I finally understand that this is a follow-up bug for bug 1120842. I think bug 1120842 is a case of 'just work', so you won't see pin request or ssp request. I think you should modify your comments based on the fact checking nameExist here is probably not suitable for SSP just work cases.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Remove the improper MOZ_ASSERT in |BluetoothServiceBluedroid::BondStateChangedNotification| (v2)

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
I think bug 1120842 is a case of 'just work', so you won't see pin
> request or ssp request. I think you should modify your comments based on the
> fact checking nameExist here is probably not suitable for SSP just work
> cases.

Thank you for your helpful feedback.
I've updated the comment based on your feedback.

Per offline discussion, |nameExists| would be 'false' if BT backend is using "just works without user interaction" or "legacy pairing with auto-pairing".
I think Bug 1120842 is the 2nd case, however, I believe we can treat these two cases as the same one since both of them wouldn't receive |PinRequestNotification| and |SspRequestNotification|.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8610994 [details] [diff] [review]
Remove the improper MOZ_ASSERT in |BluetoothServiceBluedroid::BondStateChangedNotification| (v2)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2880,5 @@
>      // also a valid name. According to Bluetooth Core Spec. v3.0 - Sec. 6.22,
>      // "a valid Bluetooth name is a UTF-8 encoding string which is up to 248
>      // bytes in length."
> +    // Furthermore, we don't assert |nameExists| here since it's expected to be
> +    // 'false' if remote device is using "just works without user interaction"

supernit: s/just works/SSP just works/

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Remove the improper MOZ_ASSERT in |BluetoothServiceBluedroid::BondStateChangedNotification| (v3), r=shuang

- Revise previous patch based on #comment 6
- Add reviewer's name to commit message

Thank Shawn for reviewing the patch.

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Skip the "Pushing to Try" procedure since Bluetooth API v2 hasn't be covered by tinderbox.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/bbc0e7cabc76

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/bbc0e7cabc76