768306 - [b2g-bluetooth] Create interface for making synchronous calls on an outside thread

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

While async calls are now handled via the framework in bug 740744, doing multiple asynchonous calls for the same DOMRequest (e.g. when building a new adapter object, we need the default adapter address, adapter properties via getproperties, and to register the adapter agent) is a bit difficult due to having to pass around the DOMRequest we build. It would be nice to have a way to call multiple synchronous calls on an outside thread that's not the IO or DBus event thread. This will most likely consist of a thread owned by the BluetoothDBusService that events can be dispatched to.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1): WIP - Blocking DBus calls on outside thread

This is a WIP of making blocking calls on an outside DBus thread. Basically, I promoted the file level global thread we had for firmware loading into a class member, and now dispatch tasks to it. The feedback here is mainly for the form of the DefaultAdapterAndProperties() function in BluetoothDBusService, as the following things are still completely wrong:

- The bluetooth task thread only shuts down when gecko does
- The runnable passing is a leaky hack just to make sure the object exists when we're done calling blocking calls
- Adapter doesn't unpack properties right, will crash gecko on bringup but I just want to get the basic ideas out there (and also would like to sleep).

The main downside I see to this strategy is that we could stack tasks on this thread (they'll timeout, but block until they do), versus the async method which times out but doesn't block any threads. This also means we're leaving another thread alive while bluetooth is on, which takes more memory in the constrained phone space. However, it's very readable and straightforward, and any of these calls actually blocking for any discernable length of time should be an exceedingly rare occasion.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2): WIP - Blocking DBus calls on outside thread

Massively cleaned up version of blocking call on thread patch.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3): Blocking DBus calls on outside thread

Fixed up threading and leak issues, ready for review.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4): Blocking DBus calls on outside thread

Helps if the diff is pointing in the correct direction :|

Comment 5

[Dietrich Ayala (:dietrich)]

Can you explain why this needs to block the release? Is this a convenience, or required in order to implement?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Required in order to implement things like bonding and other tasks that require multiple steps to happen in the same request.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v5): Blocking DBus calls on outside thread

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v6): Blocking DBus calls on outside thread

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 642786 [details] [diff] [review]
Patch 1 (v6): Blocking DBus calls on outside thread

Withdrawing review. Lots of things that've been fixed in later branches need to be backported.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v7): Blocking DBus calls on outside thread

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v8): Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v8): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 643987 [details] [diff] [review]
Patch 1 (v8): Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread

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

::: dom/bluetooth/BluetoothService.cpp
@@ +26,1 @@
>  bool gInShutdown = false;

Drive-by comment: the remaining globals should either be in an anonymous namespace or static.

::: dom/bluetooth/BluetoothService.h
@@ +195,5 @@
> +   * thread and IO thread.
> +   *
> +   * For instance, when we retrieve an Adapter object, we * would like it to
> +   * come with all of its properties filled in and registered * as an agent,
> +   * which requires a minimum of 3 calls to platform specific code * on some

Looks like some rewrapping malfunctioned here.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 643990 [details] [diff] [review]
Patch 2 (v8): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object

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

r- because of the NS_Convert...().get() gaffe. I'll r+ the next one for sure.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +131,5 @@
> + * DBus Connection held for the BluetoothCommandThread to use. Should never be
> + * used by any other thread.
> + * 
> + */
> +nsAutoPtr<RawDBusConnection> gThreadConnection;

static or unnamed namespace?

@@ +713,5 @@
> +      DispatchBluetoothReply(mRunnable, v, replyError);
> +      return NS_ERROR_FAILURE;
> +    }
> +   
> +    object_path = NS_ConvertUTF16toUTF8(v.get_nsString()).get();

I don't think this is safe. NS_ConvertUTF16toUTF8 will create a temporary that gets destroyed at the end of this line. You need to create an nsCString here to hold the memory alive.

@@ +734,5 @@
> +      dbus_message_unref(msg);
> +    }
> +    // We have to manually attach the path to the rest of the elements
> +    v.get_ArrayOfBluetoothNamedValue().AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Path"),
> +                                                                         NS_ConvertUTF8toUTF16(object_path)));

It seems like it'd be worth keeping the UTF16-encoded version of object_path around to avoid re-converting it.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v9): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object

Fixed string init and use.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8045e57e00dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2a5e7e2423

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8045e57e00dd
https://hg.mozilla.org/mozilla-central/rev/0e2a5e7e2423