1180965 - MOZ_ASSERT(!mIO) at ipc/unixsocket/ListenSocket.cpp:316

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gregor Wagner [:gwagner]]

Attachment: bt

Seen during monkey testing on Flame on current trunk.

Comment 1

[Ben Tian (inactive)]

Thomas,

Can you help on this unix socket related crash?

Comment 2

[Gregor Wagner [:gwagner]]

Not too hard to reproduce by enabling/disabling BT with high frequency.

I/Gecko   (  317): [Parent 317] WARNING: 'mCmdChannel->GetConnectionStatus() == SOCKET_CONNECTED', file ../../../dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp, line 1869
F/MOZ_Assert(  317): Assertion failure: !mIO, at ../../../ipc/unixsocket/ListenSocket.cpp:316

Comment 3

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

Thanks for reporting and sorry for the delay here. I was on PTO and will look at this now.

Comment 4

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

Ben,

AFAICT the problem is in the startup/shutdown of BT. What it does is

 1) enable BT in BluetoothServiceBluedroid::StartGonkBluetooth
 2) disable BT in BluetoothServiceBluedroid::StopGonkBluetooth
 3) enable BT again

(2) and (3) overlap in such a way that (3) starts before the shutdown of (2) has been completed. The socket data structures of (1) are still around and being shut down.

The backend code is a singleton instance, which we cannot duplicate every time we activate BT. There is only one BT driver after all.

I'd like to propose the following solution

 - If we are at (3) and detect that (2) has not been completed, we return an error message to the App.
 - Apps need to be prepared for 'Enable' to fail.
 - Apps need to be able to detect when (2) has been completed. Doing (3) should then be safe.

Is that reasonable and possible with the current DOM APIs?

Comment 5

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> AFAICT the problem is in the startup/shutdown of BT. What it does is
> 
>  1) enable BT in BluetoothServiceBluedroid::StartGonkBluetooth
>  2) disable BT in BluetoothServiceBluedroid::StopGonkBluetooth
>  3) enable BT again
> 
> (2) and (3) overlap in such a way that (3) starts before the shutdown of (2)
> has been completed. The socket data structures of (1) are still around and
> being shut down.

Thomas,

Can bluetoothd clear socket data structures before calling |AdapterStateChangedNotification(false)| for (2)?

(3) should always run after (2) since [1] ensures state to be Disabled before calling |Enable|. The state for (2) is expected to change as following:

  - When |Disable| is called, state becomes Disabling [2].
  - After adapter state becomes BT_STATE_OFF [3], Bluetooth Service fires event [4] to change state to Disabled [5].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.cpp#861
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.cpp#907
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#1858
[4] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothService.cpp#668
[5] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.cpp#944

Comment 6

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

Hi,

thanks for clarifying. Looks like the adapter code already does the right thing.

The problem is at [3], where the adapter is set to 'Disabled' too early. I have a patch for this. There is some confusion in the code about what 'disabled' actually means. In the adapter, it refer to the adapter state, but in the backend it refers to the overall Bluetooth stack. If we ever support more than one adapter, we'll have to harmonize this.

Comment 7

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

Attachment: [01] Bug 1180965: Don't disable BT before daemon has been shutdown

This patch will set the adapter to 'disabled' only after all connections to bluetoothd have been closed. I tested it locally with the given STR, but have not been able to reproduce the bug.

Comment 8

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

I think 2.2 is also affected.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8633340 [details] [diff] [review]
[01] Bug 1180965: Don't disable BT before daemon has been shutdown

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1882,3 @@
>    if (sAdapterEnabled) {
> +
> +    BluetoothService::AcknowledgeToggleBt(sAdapterEnabled);

Call |BluetoothService::AcknowledgeToggleBt(true)| directly.

Also leave comment to explain different |AcknowledgeToggleBt| timings between Enable and Disable.

Comment 10

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

Attachment: [01] Bug 1180965: Don't disable BT before daemon has been shut down (v2)

Changes since v1:

  - call |AcknowledgeToggleBt| with 'true'
  - added comments

Comment 11

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

Thanks for the review!

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

Comment 12

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

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

Comment 13

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

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

Comment 14

[Ben Tian (inactive)]

Thomas,

Is the fix applicable with APIv1 on 2.2? Since APIv2 with adapter state was landed AFTER 2.2.

Comment 15

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

Attachment: [01] Bug 1180965: Don't disable BT before daemon has been shut down (v2) {v2.2}

Here's the patch for v2.2. Same code as before.

Comment 16

[Josh Cheng [:josh]]

Hi Thomas,
Could you raise uplift approval request for b2g37?
Thanks!

Comment 17

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

Comment on attachment 8637768 [details] [diff] [review]
[01] Bug 1180965: Don't disable BT before daemon has been shut down (v2) {v2.2}

Oops. Sure, here we go.

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

  The bug was introduced by the Bluetooth daemon feature of v2.2.

User impact if declined: 

  Quickly switching BT on and off can crash the phone.

Testing completed: 

  Local testing on flame-kk, aries-l; switching BT on/off at high frequency

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

  Low, compared to not taking it. The patch contains a small refactoring of the BT shutdown code.

String or UUID changes made by this patch:

  None.

Comment 18

[Josh Cheng [:josh]]

Comment on attachment 8637768 [details] [diff] [review]
[01] Bug 1180965: Don't disable BT before daemon has been shut down (v2) {v2.2}

Approving and please NI if causing any side effect. Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/87aee5955644

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/87aee5955644