Last Comment Bug 768306 - [b2g-bluetooth] Create interface for making synchronous calls on an outside thread
: [b2g-bluetooth] Create interface for making synchronous calls on an outside t...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: x86_64 Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Kyle Machulis [:qdot]
:
: Andrew Overholt [:overholt]
Mentors:
: 775998 (view as bug list)
Depends on: 740744 761511
Blocks: b2g-bluetooth 730992 775998
  Show dependency treegraph
 
Reported: 2012-06-25 19:33 PDT by Kyle Machulis [:qdot]
Modified: 2012-08-08 22:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Patch 1 (v1): WIP - Blocking DBus calls on outside thread (10.71 KB, patch)
2012-06-28 00:17 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v2): WIP - Blocking DBus calls on outside thread (22.38 KB, patch)
2012-06-28 14:45 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v3): Blocking DBus calls on outside thread (23.45 KB, patch)
2012-06-28 17:45 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v4): Blocking DBus calls on outside thread (23.42 KB, patch)
2012-06-28 17:47 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v5): Blocking DBus calls on outside thread (22.81 KB, patch)
2012-07-16 16:20 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v6): Blocking DBus calls on outside thread (22.77 KB, patch)
2012-07-16 16:37 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v7): Blocking DBus calls on outside thread (22.68 KB, patch)
2012-07-19 12:30 PDT, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
Patch 1 (v8): Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread (14.29 KB, patch)
2012-07-19 13:08 PDT, Kyle Machulis [:qdot]
mrbkap: review+
Details | Diff | Splinter Review
Patch 2 (v8): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object (9.17 KB, patch)
2012-07-19 13:09 PDT, Kyle Machulis [:qdot]
mrbkap: review-
Details | Diff | Splinter Review
Patch 2 (v9): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object (9.19 KB, patch)
2012-08-07 22:00 PDT, Kyle Machulis [:qdot]
mrbkap: review+
Details | Diff | Splinter Review

Description Kyle Machulis [:qdot] 2012-06-25 19:33:52 PDT
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] 2012-06-28 00:17:08 PDT
Created attachment 637394 [details] [diff] [review]
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] 2012-06-28 14:45:04 PDT
Created attachment 637693 [details] [diff] [review]
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] 2012-06-28 17:45:43 PDT
Created attachment 637742 [details] [diff] [review]
Patch 1 (v3): Blocking DBus calls on outside thread

Fixed up threading and leak issues, ready for review.
Comment 4 Kyle Machulis [:qdot] 2012-06-28 17:47:02 PDT
Created attachment 637744 [details] [diff] [review]
Patch 1 (v4): Blocking DBus calls on outside thread

Helps if the diff is pointing in the correct direction :|
Comment 5 Dietrich Ayala (:dietrich) 2012-07-03 13:53:46 PDT
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] 2012-07-03 15:44:35 PDT
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] 2012-07-16 16:20:49 PDT
Created attachment 642782 [details] [diff] [review]
Patch 1 (v5): Blocking DBus calls on outside thread
Comment 8 Kyle Machulis [:qdot] 2012-07-16 16:37:49 PDT
Created attachment 642786 [details] [diff] [review]
Patch 1 (v6): Blocking DBus calls on outside thread
Comment 9 Kyle Machulis [:qdot] 2012-07-18 17:47:53 PDT
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] 2012-07-19 12:30:51 PDT
Created attachment 643974 [details] [diff] [review]
Patch 1 (v7): Blocking DBus calls on outside thread
Comment 11 Kyle Machulis [:qdot] 2012-07-19 13:08:52 PDT
Created attachment 643987 [details] [diff] [review]
Patch 1 (v8): Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread
Comment 12 Kyle Machulis [:qdot] 2012-07-19 13:09:24 PDT
Created attachment 643990 [details] [diff] [review]
Patch 2 (v8): Change BluetoothAdapter initialization to get properties via off main thread blocking calls when creating Adapter object
Comment 13 Blake Kaplan (:mrbkap) 2012-08-07 18:16:07 PDT
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) 2012-08-07 18:27:39 PDT
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] 2012-08-07 22:00:29 PDT
Created attachment 649962 [details] [diff] [review]
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 18 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-08-08 22:01:19 PDT
*** Bug 775998 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.