The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

Trunk
mozilla17
x86_64
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(2 attachments, 8 obsolete attachments)

14.29 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → kyle
Attachment #637394 - Flags: feedback?(bent.mozilla)
(Assignee)

Comment 2

5 years ago
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.
Attachment #637394 - Attachment is obsolete: true
Attachment #637394 - Flags: feedback?(bent.mozilla)
Attachment #637693 - Flags: feedback?(bent.mozilla)
(Assignee)

Comment 3

5 years ago
Created attachment 637742 [details] [diff] [review]
Patch 1 (v3): Blocking DBus calls on outside thread

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)
(Assignee)

Comment 4

5 years ago
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 :|
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?
(Assignee)

Comment 6

5 years ago
Required in order to implement things like bonding and other tasks that require multiple steps to happen in the same request.
(Assignee)

Updated

5 years ago
Blocks: 727618
(Assignee)

Updated

5 years ago
Attachment #637744 - Flags: review?(bent.mozilla) → feedback?(bent.mozilla)
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
(Assignee)

Comment 7

5 years ago
Created attachment 642782 [details] [diff] [review]
Patch 1 (v5): Blocking DBus calls on outside thread
Attachment #637744 - Attachment is obsolete: true
Attachment #637744 - Flags: feedback?(bent.mozilla)
(Assignee)

Comment 8

5 years ago
Created attachment 642786 [details] [diff] [review]
Patch 1 (v6): Blocking DBus calls on outside thread
Attachment #642782 - Attachment is obsolete: true
Attachment #642786 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
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)
Blocks: 730992
(Assignee)

Comment 10

5 years ago
Created attachment 643974 [details] [diff] [review]
Patch 1 (v7): Blocking DBus calls on outside thread
Attachment #642786 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 643987 [details] [diff] [review]
Patch 1 (v8): Fix error handling in unpacking DBus replies, set up DBus blocking call handling thread
Attachment #643974 - Attachment is obsolete: true
Attachment #643987 - Flags: review?(mrbkap)
(Assignee)

Comment 12

5 years ago
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
Attachment #643990 - Flags: review?(mrbkap)
Blocks: 775998
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-
(Assignee)

Comment 15

5 years ago
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.
Attachment #643990 - Attachment is obsolete: true
Attachment #649962 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #649962 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Priority: -- → P1
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8045e57e00dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2a5e7e2423
Priority: P1 → --
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/8045e57e00dd
https://hg.mozilla.org/mozilla-central/rev/0e2a5e7e2423
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 775998
You need to log in before you can comment on or make changes to this bug.