983576 - [Bluetooth] Cleanup start/stop code in BlueZ backend

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

The BlueZ backend still blocks a lot in its start/stop code and is accesses global variables on different threads. We should change the code to

  - minimize the blocking calls in the BT thread,
  - push as much init code as possible to the I/O thread, and
  - only access global variables from a single thread.

Comment 1

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

Attachment: [01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace

Comment 2

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

Attachment: [02] Bug 983576: Refactor BlueZ start/stop code

Comment 3

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

Attachment: [03] Bug 983576: Fail if sDBusConnection is not set

Comment 4

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

Attachment: [04] Bug 983576: Annotate global BlueZ variables

Comment 5

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

Comment on attachment 8391187 [details] [diff] [review]
[01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace

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

r=me with nit addressed.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +318,5 @@
>  
> +static nsresult
> +DispatchToBtThread(nsIRunnable* aRunnable)
> +{
> +  /* Due to the fact that the startup and shutdown of the Bluetooth 

nit: trailing whitespace

Comment 6

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #5)
> Comment on attachment 8391187 [details] [diff] [review]
> [01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace
> 
> Review of attachment 8391187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed.
> 
> ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> @@ +318,5 @@
> >  
> > +static nsresult
> > +DispatchToBtThread(nsIRunnable* aRunnable)
> > +{
> > +  /* Due to the fact that the startup and shutdown of the Bluetooth 
> 
> nit: trailing whitespace

Ah, it's fixed in the second patch. :)

Comment 7

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

Usually, my editor cleans up trailing white spaces for me, but I recently had to disable this feature for some files. I'll cleanup the code for the next version. :)

Comment 8

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

Comment on attachment 8391188 [details] [diff] [review]
[02] Bug 983576: Refactor BlueZ start/stop code

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

r=me with nits addressed and the question answered.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +1881,5 @@
>      MOZ_ASSERT(mConnection);
>    }
>  
>    void Run()
>    {

suggestion: please add a thread assertion at the beginning of Run().

@@ +1941,4 @@
>    bool mQueryDefaultAdapter;
>  };
>  
>  class StartBluetoothRunnable MOZ_FINAL : public nsRunnable

Question: After checked the code of StopBluetoothRunnable, I realized that most of the code originally in StopBluetoothRunnable::Run() is going to be moved to DeleteDBusConnectionTask which would be run on IO thread. My question is why we don't do the same thing to StartBluetoothRunnable? I mean can we move line 1963 ~ 1990 to StartDBusConnectionTask?

@@ +2016,5 @@
> +      // Forward this runnable to BT thread
> +      return DispatchToBtThread(this);
> +    }
> +
> +    MOZ_ASSERT(!NS_IsMainThread()); // BT thread

Hm, this line seems a bit redundant because the if-statement above is clear enough to me.

Comment 9

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

Hi Eric

suggestion: please add a thread assertion at the beginning of Run().
> 
> @@ +1941,4 @@
> >    bool mQueryDefaultAdapter;
> >  };
> >  
> >  class StartBluetoothRunnable MOZ_FINAL : public nsRunnable
> 
> Question: After checked the code of StopBluetoothRunnable, I realized that
> most of the code originally in StopBluetoothRunnable::Run() is going to be
> moved to DeleteDBusConnectionTask which would be run on IO thread. My
> question is why we don't do the same thing to StartBluetoothRunnable? I mean
> can we move line 1963 ~ 1990 to StartDBusConnectionTask?

I'm a bit confused by the line numbers here. Lines 1963 to 1978 load libbluedroid, which can block. Lines 1979 to 1990 run on the BT thread because of the call to dbus_bus_add_match at lines 1991 to 2007. The docs for dbus_bus_add_match [1] say that this function blocks if we supply an error structures as its third parameter. I'd love to move this call (and lines 1979~1990) to the I/O thread, but that would mean to leave out the error parameter. I don't know how we'd detect these errors then. libdbus sucks. :(

In the stop code there is a call to dbus_bus_remove_match, which has similar semantics. I removed the error parameter there and moved the call to the I/O thread. This simplifies the stop code considerably and I don't think that detecting this error in the stop code is strictly necessary.

Best regards
Thomas

[1] http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html#ga4eb6401ba014da3dbe3dc4e2a8e5b3ef

Comment 10

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

Comment on attachment 8391189 [details] [diff] [review]
[03] Bug 983576: Fail if sDBusConnection is not set

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

Looks good to me.

We sometimes use the name "I/O thread" and sometimes use "DBus thread" to represent the same thread in comments. Can we unify them?

Comment 11

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> Comment on attachment 8391189 [details] [diff] [review]
> [03] Bug 983576: Fail if sDBusConnection is not set
> 
> Review of attachment 8391189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> We sometimes use the name "I/O thread" and sometimes use "DBus thread" to
> represent the same thread in comments. Can we unify them?

Sure I'll add another patch to the next round of reviews. I think 'DBus thread' is the historical name, used when there was still a separate thread for DBus.

Comment 12

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

Comment on attachment 8391190 [details] [diff] [review]
[04] Bug 983576: Annotate global BlueZ variables

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

Thanks for making our code more readable!

Comment 13

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> > Comment on attachment 8391189 [details] [diff] [review]
> > [03] Bug 983576: Fail if sDBusConnection is not set
> > 
> > Review of attachment 8391189 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me.
> > 
> > We sometimes use the name "I/O thread" and sometimes use "DBus thread" to
> > represent the same thread in comments. Can we unify them?
> 
> Sure I'll add another patch to the next round of reviews. I think 'DBus
> thread' is the historical name, used when there was still a separate thread
> for DBus.

Thanks. :)

Comment 14

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

Hi Thomas,

> I'm a bit confused by the line numbers here. Lines 1963 to 1978 load
> libbluedroid, which can block. Lines 1979 to 1990 run on the BT thread
> because of the call to dbus_bus_add_match at lines 1991 to 2007.

I should avoid using line number to indicate code blocks. It's too easy to get confused. :|

What I meant "line 1963 ~ 1990":

====================

RawDBusConnection* connection = new RawDBusConnection();
nsresult rv = connection->EstablishDBusConnection();

...

for (uint32_t i = 0; i < ArrayLength(sBluetoothDBusSignals); ++i) {
  dbus_bus_add_match(connection->GetConnection(), sBluetoothDBusSignals[i], &err);
  if (dbus_error_is_set(&err)) {
    LOG_AND_FREE_DBUS_ERROR(&err);
  }
}

====================

My basic idea was moving this block to StartDBusConnectionTask::Run().

> The docs for dbus_bus_add_match [1] say that this function blocks if we
> supply an error structures as its third parameter. I'd love to move this
> call (and lines 1979~1990) to the I/O thread, but that would mean to leave
> out the error parameter. I don't know how we'd detect these errors then.
> libdbus sucks. :(
> 

Got it. So my question becomes: it sounds like you tried to avoid getting dbus_bus_add_match run on I/O thread. Do we have to do this?

Comment 15

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

Hi Eric,

I had another look at the implementation of |dbus_bus_add_match|. It just sets up a message and waits for the reply. I think this could be reimplemented by us on the I/O thread.

But I also checked what exactly happens in |EstablishDBusConnection| and that's really scary. The method opens a connection using |dbus_bus_get_private|, which does tons of address lookups, TCP connect, and similar operations. Reimplementing this in a non-blocking way means replacing most (all?) of the DBus connection setup.

Comment 16

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #12)
> Comment on attachment 8391190 [details] [diff] [review]
> [04] Bug 983576: Annotate global BlueZ variables
> 
> Review of attachment 8391190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for making our code more readable!

Sure, you're welcome. Some of these variables have at least questionable thread-safety and already indicate potential follow-up patches. :)

Comment 17

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

Attachment: [01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace (v2)

Changes since v1:

  - cleaned up white space error

Comment 18

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

Attachment: [02] Bug 983576: Refactor BlueZ start/stop code (v2)

Changes since v1:

  - cleaned up white space error
  - fix thread assertions

Comment 19

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

Attachment: [05] Bug 983576: Annotate all assertions for non-main threads in BlueZ backend

I hope I found all of them. :)

Comment 20

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

Comment on attachment 8394157 [details] [diff] [review]
[05] Bug 983576: Annotate all assertions for non-main threads in BlueZ backend

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

Thanks. :)

Comment 21

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

Finally landing this. :) Thanks a lot for the reviews!

https://hg.mozilla.org/integration/b2g-inbound/rev/427e85b4d845
https://hg.mozilla.org/integration/b2g-inbound/rev/a9ce6a058493
https://hg.mozilla.org/integration/b2g-inbound/rev/6657e25f8a67
https://hg.mozilla.org/integration/b2g-inbound/rev/1f5af3116aa8
https://hg.mozilla.org/integration/b2g-inbound/rev/1ad4c58f33c3

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=1ad4c58f33c3

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/427e85b4d845
https://hg.mozilla.org/mozilla-central/rev/a9ce6a058493
https://hg.mozilla.org/mozilla-central/rev/6657e25f8a67
https://hg.mozilla.org/mozilla-central/rev/1f5af3116aa8
https://hg.mozilla.org/mozilla-central/rev/1ad4c58f33c3