957103 - [Bluetooth] Move all DBus code and data to I/O thread

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We currently use DBus data structures for Bluetooth on multiple threads: the main thread, the I/O thread and the BT setup thread. This leads to dependencies and problems such ad bug 950891.

A better strategy is to move all DBus data and code to the I/O thread. We can then safely create, modify and cleanup connections without interfering with concurrent DBus operations.

Comment 1

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

Hi Eric,

Here is a list of changes I intend to implement in the BlueZ support for fixing this bug. Can you give some thoughts on them? Thanks.

1) Rework the start and stop procedure to setup the connection on the I/O thread

This needs some extra tasks and runnables to execute the BT initialization, shutdown and waiting on the BT thread. The DBus connection setup will run on the I/O thread, like the rest of the DBus code. I already have patches for this change.

2) Merge BluetoothGonkService into BluetoothDBusService.

BluetoothGonkService is really just a handful of methods that can be merged into it's parent class by the use of ifdef MOZ_WIDGET_GONK. There are already gonk-specific code paths in BluetoothDBusService, so the difference is minor. Merging both classes also allows to use the setup code from point 1 on regular Linux systems.

3) Move all DBus calls on the main thread to the I/O thread. This involves adding some tasks that execute SendWithReply methods on the I/O thread. I don't expect any problems here.

This should move all uses of DBus connections to the I/O thread. Once these changes have been implemented, we can remove the internal multi-threading in RawBusConnection and just assume everything runs on the I/O thread. This in turn will make the overall code path a bit more efficient in terms of runtime and memory consumption.

Comment 2

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

Hi Thomas,

Thanks for raising this topic.
 
> 1) Rework the start and stop procedure to setup the connection on the I/O
> thread
> 
> This needs some extra tasks and runnables to execute the BT initialization,
> shutdown and waiting on the BT thread. The DBus connection setup will run on
> the I/O thread, like the rest of the DBus code. I already have patches for
> this change.

The key point is that we should ensure DeleteDBusConnectionTask has to be executed before a new RawDBusConnection is created, otherwise we may still hit something like bug 950891. If this is covered in your new patch then let's do it.

> 2) Merge BluetoothGonkService into BluetoothDBusService.
> 
> BluetoothGonkService is really just a handful of methods that can be merged
> into it's parent class by the use of ifdef MOZ_WIDGET_GONK. There are
> already gonk-specific code paths in BluetoothDBusService, so the difference
> is minor. Merging both classes also allows to use the setup code from point
> 1 on regular Linux systems.
> 

Totally agree. In fact, several months ago I made a patch for this as well but didn't file a bug to fix it. Thanks!

> 3) Move all DBus calls on the main thread to the I/O thread. This involves
> adding some tasks that execute SendWithReply methods on the I/O thread. I
> don't expect any problems here.
> 
> This should move all uses of DBus connections to the I/O thread. Once these
> changes have been implemented, we can remove the internal multi-threading in
> RawBusConnection and just assume everything runs on the I/O thread. This in
> turn will make the overall code path a bit more efficient in terms of
> runtime and memory consumption.

This sounds like a kind of cleanup for RawBusConnection to me. I don't have so much concern about this.

Comment 3

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

Attachment: [01] Bug 957103: Invoke DBus send operations on I/O thread

I hope it's possible to review this large patch. Although it touches a lot of functions, it really just moves code from main thread to the I/O thread.

Comment 4

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

Attachment: [02] Bug 957103: Set PROP_BLUETOOTH_ENABLED in ToggleBtAck

Comment 5

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

Attachment: [03] Bug 957103: Implement StartStopBluetooth in BluetoothGonkService

This patch moves StartStopBluetooth to BluetoothGonkService and {Start,Stop}Internal of BluetoothService are not used anymore.

The old code is left in place for use by bluedroid. Maybe later we can add a similar patch for bluedroid and remove StartInternal and StopInternal all together.

Comment 6

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

Attachment: [04] Bug 957103: Refactor ToggleBtTask and BT library handling

Comment 7

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

Attachment: [05] Bug 957103: Split start and stop procedure for BlueZ into multiple steps

This should keep the original functionality and logic for starting and stopping in place. It just moves the different steps of each to the appropriate thread.

Comment 8

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

Attachment: [06] Bug 957103: Merge BlueZ Gonk into desktop code

Comment 9

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

Attachment: [07] Bug 957103: Integrate BlueZ Gonk and desktop code paths

Comment 10

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

Comment on attachment 8360998 [details] [diff] [review]
[01] Bug 957103: Invoke DBus send operations on I/O thread

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

Hi Thomas,

Please see my comments below. Sorry for taking so long.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +1344,5 @@
>  
> +class AddReservedServiceRecordsTask : public Task
> +{
> +public:
> +  AddReservedServiceRecordsTask(const nsCString& aAdapterPath)

Most of the time we pass a const nsACString& as an argument instead of const nsCString&. Here and below.

@@ +1404,5 @@
>      sAuthorizedServiceClass.AppendElement(BluetoothServiceClass::HID);
>  
> +    Task* task = new
> +      AddReservedServiceRecordsTask(NS_ConvertUTF16toUTF8(sAdapterPath));
> +    MOZ_ASSERT(task);

Operator 'new' is infallible. No need to check the validity of variable 'task'. Here and below. (https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation)

@@ +2014,5 @@
> +    handler.forget();
> +  }
> +
> +private:
> +  nsCOMPtr<BluetoothReplyRunnable> mRunnable;

nsRefPtr should be used since BluetoothReplyRunnable is a concrete class. Here and below. (https://developer.mozilla.org/en-US/docs/nsRefPtr)

@@ +2154,5 @@
> +  }
> +
> +  void Run() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread()); // /I/O thread

super nit: extra '/'

@@ +2713,5 @@
> +  const nsCString mAdapterPath;
> +  const nsCString mDeviceAddress;
> +  int mTimeout;
> +  nsCOMPtr<BluetoothReplyRunnable> mRunnable;
> +

super nit: extra line

@@ +2790,5 @@
> +private:
> +  const nsString mAdapterPath;
> +  const nsString mDeviceAddress;
> +  nsCOMPtr<BluetoothReplyRunnable> mRunnable;
> +

super nit: extra line.

@@ +2807,5 @@
>    }
>  
> +  Task* task = new RemoveDeviceTask(sAdapterPath,
> +                                    aDeviceAddress,
> +                                    aRunnable);

nit: one line should be enough (80 chars).

@@ +2889,5 @@
> +                                  aRunnable);
> +  MOZ_ASSERT(task);
> +  DispatchToDBusThread(task);
> +
> +  return true;

Since we've moved the logic to SetPinCodeTask, returning a boolean seems turns out to be redundant. You may want to make another patch otherwise this patch would be too huge. ;)

Comment 11

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

Hi Eric,

I found that some of the DBus calls in RawDBusConnection::EstablishConnection internally block, so we cannot move them to the I/O thread. I canceled the reviews until I find a way to fix this problem.

I'd still like to land the patch in bug 964817 and I'll probably re-supply some of the non-problematic patches here in a different form.

Comment 12

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

Hi Thomas,

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Hi Eric,
> 
> I found that some of the DBus calls in
> RawDBusConnection::EstablishConnection internally block, so we cannot move
> them to the I/O thread. I canceled the reviews until I find a way to fix
> this problem.
> 
> I'd still like to land the patch in bug 964817 and I'll probably re-supply
> some of the non-problematic patches here in a different form.

Got it. Thanks!

Comment 13

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

Comment on attachment 8361006 [details] [diff] [review]
[04] Bug 957103: Refactor ToggleBtTask and BT library handling

Implemented by bug 972253

Comment 14

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> Comment on attachment 8361006 [details] [diff] [review]
> [04] Bug 957103: Refactor ToggleBtTask and BT library handling
> 
> Implemented by bug 972253

s/972253/969314

Comment 15

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

Comment on attachment 8361008 [details] [diff] [review]
[06] Bug 957103: Merge BlueZ Gonk into desktop code

Implemented by bug 969314

Comment 16

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

Comment on attachment 8361009 [details] [diff] [review]
[07] Bug 957103: Integrate BlueZ Gonk and desktop code paths

Implemented by bug 969314

Comment 17

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

Comment on attachment 8360999 [details] [diff] [review]
[02] Bug 957103: Set PROP_BLUETOOTH_ENABLED in ToggleBtAck

Implemented by bug 977146.

Comment 18

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

Comment on attachment 8361004 [details] [diff] [review]
[03] Bug 957103: Implement StartStopBluetooth in BluetoothGonkService

Partially implemented by bug 983576.

Comment 19

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

Comment on attachment 8361007 [details] [diff] [review]
[05] Bug 957103: Split start and stop procedure for BlueZ into multiple steps

Partially implemented by bug 983576.

Comment 20

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

Most of the changes of this bug have landed as part of other bugs. The remaining changes would require re-implementing functions from libdbus, which is probably not worth the effort. So I'm closing this bug.