891866 - [Bluetooth] Replace RegisterAgent by non-blocking implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 891866: Cleanup RegisterLocalAgent

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 891866: Implement RegisterAgent with asyncronous functions

Comment 3

[Eric Chou [:ericchou] [:echou]]

Hi Thomas,

  I'll review this patch tomorrow. :)

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

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?

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

[Gina Yeh [:gyeh] [:ginayeh]]

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?

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 891866: Cleanup RegisterLocalAgent (v2)

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v2)

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 776235 [details] [diff] [review]
[01] Bug 891866: Cleanup RegisterLocalAgent (v2)

Review of attachment 776235 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, r=me.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 776235 [details] [diff] [review]
[01] Bug 891866: Cleanup RegisterLocalAgent (v2)

Review of attachment 776235 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 12

[Eric Chou [:ericchou] [:echou]]

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.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 891866: Support nsRefPtr<RawDBusConnection>

We need this change for holding references to our DBus connection from multiple threads.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 891866: Implement RegisterAgent with asyncronous functions (v3)

This should fix our problem for now., as we acquire the DBus connection and hold it while we're in this function.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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?

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> ::: 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. :)

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 891866: Support nsRefPtr<RawDBusConnection> (v2)

Use RefCounted<> instead of DIY reference counting.

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 891866: Implement RegisterAgent with asyncronous functions (v4)

Rebase only.

Comment 22

[Eric Chou [:ericchou] [:echou]]

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.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Gina, could you finish the review? Thanks!

Comment 24

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 25

[Gina Yeh [:gyeh] [:ginayeh]]

Just finished the review, :)

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4908c35e5097
https://hg.mozilla.org/mozilla-central/rev/ab10a0ab68cc
https://hg.mozilla.org/mozilla-central/rev/f8bc8d25e15a

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You should never use anything from the detail namespace directly. Please change it to use AtomicRefCounted.

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 891866: Use AtomicRefCounted for RawDBusConnection

Sorry, didn't know. Here's the patch.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 779668 [details] [diff] [review]
[04] Bug 891866: Use AtomicRefCounted for RawDBusConnection

Review of attachment 779668 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://hg.mozilla.org/projects/birch/rev/355f77290821

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/355f77290821