Closed Bug 768306 Opened 10 years ago Closed 9 years ago

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


(Core :: DOM: Device Interfaces, defect)

Gonk (Firefox OS)
Not set



blocking-kilimanjaro +
blocking-basecamp +


(Reporter: qdot, Assigned: qdot)




(2 files, 8 obsolete files)

14.29 KB, patch
: review+
Details | Diff | Splinter Review
9.19 KB, patch
: review+
Details | Diff | Splinter Review
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.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
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.
Assignee: nobody → kyle
Attachment #637394 - Flags: feedback?(bent.mozilla)
Massively cleaned up version of blocking call on thread patch.
Attachment #637394 - Attachment is obsolete: true
Attachment #637394 - Flags: feedback?(bent.mozilla)
Attachment #637693 - Flags: feedback?(bent.mozilla)
Fixed up threading and leak issues, ready for review.
Attachment #637693 - Attachment is obsolete: true
Attachment #637693 - Flags: feedback?(bent.mozilla)
Attachment #637742 - Flags: review?(bent.mozilla)
Helps if the diff is pointing in the correct direction :|
Attachment #637742 - Attachment is obsolete: true
Attachment #637742 - Flags: review?(bent.mozilla)
Attachment #637744 - Flags: review?(bent.mozilla)
Can you explain why this needs to block the release? Is this a convenience, or required in order to implement?
Required in order to implement things like bonding and other tasks that require multiple steps to happen in the same request.
Attachment #637744 - Flags: review?(bent.mozilla) → feedback?(bent.mozilla)
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Attachment #637744 - Attachment is obsolete: true
Attachment #637744 - Flags: feedback?(bent.mozilla)
Attachment #642782 - Attachment is obsolete: true
Attachment #642786 - Flags: review?(mrbkap)
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.
Attachment #642786 - Flags: review?(mrbkap)
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.
Attachment #643987 - Flags: review?(mrbkap) → review+
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.
Attachment #643990 - Flags: review?(mrbkap) → review-
Attachment #649962 - Flags: review?(mrbkap) → review+
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.