Closed Bug 891866 Opened 7 years ago Closed 6 years ago

[Bluetooth] Replace RegisterAgent by non-blocking implementation

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

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.
Attachment #773257 - Flags: review?(gyeh)
Attachment #773257 - Flags: review?(echou)
Hi Thomas,

  I'll review this patch tomorrow. :)
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?
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 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?
(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.
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)
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)
Attachment #776235 - Attachment description: [01] Bug 891866: Cleanup RegisterLocalAgent → [01] Bug 891866: Cleanup RegisterLocalAgent (v2)
Attachment #776237 - Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions → [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v2)
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 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 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.
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.
We need this change for holding references to our DBus connection from multiple threads.
Attachment #777758 - Flags: review?(kyle)
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)
Attachment #777758 - Attachment description: Bug 891866: Support nsRefPtr<RawDBusConnection> → [2] Bug 891866: Support nsRefPtr<RawDBusConnection>
Attachment #777758 - Attachment description: [2] Bug 891866: Support nsRefPtr<RawDBusConnection> → [02] Bug 891866: Support nsRefPtr<RawDBusConnection>
Attachment #777759 - Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v3) → [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v3)
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-
(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?
(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
> ::: 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. :)
Use RefCounted<> instead of DIY reference counting.
Attachment #777758 - Attachment is obsolete: true
Attachment #778390 - Flags: review?(kyle)
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)
Attachment #778390 - Flags: review?(kyle) → review+
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+
Attachment #778394 - Attachment description: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4) → [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v4)
Gina, could you finish the review? Thanks!
Flags: needinfo?(gyeh)
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+
Just finished the review, :)
Flags: needinfo?(gyeh)
You should never use anything from the detail namespace directly. Please change it to use AtomicRefCounted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, didn't know. Here's the patch.
Attachment #779668 - Flags: review?(Ms2ger)
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+
https://hg.mozilla.org/mozilla-central/rev/355f77290821
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.