Closed
Bug 891866
Opened 8 years ago
Closed 8 years ago
[Bluetooth] Replace RegisterAgent by non-blocking implementation
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
(Whiteboard: [fixed-in-birch])
Attachments
(4 files, 5 obsolete files)
3.68 KB,
patch
|
echou
:
review+
gyeh
:
review+
|
Details | Diff | Splinter Review |
4.11 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
echou
:
review+
gyeh
:
review+
|
Details | Diff | Splinter Review |
940 bytes,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
PrepareAdapterTask uses the Bluetooth command thread. The first step towards its cleanup is the refactoring of RegisterAgent, which can be implemented in a non-blocking fashion.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #773257 -
Flags: review?(gyeh)
Attachment #773257 -
Flags: review?(echou)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #773260 -
Flags: review?(gyeh)
Attachment #773260 -
Flags: review?(echou)
Comment 3•8 years ago
|
||
Hi Thomas, I'll review this patch tomorrow. :)
Comment 4•8 years ago
|
||
Comment on attachment 773257 [details] [diff] [review] [01] Bug 891866: Cleanup RegisterLocalAgent Review of attachment 773257 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1128,5 @@ > // device agent. > static bool > +RegisterLocalAgent(const char* aAdapterPath, > + const char* aAgentPath, > + const char* aCapabilities) Nice catch :) @@ +1142,4 @@ > return false; > } > > + DBusError err = DBUS_ERROR_INIT; I cannot find the definition of |DBUS_ERROR_INIT|. Did I miss anything?
Assignee | ||
Comment 5•8 years ago
|
||
Hi
> @@ +1142,4 @@
> > return false;
> > }
> >
> > + DBusError err = DBUS_ERROR_INIT;
>
> I cannot find the definition of |DBUS_ERROR_INIT|. Did I miss anything?
No, you're right. Until an hour ago, I wasn't aware that DBUS_ERROR_INIT is an internal API of libdbus. I'll replace it by dbus_error_init.
Comment 6•8 years ago
|
||
Comment on attachment 773260 [details] [diff] [review] [02] Bug 891866: Implement RegisterAgent with asyncronous functions Review of attachment 773260 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1119,5 @@ > +class RegisterAgentReplyHandler : public DBusReplyHandler > +{ > +public: > + RegisterAgentReplyHandler(const DBusObjectPathVTable* aAgentVTable) > + : mAgentVTable(aAgentVTable) nit: This line should be indent one more level. @@ +1187,5 @@ > + NS_ENSURE_TRUE(success, false); > + > + handler.forget(); > + > + return false; We should return |true| here, right?
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #6) > Comment on attachment 773260 [details] [diff] [review] > [02] Bug 891866: Implement RegisterAgent with asyncronous functions > > Review of attachment 773260 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > @@ +1119,5 @@ > > +class RegisterAgentReplyHandler : public DBusReplyHandler > > +{ > > +public: > > + RegisterAgentReplyHandler(const DBusObjectPathVTable* aAgentVTable) > > + : mAgentVTable(aAgentVTable) > > nit: This line should be indent one more level. Ok. > @@ +1187,5 @@ > > + NS_ENSURE_TRUE(success, false); > > + > > + handler.forget(); > > + > > + return false; > > We should return |true| here, right? Yes.
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #773257 -
Attachment is obsolete: true
Attachment #773257 -
Flags: review?(gyeh)
Attachment #773257 -
Flags: review?(echou)
Attachment #776235 -
Flags: review?(gyeh)
Attachment #776235 -
Flags: review?(echou)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #773260 -
Attachment is obsolete: true
Attachment #773260 -
Flags: review?(gyeh)
Attachment #773260 -
Flags: review?(echou)
Attachment #776237 -
Flags: review?(gyeh)
Attachment #776237 -
Flags: review?(echou)
Assignee | ||
Updated•8 years ago
|
Attachment #776235 -
Attachment description: [01] Bug 891866: Cleanup RegisterLocalAgent → [01] Bug 891866: Cleanup RegisterLocalAgent (v2)
Assignee | ||
Updated•8 years ago
|
Attachment #776237 -
Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions → [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v2)
Comment 10•8 years ago
|
||
Comment on attachment 776235 [details] [diff] [review] [01] Bug 891866: Cleanup RegisterLocalAgent (v2) Review of attachment 776235 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, r=me.
Attachment #776235 -
Flags: review?(gyeh) → review+
Comment 11•8 years ago
|
||
Comment on attachment 776235 [details] [diff] [review] [01] Bug 891866: Cleanup RegisterLocalAgent (v2) Review of attachment 776235 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #776235 -
Flags: review?(echou) → review+
Comment 12•8 years ago
|
||
Comment on attachment 776237 [details] [diff] [review] [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v2) Review of attachment 776237 [details] [diff] [review]: ----------------------------------------------------------------- Hi Thomas, Overall it looks good. Please answer my only question and fix a super nit, then I'll review again. Thanks. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1135,5 @@ > + > + // There is no "RegisterAgent" function defined in device interface. > + // When we call "CreatePairedDevice", it will do device agent registration > + // for us. (See maemo.org/api_refs/5.0/beta/bluez/adapter.html) > + if (!dbus_connection_register_object_path(gThreadConnection->GetConnection(), Question: gThreadConnection was created on mBluetoothCommandThread, but here we're going to use it on DBus thread. Would it be safe if we use it on different thread without protection? @@ +1160,1 @@ > MOZ_ASSERT(!NS_IsMainThread()); Not-even-a-nit: we mostly put MOZ_ASSERT(!NS_IsMainThread()) at the beginning of a function.
Assignee | ||
Comment 13•8 years ago
|
||
Hi
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1135,5 @@
> > +
> > + // There is no "RegisterAgent" function defined in device interface.
> > + // When we call "CreatePairedDevice", it will do device agent registration
> > + // for us. (See maemo.org/api_refs/5.0/beta/bluez/adapter.html)
> > + if (!dbus_connection_register_object_path(gThreadConnection->GetConnection(),
>
> Question: gThreadConnection was created on mBluetoothCommandThread, but here
> we're going to use it on DBus thread. Would it be safe if we use it on
> different thread without protection?
That's actually a good point. Accessing the DBusConnection structure is save, but I think gThreadConnection could be cleared in StopInternal while we're in the DBus callback function.
So we need to thread-safely make a copy of gThreadConnection first.
Assignee | ||
Comment 14•8 years ago
|
||
We need this change for holding references to our DBus connection from multiple threads.
Attachment #777758 -
Flags: review?(kyle)
Assignee | ||
Comment 15•8 years ago
|
||
This should fix our problem for now., as we acquire the DBus connection and hold it while we're in this function.
Attachment #776237 -
Attachment is obsolete: true
Attachment #776237 -
Flags: review?(gyeh)
Attachment #776237 -
Flags: review?(echou)
Attachment #777759 -
Flags: review?(gyeh)
Attachment #777759 -
Flags: review?(echou)
Assignee | ||
Updated•8 years ago
|
Attachment #777758 -
Attachment description: Bug 891866: Support nsRefPtr<RawDBusConnection> → [2] Bug 891866: Support nsRefPtr<RawDBusConnection>
Assignee | ||
Updated•8 years ago
|
Attachment #777758 -
Attachment description: [2] Bug 891866: Support nsRefPtr<RawDBusConnection> → [02] Bug 891866: Support nsRefPtr<RawDBusConnection>
Assignee | ||
Updated•8 years ago
|
Attachment #777759 -
Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v3) → [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v3)
Comment 16•8 years ago
|
||
Comment on attachment 777758 [details] [diff] [review] [02] Bug 891866: Support nsRefPtr<RawDBusConnection> Review of attachment 777758 [details] [diff] [review]: ----------------------------------------------------------------- r-'ing to make sure we're using the right structure here (since there's too many ways to refcount something in gecko :| ). Feel free to kick back to me unchanged if I'm off here. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +147,5 @@ > * DBus Connection held for the BluetoothCommandThread to use. Should never be > * used by any other thread. > * > */ > +static nsRefPtr<RawDBusConnection> gThreadConnection; Does this really need to be ns* at all? We could probably just use the RefCounted semantics in mfbt/RefPtr.h and not have to worry about implementing ns* style ref counting by hand.
Attachment #777758 -
Flags: review?(kyle) → review-
Comment 17•8 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13) > That's actually a good point. Accessing the DBusConnection structure is > save Huh. DBusConnection access is thread safe? I thought we ran into issues with this before?
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #17) > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13) > > That's actually a good point. Accessing the DBusConnection structure is > > save > > Huh. DBusConnection access is thread safe? I thought we ran into issues with > this before? Hi Kyle, I'm only aware of bug 827888, but this was caused by running our call-back function on the wrong thread. I had a look at the implementation of libdbus, and the public functions first acquired a lock and then called a second, internal function. So function calls to libdbus are linearizable when called from multiple threads. (I'm not saying it's a good thing to do so, though.) Since the connection structure in RawDBusConnection should be the same for all its instances, we already do this. We currently have 3 instances of RawDBusConnection: DBusThread in DBusThread.cpp, and mConnection and gThreadConnection in BluetoothDBusService. I think we should try to remove at least the last one; or even better expose the one in DBusThread to other modules. Best regards Thomas
Assignee | ||
Comment 19•8 years ago
|
||
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +147,5 @@
> > * DBus Connection held for the BluetoothCommandThread to use. Should never be
> > * used by any other thread.
> > *
> > */
> > +static nsRefPtr<RawDBusConnection> gThreadConnection;
>
> Does this really need to be ns* at all? We could probably just use the
> RefCounted semantics in mfbt/RefPtr.h and not have to worry about
> implementing ns* style ref counting by hand.
That's even better. :)
Assignee | ||
Comment 20•8 years ago
|
||
Use RefCounted<> instead of DIY reference counting.
Attachment #777758 -
Attachment is obsolete: true
Attachment #778390 -
Flags: review?(kyle)
Assignee | ||
Comment 21•8 years ago
|
||
Rebase only.
Attachment #777759 -
Attachment is obsolete: true
Attachment #777759 -
Flags: review?(gyeh)
Attachment #777759 -
Flags: review?(echou)
Attachment #778394 -
Flags: review?(gyeh)
Attachment #778394 -
Flags: review?(echou)
Updated•8 years ago
|
Attachment #778390 -
Flags: review?(kyle) → review+
Comment 22•8 years ago
|
||
Comment on attachment 778394 [details] [diff] [review] [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4) Review of attachment 778394 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks for answering.
Attachment #778394 -
Flags: review?(echou) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #778394 -
Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4) → [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v4)
Comment 24•8 years ago
|
||
Comment on attachment 778394 [details] [diff] [review] [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4) Review of attachment 778394 [details] [diff] [review]: ----------------------------------------------------------------- Looks great :D
Attachment #778394 -
Attachment description: [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v4) → [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4)
Attachment #778394 -
Flags: review?(gyeh) → review+
Assignee | ||
Comment 26•8 years ago
|
||
Thanks everyone! :) https://hg.mozilla.org/projects/birch/rev/4908c35e5097 https://hg.mozilla.org/projects/birch/rev/ab10a0ab68cc https://hg.mozilla.org/projects/birch/rev/f8bc8d25e15a
Whiteboard: [fixed-in-birch]
Comment 27•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4908c35e5097 https://hg.mozilla.org/mozilla-central/rev/ab10a0ab68cc https://hg.mozilla.org/mozilla-central/rev/f8bc8d25e15a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 28•8 years ago
|
||
You should never use anything from the detail namespace directly. Please change it to use AtomicRefCounted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 29•8 years ago
|
||
Sorry, didn't know. Here's the patch.
Attachment #779668 -
Flags: review?(Ms2ger)
Comment 30•8 years ago
|
||
Comment on attachment 779668 [details] [diff] [review] [04] Bug 891866: Use AtomicRefCounted for RawDBusConnection Review of attachment 779668 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.
Attachment #779668 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 31•8 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/355f77290821
Comment 32•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/355f77290821
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•