Closed
Bug 1180965
Opened 9 years ago
Closed 9 years ago
MOZ_ASSERT(!mIO) at ipc/unixsocket/ListenSocket.cpp:316
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(blocking-b2g:2.2+, firefox40 wontfix, firefox41 wontfix, firefox42 fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)
People
(Reporter: gwagner, Assigned: tzimmermann)
References
Details
(Keywords: crash)
Attachments
(3 files, 1 obsolete file)
16.56 KB,
text/plain
|
Details | |
4.41 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
3.88 KB,
patch
|
tzimmermann
:
review+
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
Seen during monkey testing on Flame on current trunk.
Comment 1•9 years ago
|
||
Thomas, Can you help on this unix socket related crash?
Flags: needinfo?(tzimmermann)
Reporter | ||
Comment 2•9 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
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
Changes since v1: - call |AcknowledgeToggleBt| with 'true' - added comments
Attachment #8633340 -
Attachment is obsolete: true
Attachment #8633423 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Thanks for the review! https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=c03777096aa8
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c03777096aa8
Comment 14•9 years ago
|
||
Thomas, Is the fix applicable with APIv1 on 2.2? Since APIv2 with adapter state was landed AFTER 2.2.
Flags: needinfo?(tzimmermann)
Assignee | ||
Comment 15•9 years ago
|
||
Here's the patch for v2.2. Same code as before.
Flags: needinfo?(tzimmermann)
Attachment #8637768 -
Flags: review+
Comment 16•9 years ago
|
||
Hi Thomas, Could you raise uplift approval request for b2g37? Thanks!
blocking-b2g: 2.2? → 2.2+
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
Flags: needinfo?(tzimmermann)
Assignee | ||
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox40:
--- → wontfix
status-firefox41:
--- → wontfix
Target Milestone: --- → FxOS-S3 (24Jul)
Comment 20•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/87aee5955644
status-b2g-v2.2r:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•