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

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gwagner, Assigned: tzimmermann)

Tracking

({crash})

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 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Posted file bt
Seen during monkey testing on Flame on current trunk.
Reporter

Updated

4 years ago
blocking-b2g: --- → 2.5+
Keywords: crash
Thomas,

Can you help on this unix socket related crash?
Flags: needinfo?(tzimmermann)
Reporter

Comment 2

4 years ago
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
Thanks for reporting and sorry for the delay here. I was on PTO and will look at this now.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Flags: needinfo?(tzimmermann)
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?
Flags: needinfo?(btian)
(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
Flags: needinfo?(btian) → needinfo?(tzimmermann)
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.
Flags: needinfo?(tzimmermann)
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.
Attachment #8633340 - Flags: review?(btian)
I think 2.2 is also affected.
blocking-b2g: 2.5+ → 2.2?
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.
Attachment #8633340 - Flags: review?(btian) → review+
Changes since v1:

  - call |AcknowledgeToggleBt| with 'true'
  - added comments
Attachment #8633340 - Attachment is obsolete: true
Attachment #8633423 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c03777096aa8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Reporter

Updated

4 years ago
Blocks: 1185768
Thomas,

Is the fix applicable with APIv1 on 2.2? Since APIv2 with adapter state was landed AFTER 2.2.
Flags: needinfo?(tzimmermann)
Here's the patch for v2.2. Same code as before.
Flags: needinfo?(tzimmermann)
Attachment #8637768 - Flags: review+
Hi Thomas,
Could you raise uplift approval request for b2g37?
Thanks!
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(tzimmermann)
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.
Flags: needinfo?(tzimmermann)
Attachment #8637768 - Flags: approval-mozilla-b2g37?
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!
Attachment #8637768 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Target Milestone: --- → FxOS-S3 (24Jul)
Duplicate of this bug: 1185768
You need to log in before you can comment on or make changes to this bug.