972253 - [Bluetooth] Move content of DBusThread.{cpp,h} to BT

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

DBusThread.{cpp,h} maintains state for Bluetooth, so the code should be located in the Bluetooth module itself. DBusThread.{cpp,h} can be removed afterwards.

This change also allows us to fix some problems in BluetoothDBusService::StartInternal, such as the re-initialization of an existing connection, or the concurrent setup on the I/O thread.

Comment 1

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

Attachment: [01] Bug 972253: Merge the content of DBusThread.cpp into Bluetooth code

Comment 2

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

Attachment: [02] Bug 972253: Cleanup Start/Stop methods in BlueZ backend

Comment 3

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

Attachment: [03] Bug 972253: Remove GetDBusConnection

Comment 4

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

Attachment: [04] Bug 972253: Remove DBusThread.{cpp,h}

Comment 5

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8376126 [details] [diff] [review]
[02] Bug 972253: Cleanup Start/Stop methods in BlueZ backend

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

Looks good. One typo in the patch summary: " ... up the cod efor ... "

I just realized that sDBusConnection is accessed from two different threads (mostly from I/O thread but from BT thread in StartInternal()/ StopInternal()). We might need do some protection on this variable. Any suggestions, Thomas?

Comment 6

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

Hi Eric,

Thanks for the review.

> I just realized that sDBusConnection is accessed from two different threads
> (mostly from I/O thread but from BT thread in StartInternal()/
> StopInternal()). We might need do some protection on this variable. Any
> suggestions, Thomas?

Yeah, I've been thinking about this for a while and it would have been one of my next patch sets. :) Right now we need to set and clear sDBusConnection in {Start|Stop}Internal because of the reply that is signaled after these methods return. So we should move the reply to the end of each process (i.e. StartDBusConnectionTask and DeleteDBusConnection) and re-factor the functionality as follows:

 Starting:
   - setup Bluedroid and DBus connection in StartInternal (BT thread)
   - set sDBusConnection and start watching in StartDbusConnectionTask::Run (I/O thread)
   - signal success

 Stopping:
   - clear gDBusConnection (I/O)
   - clean up connection (BT)
   - delete connection in DeleteDBusConnetion::Run (I/O)
   - close Bluedroid (BT)
   - signal success

Stopping is ugly. There is no good way for telling DBus to stop watching a connection. This means that a connection can still receive DBus replies while being shut down. This seems to work right now because some of the code handles this case explicitly. The only way to fully protect us from racy shutdowns is to run the cleanup on the I/O thread. But there are calls to dbus_bus_remove_match [1] in StopInternal. These calls block if we want them to report errors. The other DBus calls should be fine. I'd suggest to remove error reporting from dbus_bus_remove_match and collapse steps 1 to 3 into one, which can run on the I/O thread.

Best regards
Thomas

[1] http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html#ga6e6b98e19fa4400de7ef99c27b866b99

Comment 7

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8376127 [details] [diff] [review]
[03] Bug 972253: Remove GetDBusConnection

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

Thanks! :)

Comment 8

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

Attachment: [02] Bug 972253: Cleanup Start/Stop methods in BlueZ backend (v2)

Updated commit message

Comment 9

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

Thanks a lot.

https://hg.mozilla.org/integration/b2g-inbound/rev/7c5218d79785
https://hg.mozilla.org/integration/b2g-inbound/rev/c41b71017606
https://hg.mozilla.org/integration/b2g-inbound/rev/1cc54f3d9119
https://hg.mozilla.org/integration/b2g-inbound/rev/ec385113970a

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=ec385113970a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c5218d79785
https://hg.mozilla.org/mozilla-central/rev/c41b71017606
https://hg.mozilla.org/mozilla-central/rev/1cc54f3d9119
https://hg.mozilla.org/mozilla-central/rev/ec385113970a