1132229 - b2g process crashes when bluetoothd is killed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

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

STR:
0. Load a recent b2g v2.2 L-based build.
1. Attach gdb to the b2g main process.
2. Get the bluetoothd process - adb shell ps bluetoothd
3. Kill the bluetoothd process - adb shell kill /pid/
4. Watch the b2g process crash.

Backtrace:
Program received signal SIGSEGV, Segmentation fault.
0xb6e783a2 in memmove (dst0=0xb667b73c <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  0xb6e783a2 in memmove (dst0=0xb667b73c <mozilla::CountingAllocatorBase<NesteggReporter>::sAmount>,
    src0=<optimized out>, length=4294967292) at bionic/libc/upstream-openbsd/lib/libc/string/bcopy.c:97
#1  0xb570f2de in nsTArray_Impl<nsRefPtr<mozilla::dom::bluetooth::BluetoothResultHandler>, nsTArrayInfallibleAllocator>:
:RemoveElementsAt (this=this@entry=0xb29b3520, aStart=aStart@entry=0, aCount=aCount@entry=1)
    at ../../dist/include/nsTArray.h:1398
#2  0xb570f30e in RemoveElementAt (aIndex=0, this=0xb29b3520) at ../../dist/include/nsTArray.h:1403
#3  mozilla::dom::bluetooth::BluetoothDaemonInterface::OnDisconnect (this=0xb29b3500, aChannel=<optimized out>)
    at /local/mnt/buildbot/sink3/build/LF.BR.1.2.3/gecko/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1909
#4  0xb4f9c2e4 in mozilla::ipc::SocketBase::NotifyDisconnect (this=this@entry=0xaeb26d80)
    at /local/mnt/buildbot/sink3/build/LF.BR.1.2.3/gecko/ipc/unixsocket/SocketBase.cpp:221
#5  0xb4f9bd6e in mozilla::ipc::ListenSocket::Close (this=0xaeb26d80)
    at /local/mnt/buildbot/sink3/build/LF.BR.1.2.3/gecko/ipc/unixsocket/ListenSocket.cpp:380
#6  0xb570f2f4 in mozilla::dom::bluetooth::BluetoothDaemonInterface::OnDisconnect (this=0xb29b3500,
    aChannel=<optimized out>)
    at /local/mnt/buildbot/sink3/build/LF.BR.1.2.3/gecko/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1905

Interestingly if gdb is not attached the b2g process silently restarts and no minidump is produced in this case.  Perhaps this is a different bug?

Comment 1

[Michael Vines [:m1] [:evilmachines]]

(this reproduces on KK builds as well)

Comment 2

[howie [:howie]]

Triage: blocking

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Can you please look at this?  This bug defeats the purpose of removing bluedroid from the b2g process :-/

Comment 4

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

Hi,

sorry about this taking so long. We had bug 1115656 for the problem and discussed about handling failures gracefully (fault-tolerant, etc).

Let's fix the basic problem here, and do the long-term work in bug 1115656.

We'll provide a patch soon.

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> Let's fix the basic problem here, and do the long-term work in bug 1115656.

Cool, sounds good.  Any reason why we can't just let init restart bluetoothd if it crashes by removing "disabled/onshot" from https://www.codeaurora.org/cgit/quic/lf/b2g/platform_system_bluetoothd/tree/scripts/init.bluetooth.rc?h=mozilla/v2.2#n23

This'll probably cause bluetoothd to come up sooner than b2g during normal boot, so there's maybe something to be managed there, but that would make bluetoothd look and behave more like other Gonk daemons.

Comment 6

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

Restarting is easy, but we are worried about stale resources in Gecko or inconsistent state. I'll need to investigate this first in some detail.

Comment 7

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

Attachment: [01] Bug 1132229: Return in Dispatch method if Bluetooth runnable is nullptr

Comment 8

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

Attachment: [02] Bug 1132229: Handle I/O errors in Bluetooth's daemon backend

If the daemon crashes, sending commands will fail. This currently prevents us from disabling a crashed Bluetooth. This patch is required to handle the internal state correctly and make progress in all cases. So even if sending fails, the caller of an interface method will get an error result at least.

Comment 9

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

Attachment: [03] Bug 1132229: Survive crashes of bluetoothd

If we detect a crash, we signal a change in the adapter state. This will cleanup the internal state. Disabling now makes progress, even if individual steps fail (e.g., because of command-send failures).

Comment 10

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

Comment on attachment 8565425 [details] [diff] [review]
[01] Bug 1132229: Return in Dispatch method if Bluetooth runnable is nullptr

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

This looks good.

Comment 11

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

Comment on attachment 8565427 [details] [diff] [review]
[03] Bug 1132229: Survive crashes of bluetoothd

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +310,5 @@
>  
>      BT_LOGR("BluetoothInterface::Disable failed: %d", aStatus);
>  
> +    // Always make progress; even on failures
> +    BluetoothService::AcknowledgeToggleBt(false);

OK, the original code seems to send 'Enabled' signal even disable failed. In order to refresh UI status, sending 'Disabled' signal even disable fail would be easier to handle the failure.

Comment 12

[Mike Lee [:mlee]]

Thomas,

Is anything else needed to land this for 2.2? CAF testing is currently blocked waiting for this.

Thanks,
Mike

Comment 13

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

Sorry, I think I just forgot about this bug. Will land very soon...

Comment 14

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

https://hg.mozilla.org/integration/b2g-inbound/rev/15fc5f2edcab
https://hg.mozilla.org/integration/b2g-inbound/rev/1c7153d1ffe8
https://hg.mozilla.org/integration/b2g-inbound/rev/d9fc494771af

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

Comment 15

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

Comment on attachment 8565425 [details] [diff] [review]
[01] Bug 1132229: Return in Dispatch method if Bluetooth runnable is nullptr

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

  Bluetooth daemon, bug 1005934.

User impact if declined: 

  If the Bluetooth driver crashes, Gecko will crash as well.

Testing completed: 

  I tested this locally by killing the Bluetooth daemon. Gecko now survives
  this. I was also able to re-enable Bluetooth in Gecko and continue.

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

  Affects Bluetooth initialization and cleanup. Some QA of this functionality
  probably makes sense.

String or UUID changes made by this patch:

  None

Comment 16

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

Comment on attachment 8565426 [details] [diff] [review]
[02] Bug 1132229: Handle I/O errors in Bluetooth's daemon backend

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]

  Please see comment 15

Comment 17

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

Comment on attachment 8565427 [details] [diff] [review]
[03] Bug 1132229: Survive crashes of bluetoothd

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]

  Please see comment 15

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/15fc5f2edcab
https://hg.mozilla.org/mozilla-central/rev/1c7153d1ffe8
https://hg.mozilla.org/mozilla-central/rev/d9fc494771af

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb95c3b72a17
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8744a4adc69e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5a7cb3b9f78e