Closed Bug 1125719 Opened 9 years ago Closed 9 years ago

[Bluetooth] Redial from bluetooth device cause bluetoothd and gecko crash.

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S5 (6feb)
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: brsun, Assigned: tzimmermann)

Details

Attachments

(2 files, 1 obsolete file)

Github pull request
421 bytes, patch
shawnjohnjr
: review+
brsun
: feedback+
bajaj
: approval-gaia-v2.2+
Details | Diff | Splinter Review
bug1125719_bluetoothd_redial_crash.checkin.patch
1.46 KB, patch
brsun
: review+
bajaj
: approval-mozilla-b2g37+
Details | Diff | Splinter Review 
The scenario can refer [1] for more details.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1124760#c14
Gecko call stacks:

Program received signal SIGSEGV, Segmentation fault.
0xb6e8b6ea in memmove (dst0=0xb6578efc <mozilla::CountingAllocatorBase<NesteggReporter>::sAmount>, src0=<optimized out>, length=4294967292) at bionic/libc/upstream-openbsd/lib/libc/string/bcopy.c:97
97			TLOOP(*(word *)dst = *(word *)src; src += wsize; dst += wsize);
(gdb) bt
#0  0xb6e8b6ea in memmove (dst0=0xb6578efc <mozilla::CountingAllocatorBase<NesteggReporter>::sAmount>, src0=<optimized out>, length=4294967292) at bionic/libc/upstream-openbsd/lib/libc/string/bcopy.c:97
#1  0xb56148c2 in nsTArray_Impl<nsRefPtr<mozilla::dom::bluetooth::BluetoothResultHandler>, nsTArrayInfallibleAllocator>::RemoveElementsAt (this=this@entry=0xb286ab20, aStart=aStart@entry=0, 
    aCount=aCount@entry=1) at ../../dist/include/nsTArray.h:1398
#2  0xb56148f2 in RemoveElementAt (aIndex=0, this=0xb286ab20) at ../../dist/include/nsTArray.h:1403
#3  mozilla::dom::bluetooth::BluetoothDaemonInterface::OnDisconnect (this=0xb286ab00, aChannel=<optimized out>)
    at ../../../../mozilla-central-working/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1909
#4  0xb4e8b668 in mozilla::ipc::SocketBase::NotifyDisconnect (this=this@entry=0xa8a29620) at ../../../../mozilla-central-working/ipc/unixsocket/SocketBase.cpp:221
#5  0xb4e8b0f2 in mozilla::ipc::ListenSocket::Close (this=0xa8a29620) at ../../../../mozilla-central-working/ipc/unixsocket/ListenSocket.cpp:380
#6  0xb56148d8 in mozilla::dom::bluetooth::BluetoothDaemonInterface::OnDisconnect (this=0xb286ab00, aChannel=<optimized out>)
    at ../../../../mozilla-central-working/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1905
#7  0xb4e8b668 in mozilla::ipc::SocketBase::NotifyDisconnect (this=this@entry=0xa8036280) at ../../../../mozilla-central-working/ipc/unixsocket/SocketBase.cpp:221
#8  0xb4e8a386 in mozilla::ipc::BluetoothDaemonConnection::CloseSocket (this=0xa8036280) at ../../../../mozilla-central-working/ipc/bluetooth/BluetoothDaemonConnection.cpp:577
#9  0xb4e8a212 in mozilla::ipc::SocketIORequestClosingRunnable<mozilla::ipc::BluetoothDaemonConnectionIO>::Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/SocketBase.h:438
#10 0xb4ca9646 in nsThread::ProcessNextEvent (this=0xb67025c0, aMayWait=<optimized out>, aResult=0xbed92647) at ../../../../mozilla-central-working/xpcom/threads/nsThread.cpp:855
#11 0xb4cb7028 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at /home/bruce_sun/Source/mozilla-central-working/xpcom/glue/nsThreadUtils.cpp:265
#12 0xb4daa020 in mozilla::ipc::MessagePump::Run (this=0xb6781190, aDelegate=0xb676d1a0) at ../../../../mozilla-central-working/ipc/glue/MessagePump.cpp:99
#13 0xb4da0370 in MessageLoop::RunInternal (this=this@entry=0xb676d1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:233
#14 0xb4da0424 in RunHandler (this=0xb676d1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:226
#15 MessageLoop::Run (this=0xb676d1a0) at ../../../../mozilla-central-working/ipc/chromium/src/base/message_loop.cc:200
#16 0xb5626636 in nsBaseAppShell::Run (this=0xb2a5b460) at ../../../mozilla-central-working/widget/nsBaseAppShell.cpp:164
#17 0xb58b70ea in nsAppStartup::Run (this=0xb2a61340) at ../../../../../mozilla-central-working/toolkit/components/startup/nsAppStartup.cpp:281
#18 0xb58cd070 in XREMain::XRE_mainRun (this=this@entry=0xbed927b8) at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4160
#19 0xb58cd2a2 in XREMain::XRE_main (this=this@entry=0xbed927b8, argc=argc@entry=1, argv=argv@entry=0xb672e188, aAppData=aAppData@entry=0xb6f82838 <_ZL8sAppData>)
    at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4236
#20 0xb58cd44a in XRE_main (argc=1, argv=0xb672e188, aAppData=0xb6f82838 <_ZL8sAppData>, aFlags=<optimized out>) at ../../../../mozilla-central-working/toolkit/xre/nsAppRunner.cpp:4456
#21 0xb6f67970 in do_main (argc=argc@entry=1, argv=argv@entry=0xb672e188) at ../../../../mozilla-central-working/b2g/app/nsBrowserApp.cpp:167
#22 0xb6f67abc in b2g_main (argc=1, argv=<optimized out>) at ../../../../mozilla-central-working/b2g/app/nsBrowserApp.cpp:299
#23 0xb6f67818 in RunProcesses (aReservedFds=..., argv=0xbed93a74, argc=1) at ../../../../mozilla-central-working/b2g/app/B2GLoader.cpp:225
#24 main (argc=1, argv=0xbed93a74) at ../../../../mozilla-central-working/b2g/app/B2GLoader.cpp:290
bluetoothd call stacks:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2385.2399]
mainloop () at bionic/libc/arch-arm/cortex-a15/bionic/strlen.S:100
100	    ldrd    r2, r3, [r1], #8
(gdb) bt
#0  mainloop () at bionic/libc/arch-arm/cortex-a15/bionic/strlen.S:100
#1  0xb6faec44 in strlen (s=0x0) at bionic/libc/include/string.h:239
#2  dial_call_cmd_cb (number=0x0, bd_addr=0xb6b1601a <btif_hf_cb+2>) at system/bluetoothd/src/bt-hf-io.c:259
#3  0xb6a28922 in btif_hf_upstreams_evt (event=<optimized out>, p_param=<optimized out>) at external/bluetooth/bluedroid/main/../btif/src/btif_hf.c:614
#4  0xb6a19c8a in btif_context_switched (p_msg=0xb5b0bc68) at external/bluetooth/bluedroid/main/../btif/src/btif_core.c:181
#5  btif_task (params=<optimized out>) at external/bluetooth/bluedroid/main/../btif/src/btif_core.c:333
#6  0xb6a7254a in gki_task_entry (params=3066029332) at external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:263
#7  0xb6f382e4 in __pthread_start (arg=0xb6c35580, arg@entry=<error reading variable: value has been optimized out>) at bionic/libc/bionic/pthread_create.cpp:141
#8  0xb6f362d4 in __start_thread (fn=<optimized out>, arg=<optimized out>) at bionic/libc/bionic/clone.cpp:41
#9  0x00000000 in ?? ()
This basically is a crash issue of bluetoothd, but Gecko suffers from bug 1115656 so b2g crashes as well.

This issue occurs on my nexus-5-l environment, but doesn't happens on my flame-kk environment.
(In reply to Bruce Sun [:brsun] from comment #3)
> This issue occurs on my nexus-5-l environment, but doesn't happens on my
> flame-kk environment.

It turns out that I forget to cleanup |out/target/product/flame/system/build.prop| on my environment, so that |ro.moz.bluetooth.backend=bluedroid| exists in |system/build.prop| on my flame device.
(In reply to Bruce Sun [:brsun] from comment #4)
> (In reply to Bruce Sun [:brsun] from comment #3)
> > This issue occurs on my nexus-5-l environment, but doesn't happens on my
> > flame-kk environment.
> 
> It turns out that I forget to cleanup
> |out/target/product/flame/system/build.prop| on my environment, so that
> |ro.moz.bluetooth.backend=bluedroid| exists in |system/build.prop| on my
> flame device.

My flame crashes as well.
(In reply to Bruce Sun [:brsun] from comment #2)
> bluetoothd call stacks:
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 2385.2399]
> mainloop () at bionic/libc/arch-arm/cortex-a15/bionic/strlen.S:100
> 100	    ldrd    r2, r3, [r1], #8
> (gdb) bt
> #0  mainloop () at bionic/libc/arch-arm/cortex-a15/bionic/strlen.S:100
> #1  0xb6faec44 in strlen (s=0x0) at bionic/libc/include/string.h:239
> #2  dial_call_cmd_cb (number=0x0, bd_addr=0xb6b1601a <btif_hf_cb+2>) at
> system/bluetoothd/src/bt-hf-io.c:259

'number' is NULL. When does that happen?

> #3  0xb6a28922 in btif_hf_upstreams_evt (event=<optimized out>,
> p_param=<optimized out>) at
> external/bluetooth/bluedroid/main/../btif/src/btif_hf.c:614
> #4  0xb6a19c8a in btif_context_switched (p_msg=0xb5b0bc68) at
> external/bluetooth/bluedroid/main/../btif/src/btif_core.c:181
> #5  btif_task (params=<optimized out>) at
> external/bluetooth/bluedroid/main/../btif/src/btif_core.c:333
> #6  0xb6a7254a in gki_task_entry (params=3066029332) at
> external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:263
> #7  0xb6f382e4 in __pthread_start (arg=0xb6c35580, arg@entry=<error reading
> variable: value has been optimized out>) at
> bionic/libc/bionic/pthread_create.cpp:141
> #8  0xb6f362d4 in __start_thread (fn=<optimized out>, arg=<optimized out>)
> at bionic/libc/bionic/clone.cpp:41
> #9  0x00000000 in ?? ()
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
NULL means redial [1]:

/** Callback for dialing an outgoing call
 * If number is NULL, redial
 */
typedef void (* bthf_dial_call_cmd_callback)(char *number, bt_bdaddr_t *bd_addr);

[1] https://source.android.com/devices/halref/bt__hf_8h_source.html
This should fix the crash, but I'm not sure if Gecko interprets this as 'redial.' Bruce, can you give it a try?
Attachment #8554425 - Flags: feedback?(brsun)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> Created attachment 8554425 [details] [diff] [review]
> Github pull request
> 
> This should fix the crash, but I'm not sure if Gecko interprets this as
> 'redial.' Bruce, can you give it a try?

I've tried a similar patch locally, but redial function doesn't work. I am trying to digging into it.
OK, thanks! It's probably just that Gecko doesn't interpret the string correctly. I suggest to look into the Notification handlers in the Handsfree manager.
Do you want this bug?
Attachment 8554425 [details] [diff] can fix the crash issue in bluetoothd.

This patch can fix the redial issue as stated on comment 9.
Attachment #8554450 - Flags: review?(tzimmermann)
Comment on attachment 8554425 [details] [diff] [review]
Github pull request

This patch can avoid the crash issue. :)
Attachment #8554425 - Flags: feedback?(brsun) → feedback+
Comment on attachment 8554425 [details] [diff] [review]
Github pull request

BTW, you can simply paste PR url as plain text attachment instead of html. Current redirection html code is not working
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Do you want this bug?

Well...let's share this bug.
Comment on attachment 8554450 [details] [diff] [review]
bug1125719_bluetoothd_redial_crash.patch

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

I don't understand this patch. Can you explain what is does?

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +1599,5 @@
>    if (NS_WARN_IF(!str)) {
>      return NS_ERROR_ILLEGAL_VALUE; // end of PDU
>    }
>  
> +  const char* end = static_cast<char*>(memchr(str, '\0', aPDU.GetSize() + 1));

This reads beyond the end of the PDU, so we probably can't do that.

@@ +1604,5 @@
>    if (NS_WARN_IF(!end)) {
>      return NS_ERROR_ILLEGAL_VALUE; // no string terminator
>    }
>  
>    ptrdiff_t len = end - str;

What about 'len = end - str + 1'? That would include the final '\0' in the length without reading beyond the end of the PDU.
Attachment #8554450 - Flags: feedback?(brsun)
Because there is |aPDU.Consume(1)| at the beginning of |UnpackPDU()|, |aPDU.GetSize()| becomes 1 less than the actual data length we want to parse. So |memchr()| will always fail by missing the final character by using |aPDU.GetSize()|.

ex: "1234567890" contains in PDU while receiving callback:
step 0: |UnpackPDU()| enters
 - |aPDU.GetSize()| is 11 (there is one character for the trailing |'\0'|).
step 1: |aPDU.Consume(1)|
 - |str| points to '1'.
 - |aPDU.GetSize()| is 10.
step 2: |memchr(str, '\0', aPDU.GetSize())|
 - |memchr()| never finds |'\0'| character because |'\0'| is not within the lookup range, 10, from |str|.
Flags: needinfo?(tzimmermann)
Comment on attachment 8554450 [details] [diff] [review]
bug1125719_bluetoothd_redial_crash.patch

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

Oh that makes sense! Classical off-by-one. Thanks for fixing this bug.
Attachment #8554450 - Flags: review?(tzimmermann)
Attachment #8554450 - Flags: review+
Attachment #8554450 - Flags: feedback?(brsun)
Flags: needinfo?(tzimmermann)
TBPL results seem good.
blocking-b2g: --- → 2.2?
Keywords: checkin-needed
Comment on attachment 8554482 [details] [diff] [review]
bug1125719_bluetoothd_redial_crash.checkin.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 1091588, Bug 1087195
User impact if declined: Not able to perform redial through the Bluetooth device.
Testing completed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21bfb41d57cb
Risk to taking this patch (and alternatives if risky): Low. This fixes crash issues by simply correcting misbehaved codes.
String or UUID changes made by this patch: N/A
Attachment #8554482 - Flags: approval-mozilla-b2g37?
Comment on attachment 8554425 [details] [diff] [review]
Github pull request

Kindly help refer to comment 21 as the request comment. Thanks.
Attachment #8554425 - Flags: approval-mozilla-b2g37?
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8554425 [details] [diff] [review]
Github pull request

Switching the approval to gaia.
Attachment #8554425 - Flags: approval-mozilla-b2g37? → approval-gaia-v2.2+
Attachment #8554482 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/mozilla-central/rev/88f85a8099ca
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
You need to log in before you can comment on or make changes to this bug.