Closed
Bug 768306
Opened 12 years ago
Closed 12 years ago
[b2g-bluetooth] Create interface for making synchronous calls on an outside thread
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: qdot, Assigned: qdot)
References
Details
Attachments
(2 files, 8 obsolete files)
14.29 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
9.19 KB,
patch
|
mrbkap
:
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.
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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)
Comment 5•12 years ago
|
||
Can you explain why this needs to block the release? Is this a convenience, or required in order to implement?
Assignee | ||
Comment 6•12 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•12 years ago
|
Blocks: b2g-bluetooth
Assignee | ||
Updated•12 years ago
|
Attachment #637744 -
Flags: review?(bent.mozilla) → feedback?(bent.mozilla)
Updated•12 years ago
|
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #637744 -
Attachment is obsolete: true
Attachment #637744 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #642782 -
Attachment is obsolete: true
Attachment #642786 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•12 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)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #642786 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #643974 -
Attachment is obsolete: true
Attachment #643987 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #643990 -
Flags: review?(mrbkap)
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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•12 years ago
|
||
Fixed string init and use.
Attachment #643990 -
Attachment is obsolete: true
Attachment #649962 -
Flags: review?(mrbkap)
Updated•12 years ago
|
Attachment #649962 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 16•12 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
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8045e57e00dd https://hg.mozilla.org/mozilla-central/rev/0e2a5e7e2423
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•