830290 - Move all DBUS code to single thread

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We currently use DBus with multiple threads, which can lead to lost responses, like in bug 827888. DBus is not thread-safe and should only be used from within a single thread [1].

The workaround for bug 872888 should be OK or now, but to fix the problem we need to run all DBus related code within a single thread.

[1] http://lists.freedesktop.org/archives/dbus/2010-August/013307.html

Comment 1

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

Attachment: Cleanup DBus EventLoop function

Comment 2

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

Attachment: Added DBusPollTask

Comment 3

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

Attachment: Use DBusPollTask

Comment 4

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

Attachment: Added dispatch function for DBus thread

Comment 5

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

Attachment: Setup asyncronous DBus messages in DBus thread

Hi Eric,

Could I have some feedback from your side on the attached patches. They attempt to fix the problem of race conditions when sending asynchronous messages via DBus. The first four patches rework the DBus thread and event loop to execute as an nsRunnable, which can be preempted in favor of some other task. The final patch uses this feature to make the DBus utility function send asynchronous messages from the DBus thread. Since the event handling is not running concurrently; the race condition does not happen.

Comment 6

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

Comment on attachment 707572 [details] [diff] [review]
Setup asyncronous DBus messages in DBus thread

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

Hi Thomas,

Thanks for taking a look at this issue. I just finished review, but I'm not sure if this patch could really solve the problem.

The root cause of this issue is, in function dbus_func_send_async, we call two functions to send our message asynchronously:

(1) Call dbus_connection_send_with_reply() to get an allocated DBusPendingCall object.
(2) Call dbus_pending_call_set_notify() to define the callback function, which should be invoked after returning from remote function call.

If the message sent to remote process is handled /really/ fast, even before (2) is executed, the callback function would never be called because it has not been set yet. With your patches applied, I think this may still happen. Please feel free to let me know if I missed something, and I think it would be better if Kyle can take a look.

Kyle, could you give some feedback? Thank you.

References: 
https://bugs.freedesktop.org/show_bug.cgi?id=19796
http://lists.freedesktop.org/archives/dbus/2009-March/011031.html

Comment 7

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

Hi Eric,

Thanks for looking at the patches. They are quite complex and maybe I should have pointed out more clearly what they are supposed to do.

The fifth patch contains a new class DBusConnectionSendWithReplyTask, which does execute functions (1) and (2) in its Run method. An instance of this class is constructed by dbus_func_send_async and dispatched to the DBus thread (i.e., the one that runs the Dbus receive loop). The DBus thread will now interrupt its receive loop and execute the dispatched instance of DBusConnectionSendWithReplyTask. AFAICS receiving a DBus message depends on the DBus receive loop. And that's why we can run function (1) and (2) without receiving a reply in between.

Before interrupting itself, the receive loop dispatches an instance of itself to the end of the DBus thread's event queue (it's in patch no. 2). So after DBusConnectionSendWithReplyTask has finished, the receive loop will continue and process any reply to the message we just sent.

Comment 8

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

First off, really happy to see this patch! Moving to an interruptable poll loop via wakeups was something I was thinking about back in May-June of last year when I was first putting this together, but at the time we were still doing the weird gingerbread/ics support which meant that I didn't have the guarantee of a nice wakeup message yet and didn't want to hack it together between the platforms. I think this is a much cleaner implementation than what I had before.

Comment 9

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

Comment on attachment 707572 [details] [diff] [review]
Setup asyncronous DBus messages in DBus thread

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

Makes sense to me. Since we only poll while not doing other things (versus ALWAYS polling while possibly making other calls), this should fix the desync issue. The question is, how does this deal with the inherently blocking calls that we have running over on the BluetoothCommandThread? Do we move them to this thread and just deal with the fact that we'll sometimes block?

Comment 10

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

Hi Thomas,

Thanks for your explanation. I got your idea. :)
My only question is, do you think if it's possible to miss DBus messages during the interrupt?

Eric

Comment 11

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

I think the problem is more that they could queue and possibly overflow while something is blocking?

Comment 12

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

Hey everyone,

Thanks for your comments. I've had a look into the DBus sources and docs to find some answers to the points you raised.

1) What about blocking calls?

Distributed systems with blocking calls are just broken, because of possible DOS attacks, priority inversion, etc. It's especially broken in DBus, were incoming messages get reordered during blocked calls. [1] has a list of all the problems. So I think I the long run all blocking calls should be removed. My guess is that we can replace them with asynchronous calls in the DBusThread, and just block the BluetoothCommandThread internally until the reply has been received.

2) Could we miss some messages?

The DBusConnection structure has an internal list of outgoing messages. These messages remain in the queue until they got sent. So the DBus or Bluetooth daemon will retry again if we're not ready to receive.

3) Could we overflow the message queues?

The message queues are linked lists. We should be fine as long as there is memory available.

[1] http://smcv.pseudorandom.co.uk/2008/11/nonblocking/

Comment 13

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

Response to 1:

I think the reason I had blocking calls in there was because I was using the android stuff as a template, which also mixed calls. It'd probably be a good idea to go through and make a list of what we call where, and how it's called, and just start filing bugs on all the things we need to fix. I'm happy to help out with this.

Response to 3:

Ah, ok. I'd just heard there was ways to DOS dbus via message overloading (this project was my first dbus experience), though I don't see how we'd manage that with this system.

General Response:

We might want to start thinking about this in wider terms, too. I would figure we're dropping dbus for whatever android's new bluetooth stack is when we move to a 4.2 base, but dbus bluetooth may stay in for linux WebAPI support, and there's other pieces of the system that use dbus on linux (Power, network). Might be worth looking at trying to centralize all of these to the core we've written here, though don't know how much work that would be or if it'd even be worth it. Just something to ponder for the future.

Comment 14

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

Hi Kyle,

Sorry if point 1 sounded offensive. It was definitely not meant this way. I just wanted to point out that we should switch to non-blocking calls where ever possible. Actually its quite amazing how well DBus, Bluetooth, UnixSocket, etc work together, given the size and complexity of the code.

Just out of interest, how would that DOS work?

Do we use a widget toolkit on Linux (e.g., Gtk+)? I think these toolkits already handle DBus in there main loops and it might be possible to use them for talking to services, instead of running our own DBus thread. But that's definitely not something for the near term.

Comment 15

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

(In reply to Thomas Zimmermann [:tzimmermann] from comment #14)
> Sorry if point 1 sounded offensive. It was definitely not meant this way. I
> just wanted to point out that we should switch to non-blocking calls where
> ever possible. Actually its quite amazing how well DBus, Bluetooth,
> UnixSocket, etc work together, given the size and complexity of the code.

Oh, didn't sound offensive at all! I was just giving you background as to why things are the way they are, and basically saying "If anything looks seriously out of whack, say so". I had trouble finding advice internally on a lot of the dbus stuff so I just kinda learned as I went. :)

> Just out of interest, how would that DOS work?

As far as I (vaguely) remember it, you load up so many messages into the queue that it just starts delaying other calls and tasks, which means you get more timeouts, failures cascade, etc... I think that case is more for systems that are using dbus for everything (and lord knows we don't wanna be openmoko :) ), so my worries there are unfounded.
 
> Do we use a widget toolkit on Linux (e.g., Gtk+)? I think these toolkits
> already handle DBus in there main loops and it might be possible to use them
> for talking to services, instead of running our own DBus thread. But that's
> definitely not something for the near term.

On linux, we use gtk+ and the glib main loop takes care of our dbus stuff for us. That's why I built the BluetoothService architecture the way I did, so we can replace calls as needed if we're on systems that use different APIs.

Didn't really want to use raw DBus for FxOS in the first place, but it was our only choice at the time since libdbus++ hadn't had its exceptions cut out by the chromium team yet. I'm assuming that project is done now though, so if we ever wanted to clean this up a bit more, there's that option, but like I said, I think a lot of this will disappear once we have to move to 4.2 and the new android stack.

Comment 16

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

FYI: the current patches don't completely solve the reported problem, but provide the necessary infrastructure and fix the resulting bug. The plan is to get them r+'ed and landed until March, 15.

Comment 17

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

Attachment: Cleanup DBus EventLoop function

Comment 18

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

Attachment: Added DBusPollTask

Comment 19

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

Attachment: Added DBusPollTask

Comment 20

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

Attachment: Use DBusPollTask

Comment 21

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

Attachment: Added dispatch function for DBus thread

Comment 22

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

Attachment: Setup asyncronous DBus messages in DBus thread

Comment 23

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

Kyle, please review the attached patches. They are mostly the same as the old one's, but I added better error handling to the system functions and cleaned up the "main loop" in DBusPollTask::Run. Thanks!

Comment 24

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

Comment on attachment 718990 [details] [diff] [review]
Use DBusPollTask

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

::: ipc/dbus/DBusThread.cpp
@@ +472,5 @@
>    DBusThread* mConnection;
>  };
>  
> +static nsAutoPtr<DBusThread> sDBusThread;
> +static nsCOMPtr<nsIThread>   sDBusServiceThread;

This isn't valid. We can't have static COMPtrs like this because XPCOM won't have started up during static initialization.

Comment 25

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

Attachment: Cleanup DBus EventLoop function

Comment 26

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

Attachment: Added DBusPollTask

Comment 27

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

Attachment: Use DBusPollTask

I replaced the nsCOMPtr with an nsRefPtr.

Comment 28

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

Attachment: Added dispatch function for DBus thread

Comment 29

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

Attachment: Setup asyncronous DBus messages in DBus thread

Comment 30

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

I think we shouldn't commit this before Bluetooth on m-c is working again.

Comment 31

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

Comment on attachment 719880 [details] [diff] [review]
Use DBusPollTask

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

r=me with one more type change to yet another refptr because you can never have too many refptr types! :|

::: ipc/dbus/DBusThread.cpp
@@ +472,5 @@
>    DBusThread* mConnection;
>  };
>  
> +static nsAutoPtr<DBusThread> sDBusThread;
> +static nsRefPtr<nsIThread>   sDBusServiceThread;

StaticRefPtr. Then we're good. :)

Comment 32

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

Attachment: Use DBusPollTask

Hi Kyle,

The interface of StaticRefPtr (and StaticAutoPtr) is quite limited. So when changing the code to use StaticRefPtr, I had to update the start and stop functions as well. I'd appreciate another review from your side. Thanks!

Comment 33

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

FYI, I have several more patches for this issue in the queue. They move the sending operations to the DBus thread, similar to what no. 5 does with dbus_connection_send_with_reply.

Comment 34

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

Attachment: Use DBusPollTask

The previous patch accidentally free'd the sDBusThread at the end of the start function.

Comment 35

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

In the interest of getting a few more eyes on the shifts in thread usage around bt, I'm throwing bent in as a addl review on these patches.

Comment 36

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

(In reply to Thomas Zimmermann [:tzimmermann] from comment #33)
> FYI, I have several more patches for this issue in the queue. They move the
> sending operations to the DBus thread, similar to what no. 5 does with
> dbus_connection_send_with_reply.

Can you go ahead and start posting these in other bugs? The more thread issues gregor finds, the more I think this is our solution. :)

Comment 37

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

Attachment: [01] Cleanup DBus EventLoop function

Rebased on tip

Comment 38

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

Attachment: [02] Added DBusPollTask

Rebased on tip

Comment 39

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

Attachment: [03] Use DBusPollTask

Rebased on tip

Comment 40

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

Attachment: [04] Added dispatch function for DBus thread

Rebased on tip

Comment 41

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

Attachment: [05] Setup asyncronous DBus messages in DBus thread

Comment 42

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

Attachment: [06] Cleanup DBusConnectionSendWithReplyTask

Comment 43

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

Attachment: [07] Execute DBus send operation in DBus thread

Comment 44

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

Attachment: [08] Replace dbus_connection_send by dbus_func_send

Comment 45

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

Attachment: [09] Send blocking DBus messages from within DBus thread

Comment 46

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #36)
> (In reply to Thomas Zimmermann [:tzimmermann] from comment #33)
> > FYI, I have several more patches for this issue in the queue. They move the
> > sending operations to the DBus thread, similar to what no. 5 does with
> > dbus_connection_send_with_reply.
> 
> Can you go ahead and start posting these in other bugs? The more thread
> issues gregor finds, the more I think this is our solution. :)

In other tickets we also often see problems with Unix sockets or Bluetooth, right? These patches here only concern the internal DBus implementation, but maybe they help. :)  Actually, I'd like to get them merged soon, because it feels a bit painful to maintain them on top of tip. Every time I do a 'hg pull' I fear to run into merge conflicts.

Comment 47

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

(In reply to Thomas Zimmermann [:tzimmermann] from comment #46)
> In other tickets we also often see problems with Unix sockets or Bluetooth,
> right? These patches here only concern the internal DBus implementation, but
> maybe they help. :)  Actually, I'd like to get them merged soon, because it
> feels a bit painful to maintain them on top of tip. Every time I do a 'hg
> pull' I fear to run into merge conflicts.

Sure, we're seeing them in unix sockets or bluetooth, but they're mostly about thread races, so we're trying to reduce the overall thread count as much as possible. Being able to move BluetoothDBusServer to a single DBusConnection and get rid of the BluetoothCommandThread in BluetoothServer may fix things we haven't even run into yet. :)

Comment 48

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

Hey Kyle!

I currently try to replace dbus_connection_send_and block with dbus_connection_send_with_reply. This will allow us to block a single thread while waiting for a reply without blocking the whole DBus connection. But dbus_connection_send_and_block returns an optional DBusError. Do you know how I can get an error structure in the case of dbus_connection_send_with_reply. The documentation of this function has not been overly helpful to me. :(

Comment 49

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

(In reply to Thomas Zimmermann [:tzimmermann] from comment #48)
> Hey Kyle!
> 
> I currently try to replace dbus_connection_send_and block with
> dbus_connection_send_with_reply. This will allow us to block a single thread
> while waiting for a reply without blocking the whole DBus connection. But
> dbus_connection_send_and_block returns an optional DBusError. Do you know
> how I can get an error structure in the case of
> dbus_connection_send_with_reply. The documentation of this function has not
> been overly helpful to me. :(

This was part of why I went with the blocking approach in the first place, I just could not figure out how to get a decent error return from the non-blocking functions. DBus's API is so schizo. :/

I'll take a look again, it's been 10 months since I last tried this. :)

Comment 50

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

I just looked at the implementation of dbus_connection_send_with_block, and it only does a handful of trivial checks for setting the error value. It should be easy for me to do the same in the helper function.

Comment 51

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

Also: can you number your patches in the descriptions? (i.e. Patch 1, Patch 2, etc...). Just makes it easier to make sure I'm following the patch order correctly, which I'm assumign is chronological currently. :)

Comment 52

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

Attachment: [10] Replace calls to dbus_connection_send_with_reply_and_block

Comment 53

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

Attachment: [11] Don't block DBus while waiting for replies

Comment 54

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

I think that these 11 patches fix the problems in our DBus implementation. The DBus thread is now interruptible and allows for the execution of DBus commands. All send operations have been moved to the DBus thread and can be executed concurrently until their results are needed. The use of dbus_connection_send_with_reply_and_block has been replaced by an implementation that doesn't block the whole DBus.

I'd first like to get these patches landed; afterwards there are still cleanups possible here and the interface could be adapted to our conventions.

Comment 55

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

Awesome! I'm hoping to pair review this with bent in the next couple of days.

Comment 56

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

Attachment: [02] Added DBusPollTask (v2)

Fixes DEBUG.

Comment 57

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

Attachment: [07] Execute DBus send operation in DBus thread (v2)

Fixes Monitor usage.

Comment 58

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

Comment on attachment 722816 [details] [diff] [review]
[06] Cleanup DBusConnectionSendWithReplyTask

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

::: ipc/dbus/DBusUtils.cpp
@@ +99,5 @@
>                                                            &call,
>                                                            mTimeout);
>      NS_ENSURE_TRUE(success == TRUE, NS_ERROR_FAILURE);
>  
> +    success = dbus_pending_call_set_notify(call, Notify, data, NULL);

Supernit: I realize Notify is in the scope here, but it's not immediately obvious. Might declare it before this block?

Comment 59

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

Comment on attachment 722822 [details] [diff] [review]
[09] Send blocking DBus messages from within DBus thread

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

Might go ahead and file a followup on the async functionality?

::: ipc/dbus/DBusUtils.cpp
@@ +430,5 @@
> +                         int aTimeout,
> +                         DBusMessage** aReply,
> +                         DBusError* aError,
> +                         DBusMessage* aMessage)
> +{

Since this blocks, do we want some sort of thread assertion on it, or are we expecting that from callers?

Comment 60

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

Hi Thomas, just a reminding, please remember to reconsider if we need to undo Bug 841336 after this bug gets fixed. Thanks.

Comment 61

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

@Kyle: I'll fix the things you mentioned once bent's reviews are in.

@Eric: Sure, I'll keep it in mind.

Comment 62

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

Thomas, can you maybe throw an all-in-one patch up too? I'd kinda like to see everything in its finished state via splinter (and bent actually likes reviewing single, large patches better. He's weird like that.)

Comment 63

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

Attachment: All-in-one patch (for review only)

Hey! This is the all-in-one patch. I wouldn't like a patch like this one to be committed to the tree, so please use it for the review only.

Comment 64

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sorry for the delay here (MMS was due last week...) but I'm halfway done looking through this. Comments soon!

Comment 65

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 729599 [details] [diff] [review]
All-in-one patch (for review only)

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

Sorry that took so long! Lots of comments follow.

It's hard to tell how many of DBusThread.cpp comments are due to new code that you added or simply to code that has moved, so please don't feel like we have to fix everything right this minute. Followups for some of this would be totally fine.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +673,1 @@
>      dbus_message_unref(reply);

Wait, we don't need anything out of the reply here? If that's true then why wouldn't we just send asynchronously?

::: ipc/dbus/DBusThread.cpp
@@ +123,2 @@
>    // Thread members
>    nsCOMPtr<nsIThread> mThread;

This is now totally unused, right?

@@ +311,5 @@
> +  // android code, but we break them out into class members here.
> +  // Therefore we read into a local array and then copy.
> +
> +  int sockets[2];
> +  if (socketpair(AF_LOCAL, SOCK_STREAM, 0, (int*)(&sockets)) < 0) {

You should be able to just pass 'sockets' for the last arg.

@@ +320,5 @@
> +  mControlFdW.rwget() = sockets[1];
> +  pollfd p;
> +  p.fd = mControlFdR.get();
> +  p.events = POLLIN;
> +  mPollData.AppendElement(p);

I know that the old code did this, but this would be better written as:

  pollfd* p = mPollData.AppendElement();
  p->fd = ...;
  p->events = ...;

And shouldn't we set revents to 0 just to be safe?

@@ +326,5 @@
> +  // Due to the fact that mPollData and mWatchData have to match, we
> +  // push a null to the front of mWatchData since it has the control
> +  // fd in the first slot of mPollData.
> +
> +  mWatchData.AppendElement((DBusWatch*)NULL);

nullptr here and everywhere else.

@@ +332,1 @@
>    dbus_threads_init_default();

We should probably move this to RawDBusConnection::EstablishDBusConnection, don't you think?

@@ +332,3 @@
>    dbus_threads_init_default();
>    DBusError err;
>    dbus_error_init(&err);

This looks unused.

@@ +340,5 @@
>      return false;
>    }
>  
> +  dbus_connection_set_watch_functions(mConnection, AddWatch,
> +                                      RemoveWatch, ToggleWatch, this, NULL);

According to the docs this can fail.

@@ +351,5 @@
>  DBusThread::TearDownData()
>  {
> +  dbus_connection_set_wakeup_main_function(mConnection, NULL, NULL, NULL);
> +  dbus_connection_set_watch_functions(mConnection,
> +                                      NULL, NULL, NULL, NULL, NULL);

Here too, though at this point you'd probably just print a warning if it failed.

@@ +390,5 @@
>  
> +bool
> +DBusThread::WakeUp()
> +{
> +  char control = DBUS_EVENT_LOOP_WAKEUP;

This should be static const.

@@ +411,5 @@
> +  DBusPollTask(DBusThread *aConnection)
> +  : mConnection(aConnection)
> +  { }
> +
> +  NS_IMETHOD Run()

Which thread is this supposed to run on?

@@ +420,5 @@
> +    do {
> +      int res = TEMP_FAILURE_RETRY(poll(mConnection->mPollData.Elements(),
> +                                        mConnection->mPollData.Length(),
> +                                        -1));
> +      NS_ENSURE_TRUE(res >= 0, NS_OK);

This means that a single error will keep us from ever re-polling, right? That seems suboptimal but I guess we can always tackle that later.

Also, you should be able to assert that res is never 0.

@@ +422,5 @@
> +                                        mConnection->mPollData.Length(),
> +                                        -1));
> +      NS_ENSURE_TRUE(res >= 0, NS_OK);
> +
> +      for (uint32_t i = 0; i < mConnection->mPollData.Length(); i++) {

There's a bug here, we'll skip the next fd if we go into the HandleWatchRemove() call below... I guess this was present before :(

@@ +430,5 @@
> +
> +        if (mConnection->mPollData[i].fd == mConnection->mControlFdR.get()) {
> +          char data;
> +          res = TEMP_FAILURE_RETRY(read(mConnection->mControlFdR.get(), &data, sizeof(char)));
> +          NS_ENSURE_TRUE(res >= 0, NS_OK);

I think here again you can assert that res is never 0, but you'd need to handle negative values. Bailing out seems suboptimal again because we won't re-dispatch this runnable.

@@ +435,5 @@
> +
> +          switch (data) {
> +          case DBUS_EVENT_LOOP_EXIT:
> +            exitThread = true;
> +            break;

Why not just return here? I don't see why we'd waste time processing everything else in our queues if it's time to shut down.

@@ +443,5 @@
> +          case DBUS_EVENT_LOOP_REMOVE:
> +            HandleWatchRemove(mConnection);
> +            break;
> +          case DBUS_EVENT_LOOP_WAKEUP:
> +            exitTask = true;

Hm. It seems to me that you could avoid the re-dispatch stuff below if you just call NS_ProcessNextEvent() here. That would make this logic a little clearer I think. If there's no event to process then the return will be false and you'll know that DBusWakeup was called (if that makes any difference... I don't understand how that currently helps us at all).

@@ +446,5 @@
> +          case DBUS_EVENT_LOOP_WAKEUP:
> +            exitTask = true;
> +            // noop
> +            break;
> +          default:

All of this warning stuff should be #ifdef DEBUG. And no need for a break statement in the default handler here.

@@ +468,5 @@
> +      {}
> +    } while ( !(exitTask || exitThread) );
> +
> +    if (!exitThread) {
> +      nsAutoPtr<nsIRunnable> pollTask(new DBusPollTask(mConnection));

This should be using an nsCOMPtr (DBusPollTask is refcounted and you're using an interface pointer).

However, why make a new one? Why not just re-dispatch 'this'?

@@ +478,5 @@
> +      nsIThread *thread = NULL;
> +      if (NS_GetCurrentThread(&thread) != NS_OK) {
> +        return NS_OK;
> +      }
> +      thread->Dispatch(pollTask, NS_DISPATCH_NORMAL);

NS_DispatchToCurrentThread handles all this for you.

@@ +490,5 @@
> +  DBusThread* mConnection;
> +};
> +
> +static StaticAutoPtr<DBusThread> sDBusThread;
> +static StaticRefPtr<nsIThread>   sDBusServiceThread;

Nit: In general 's' stands for 'static member'. These are more accurately globals, so 'g' would be better. I know the old code had it with 's', but since we're fixing everything might as well do this too.

@@ +503,5 @@
> +
> +  nsAutoPtr<DBusThread> dbusThread(new DBusThread());
> +  NS_ENSURE_TRUE(dbusThread, false);
> +
> +  bool eventLoopStarted = dbusThread->StartEventLoop();

The order of this (and, similarly, in StopDBus below) seems wrong. Conceptually an event loop requires a thread, and you're starting the event loop before you start the thread. I see why the functions are called in this order but maybe StartEventLoop is just poorly named now that you've removed the third thread. StartEventLoop/StopEventLoop should probably become something like Init and Cleanup.

@@ +520,5 @@
> +
> +  nsRefPtr<nsIRunnable> pollTask(new DBusPollTask(dbusThread));
> +  NS_ENSURE_TRUE(pollTask, false);
> +
> +  nsresult rv = sDBusServiceThread->Dispatch(pollTask, NS_DISPATCH_NORMAL);

You already have an 'nsresult rv' above, why not just declare it once?

@@ +538,5 @@
> +
> +  if (sDBusThread) {
> +    static const char data = DBUS_EVENT_LOOP_EXIT;
> +    ssize_t wret = TEMP_FAILURE_RETRY(write(sDBusThread->mControlFdW.get(),
> +                                            &data, sizeof(char)));

'sizeof(data)' is better.

@@ +564,5 @@
> +  return true;
> +}
> +
> +nsresult
> +DispatchToDBusThread(nsIRunnable *event, uint32_t flags)

It looks like you always pass NS_DISPATCH_NORMAL as the flags here... Any reason not to just eliminate the second arg and always pass NS_DISPATCH_NORMAL in the call to Dispatch below? I don't think you'd ever want to allow any other flags here.

::: ipc/dbus/DBusThread.h
@@ +6,5 @@
>  
>  #ifndef mozilla_ipc_dbus_gonk_dbusthread_h__
>  #define mozilla_ipc_dbus_gonk_dbusthread_h__
>  
> +#include "nsError.h"

You probably want "nscore.h" here. You don't specifically need any error code in this header, right?

@@ +35,5 @@
> + * Dispatch an event to the DBus thread
> + *
> + * @param event An nsIRunnable to run in the DBus thread
> + * @param flags Flags for dispatching the event
> + * @return NS_OK on success, or an error cod eotherwise

Nit: "code otherwise"

::: ipc/dbus/DBusUtils.cpp
@@ +22,5 @@
> +#include "mozilla/Monitor.h"
> +#include "nsAutoPtr.h"
> +#include "nsThreadUtils.h"
> +#include "DBusThread.h"
> +#include "DBusUtils.h"

You should always (well, I guess there are a few exceptions, but not here) make your own header the first include line in your cpp file so that you know your header compiles on its own and can be included elsewhere.

@@ +52,5 @@
>    }
>    dbus_error_free((err));
>  }
>  
> +class DBusConnectionSendRunnableBase : public nsRunnable

What is the mechanism that ensures mConnection here is always valid? Is there some higher-order logic that guarantees it?

@@ +55,5 @@
>  
> +class DBusConnectionSendRunnableBase : public nsRunnable
> +{
> +public:
> +  virtual ~DBusConnectionSendRunnableBase()

In general all refcounted classes should have private or protected destructors to prevent people from deleting them (the destructor should should only run within the Release() method). Since this is meant to be a base class it should be protected.

@@ +61,3 @@
>  
> +protected:
> +  DBusConnectionSendRunnableBase(DBusConnection* aConnection,

What thread is this supposed to run on?

@@ +72,5 @@
> +  DBusConnection   *mConnection;
> +  DBusMessageRefPtr mMessage;
> +};
> +
> +class DBusConnectionSendSyncRunnable : public DBusConnectionSendRunnableBase

There's a deadlock hazard here. Our condition variables do not stay "signaled" for any period of time.

In the normal way you use this thing you do something like this:

  nsRefPtr<SyncRunnable> t = new DerivedSyncRunnable();
  DispatchToDBusThread(t);
  t->WaitForCompletion();

Then, on the DBus thread, you basically have:

  // Do something blocking
  Completed(success);

In the normal case the blocking operation on the DBus thread will take longer than the main thread and you'll be fine. But this ordering is also possible:

  1. Main thread -> DispatchToDBusThread()
  2. Context switch
  3. DBus thread -> // Blocking stuff
  4. DBus thread -> Completed()
  5. Context switch
  6. Main thread -> WaitForCompletion()

In this case you will deadlock.

The usual pattern we use (something I forgot to cover in Half Moon Bay!) is like this:

  class DBusConnectionSendSyncRunnable
  {
    // ...
    Monitor mMonitor;
    bool mSuccess;
    bool mCompleted;

  public:
    bool WaitForCompletion()
    {
      MonitorAutoLock autoLock(mMonitor);
      while (!mCompleted) {
        mMonitor.Wait();
      }
      return mSuccess;
    }

  protected:
    void Completed(bool aSuccess)
    {
      MonitorAutoLock autoLock(mMonitor);
      MOZ_ASSERT(!mCompleted);
      mSuccess = aSuccess;
      mCompleted = true;
      mMonitor.Notify();
    }
  };

You'll need an additional state bit like that to know if you should wait or not.

@@ +78,5 @@
> +public:
> +  virtual ~DBusConnectionSendSyncRunnable()
> +  { }
> +
> +  bool WaitForCompletion()

Since the purpose of this method is to block please assert that it never happens on the main thread.

@@ +86,5 @@
> +    return mSuccess;
> +  }
> +
> +protected:
> +  DBusConnectionSendSyncRunnable(DBusConnection* aConnection,

What thread is this supposed to run on?

@@ +99,5 @@
> +  void Completed(bool aSuccess)
> +  {
> +    MonitorAutoLock autoLock(mCompleted);
> +    mSuccess = aSuccess;
> +    mCompleted.NotifyAll();

Since there's only one thread that should ever be waiting on this runnable a single Notify() should be sufficient.

@@ +110,5 @@
> +
> +class DBusConnectionSendRunnable : public DBusConnectionSendSyncRunnable
> +{
> +public:
> +  DBusConnectionSendRunnable(DBusConnection* aConnection,

What thread is this supposed to run on?

@@ +117,5 @@
> +  : DBusConnectionSendSyncRunnable(aConnection, aMessage),
> +    mSerial(aSerial)
> +  { }
> +
> +  NS_METHOD Run()

All these Run methods need to be NS_IMETHOD, not NS_METHOD.

And what thread is this supposed to run on?

@@ +120,5 @@
> +
> +  NS_METHOD Run()
> +  {
> +    dbus_bool_t success = dbus_connection_send(mConnection, mMessage, mSerial);
> +    Completed(success == TRUE);

This is a little awkward. You're supposed to pass a bool to Completed() so this == is probably the most correct. But then whenever you call WaitForCompletion() (which returns this same bool) you invariably end up having to convert it back to TRUE/FALSE. Can you just store dbus_bool_t?

@@ +133,5 @@
> +};
> +
> +dbus_bool_t
> +dbus_func_send(DBusConnection *aConnection, dbus_uint32_t *aSerial,
> +               DBusMessage *aMessage)

This is more of a nit, but you've got the * on the left in a bunch of places in this file, and then you've got them on the right in others. Either way is fine but you should be consistent.

@@ +137,5 @@
> +               DBusMessage *aMessage)
> +{
> +  nsRefPtr<DBusConnectionSendRunnable> t(
> +    new DBusConnectionSendRunnable(aConnection, aMessage, aSerial));
> +  if (!t) {

We've switched over to an infallible malloc/new (by default, you can opt out at the call site), so this branch is dead code. There are lots of places where you check the result of new below so all of them need updating.

@@ +147,5 @@
> +
> +  nsresult rv = DispatchToDBusThread(t, NS_DISPATCH_NORMAL);
> +
> +  if (NS_FAILED(rv)) {
> +    if (aMessage) {

You assert in the DBusConnectionSendRunnableBase constructor that the passed in message is never null. If that's the case then this check seems unnecessary, right?

@@ +164,4 @@
>  }
>  
> +static dbus_bool_t
> +dbus_func_args_send_valist(DBusConnection *aConnection,

Looks like you can assert a lot of stuff about the arguments here. And the thread that this is supposed to run on?

@@ +178,5 @@
> +                                                      aInterface,
> +                                                      aFunction);
> +  if (message == NULL) {
> +    LOG("Could not allocate D-Bus message object!");
> +    goto done;

In general "goto" is highly discouraged. Here it indicates the need for some kind of smart pointer for DBusMessage. Can we switch all this to use nsAutoRef? I see that someone made DBusMessageRefPtr too, maybe that would be better? Let's try to remove goto everywhere in this file.

@@ +197,5 @@
> +  return FALSE;
> +}
> +
> +dbus_bool_t
> +dbus_func_args_send(DBusConnection *aConnection,

This needs lots of assertions too.

@@ +222,5 @@
> +
> +class DBusConnectionSendWithReplyRunnable : public DBusConnectionSendRunnableBase
> +{
> +public:
> +  DBusConnectionSendWithReplyRunnable(DBusConnection* aConnection,

You should be able to assert a non-null callback here, right? And what thread is this supposed to run on?

@@ +233,5 @@
> +    mData(aData),
> +    mTimeout(aTimeout)
> +  { }
> +
> +  NS_METHOD Run()

What thread is this supposed to run on?

@@ +251,5 @@
> +    success = dbus_pending_call_set_notify(call, Notify, data, NULL);
> +    NS_ENSURE_TRUE(success == TRUE, NS_ERROR_FAILURE);
> +
> +    data.forget();
> +    dbus_message_unref(mMessage);

Hm, mMessage is a DBusMessageRefPtr, and it automatically calls dbus_message_unref. Will this be a double-unref?

@@ +258,5 @@
> +  };
> +
> +private:
> +
> +  class NotifyData

It looks like this class is kind of redundant. You could just add an extra reference to DBusConnectionSendWithReplyRunnable and pass 'this' to dbus_pending_call_set_notify, right?

@@ +268,5 @@
> +    { }
> +
> +    void RunNotifyCallback(DBusMessage *aMessage)
> +    {
> +      if (mCallback) {

Seems like this should always be non-null, right?

@@ +278,5 @@
> +    void (*mCallback)(DBusMessage*, void*);
> +    void *mData;
> +  };
> +
> +  static void Notify(DBusPendingCall *aCall, void *aData)

What thread is this supposed to run on?

@@ +280,5 @@
> +  };
> +
> +  static void Notify(DBusPendingCall *aCall, void *aData)
> +  {
> +    nsAutoPtr<NotifyData> data(reinterpret_cast<NotifyData*>(aData));

static_cast when casting from void*.

@@ +286,5 @@
> +    // This is guaranteed to be non-NULL, because this function is
> +    // called only once, when the remote method invokation returns.
> +    DBusMessage *reply = dbus_pending_call_steal_reply(aCall);
> +
> +    if (reply) {

The comment above says "guaranteed to be non-NULL", so why are you checking here?

@@ +381,5 @@
>    va_end(lst);
>    return ret;
>  }
>  
> +class DBusConnectionSendAndBlockRunnable : public DBusConnectionSendSyncRunnable

So what's the difference between DBusConnectionSendRunnable and DBusConnectionSendAndBlockRunnable? They both inherit DBusConnectionSendSyncRunnable and they both block (although DBusConnectionSendRunnable only does so if it has a "serial"?).

In general it looks like you have three different leaf classes, and their main differences are:

  1. DBusConnectionSendWithReplyRunnable - lock free, callback happens on DBus thread?
  2.  DBusConnectionSendRunnable - maybe blocks if a serial is passed, no callback
  3. DBusConnectionSendAndBlockRunnable - always blocks, waits for reply, no callback

Did I get that right? If so, those major differences are difficult to discern based on the names of the classes...

Comment 66

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

Hey Ben!

> Sorry that took so long! Lots of comments follow.

Thanks for the review. I'll update the patch set asap.

> It's hard to tell how many of DBusThread.cpp comments are due to new code
> that you added or simply to code that has moved, so please don't feel like
> we have to fix everything right this minute. Followups for some of this
> would be totally fine.
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +673,1 @@
> >      dbus_message_unref(reply);
> 
> Wait, we don't need anything out of the reply here? If that's true then why
> wouldn't we just send asynchronously?

This patch set is only about refactoring our implementation of the DBus system, so I tried to not touch the interface as far as possible. For this specific function here, I already work on bug 853550 to remove the Bluetooth command thread. This will remove all blocking DBus calls from the code base.

> ::: ipc/dbus/DBusThread.cpp
> @@ +123,2 @@
> >    // Thread members
> >    nsCOMPtr<nsIThread> mThread;
> 
> This is now totally unused, right?

Removed.

> @@ +311,5 @@
> > +  // android code, but we break them out into class members here.
> > +  // Therefore we read into a local array and then copy.
> > +
> > +  int sockets[2];
> > +  if (socketpair(AF_LOCAL, SOCK_STREAM, 0, (int*)(&sockets)) < 0) {
> 
> You should be able to just pass 'sockets' for the last arg.

Fixed.

> @@ +320,5 @@
> > +  mControlFdW.rwget() = sockets[1];
> > +  pollfd p;
> > +  p.fd = mControlFdR.get();
> > +  p.events = POLLIN;
> > +  mPollData.AppendElement(p);
> 
> I know that the old code did this, but this would be better written as:
> 
>   pollfd* p = mPollData.AppendElement();
>   p->fd = ...;
>   p->events = ...;

This looks much nicer. :)

> And shouldn't we set revents to 0 just to be safe?

Actually it's an output parameter, but I added that line anyway.

> @@ +326,5 @@
> > +  // Due to the fact that mPollData and mWatchData have to match, we
> > +  // push a null to the front of mWatchData since it has the control
> > +  // fd in the first slot of mPollData.
> > +
> > +  mWatchData.AppendElement((DBusWatch*)NULL);
> 
> nullptr here and everywhere else.

Fixed.

> @@ +332,1 @@
> >    dbus_threads_init_default();
> 
> We should probably move this to RawDBusConnection::EstablishDBusConnection,
> don't you think?

That's a good idea. I moved the code to EstablishDBusConnection for now. In a later patch for cleaning up the interfaces, I'll probably move this call to an utility function.

> 
> @@ +332,3 @@
> >    dbus_threads_init_default();
> >    DBusError err;
> >    dbus_error_init(&err);
> 
> This looks unused.

Removed.

> @@ +340,5 @@
> >      return false;
> >    }
> >  
> > +  dbus_connection_set_watch_functions(mConnection, AddWatch,
> > +                                      RemoveWatch, ToggleWatch, this, NULL);
> 
> According to the docs this can fail.
> 
> @@ +351,5 @@
> >  DBusThread::TearDownData()
> >  {
> > +  dbus_connection_set_wakeup_main_function(mConnection, NULL, NULL, NULL);
> > +  dbus_connection_set_watch_functions(mConnection,
> > +                                      NULL, NULL, NULL, NULL, NULL);
> 
> Here too, though at this point you'd probably just print a warning if it
> failed.
> 
> @@ +390,5 @@
> >  
> > +bool
> > +DBusThread::WakeUp()
> > +{
> > +  char control = DBUS_EVENT_LOOP_WAKEUP;
> 
> This should be static const.

Fixed.

> @@ +411,5 @@
> > +  DBusPollTask(DBusThread *aConnection)
> > +  : mConnection(aConnection)
> > +  { }
> > +
> > +  NS_IMETHOD Run()
> 
> Which thread is this supposed to run on?

The DBus thread. For now, I added an assertion for !NS_IsMainThread()

> 
> @@ +420,5 @@
> > +    do {
> > +      int res = TEMP_FAILURE_RETRY(poll(mConnection->mPollData.Elements(),
> > +                                        mConnection->mPollData.Length(),
> > +                                        -1));
> > +      NS_ENSURE_TRUE(res >= 0, NS_OK);
> 
> This means that a single error will keep us from ever re-polling, right?
> That seems suboptimal but I guess we can always tackle that later.

Well, I'm not sure what else we could do here. The poll manpage lists a number of errors, but none can be solved easily. The only exception is EINTR, which we already handle. If we go for fault tolerance, we could restart the DBus system from scratch. But that's not something i'd like to handle in this patch set.

> Also, you should be able to assert that res is never 0.

Fixed.
 
> @@ +422,5 @@
> > +                                        mConnection->mPollData.Length(),
> > +                                        -1));
> > +      NS_ENSURE_TRUE(res >= 0, NS_OK);
> > +
> > +      for (uint32_t i = 0; i < mConnection->mPollData.Length(); i++) {
> 
> There's a bug here, we'll skip the next fd if we go into the
> HandleWatchRemove() call below... I guess this was present before :(

Yes, you're right. Fixed.

> @@ +430,5 @@
> > +
> > +        if (mConnection->mPollData[i].fd == mConnection->mControlFdR.get()) {
> > +          char data;
> > +          res = TEMP_FAILURE_RETRY(read(mConnection->mControlFdR.get(), &data, sizeof(char)));
> > +          NS_ENSURE_TRUE(res >= 0, NS_OK);
> 
> I think here again you can assert that res is never 0, but you'd need to
> handle negative values. Bailing out seems suboptimal again because we won't
> re-dispatch this runnable.
> 
> @@ +435,5 @@
> > +
> > +          switch (data) {
> > +          case DBUS_EVENT_LOOP_EXIT:
> > +            exitThread = true;
> > +            break;
> 
> Why not just return here? I don't see why we'd waste time processing
> everything else in our queues if it's time to shut down.

Suppose we stop DBus because the user turned of Bluetooth. There might still be messages waiting in the DBus queue, and some component of the system might wait for them. Although I don't have an actual real-world example, I'd prefer to not change the code here. And the overhead is probably not that high.

If we want to change anything in the shutdown logic, I'd rather make the dispatch function shutdown-aware. I think that currently threads can dispatch messages when the DBus thread is already shutting down.


To be continued...

Comment 67

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

(In reply to ben turner [:bent] from comment #65)

> @@ +430,5 @@
> > +
> > +        if (mConnection->mPollData[i].fd == mConnection->mControlFdR.get()) {
> > +          char data;
> > +          res = TEMP_FAILURE_RETRY(read(mConnection->mControlFdR.get(), &data, sizeof(char)));
> > +          NS_ENSURE_TRUE(res >= 0, NS_OK);
> 
> I think here again you can assert that res is never 0, but you'd need to
> handle negative values. Bailing out seems suboptimal again because we won't
> re-dispatch this runnable.

Like in the  case of poll, it occurs to me we can't do much in the case of an error except restarting DBus, which would probably be too much of a change here. For now, I fixed the check to only accept results larger than 0.

> @@ +443,5 @@
> > +          case DBUS_EVENT_LOOP_REMOVE:
> > +            HandleWatchRemove(mConnection);
> > +            break;
> > +          case DBUS_EVENT_LOOP_WAKEUP:
> > +            exitTask = true;
> 
> Hm. It seems to me that you could avoid the re-dispatch stuff below if you
> just call NS_ProcessNextEvent() here. That would make this logic a little
> clearer I think. If there's no event to process then the return will be
> false and you'll know that DBusWakeup was called (if that makes any
> difference... I don't understand how that currently helps us at all).

Thanks a lot! I was looking for a way to replace the re-dispatch code. Fixing this also fixes some of the comments below.

> @@ +446,5 @@
> > +          case DBUS_EVENT_LOOP_WAKEUP:
> > +            exitTask = true;
> > +            // noop
> > +            break;
> > +          default:
> 
> All of this warning stuff should be #ifdef DEBUG. And no need for a break
> statement in the default handler here.

Fixed.

> @@ +468,5 @@
> > +      {}
> > +    } while ( !(exitTask || exitThread) );
> > +
> > +    if (!exitThread) {
> > +      nsAutoPtr<nsIRunnable> pollTask(new DBusPollTask(mConnection));
> 
> This should be using an nsCOMPtr (DBusPollTask is refcounted and you're
> using an interface pointer).
> 
> However, why make a new one? Why not just re-dispatch 'this'?
> 
> @@ +478,5 @@
> > +      nsIThread *thread = NULL;
> > +      if (NS_GetCurrentThread(&thread) != NS_OK) {
> > +        return NS_OK;
> > +      }
> > +      thread->Dispatch(pollTask, NS_DISPATCH_NORMAL);
> 
> NS_DispatchToCurrentThread handles all this for you.

All this code has been removed.

> @@ +490,5 @@
> > +  DBusThread* mConnection;
> > +};
> > +
> > +static StaticAutoPtr<DBusThread> sDBusThread;
> > +static StaticRefPtr<nsIThread>   sDBusServiceThread;
> 
> Nit: In general 's' stands for 'static member'. These are more accurately
> globals, so 'g' would be better. I know the old code had it with 's', but
> since we're fixing everything might as well do this too.

Fixed.

> @@ +503,5 @@
> > +
> > +  nsAutoPtr<DBusThread> dbusThread(new DBusThread());
> > +  NS_ENSURE_TRUE(dbusThread, false);
> > +
> > +  bool eventLoopStarted = dbusThread->StartEventLoop();
> 
> The order of this (and, similarly, in StopDBus below) seems wrong.
> Conceptually an event loop requires a thread, and you're starting the event
> loop before you start the thread. I see why the functions are called in this
> order but maybe StartEventLoop is just poorly named now that you've removed
> the third thread. StartEventLoop/StopEventLoop should probably become
> something like Init and Cleanup.

Fixed.

> @@ +520,5 @@
> > +
> > +  nsRefPtr<nsIRunnable> pollTask(new DBusPollTask(dbusThread));
> > +  NS_ENSURE_TRUE(pollTask, false);
> > +
> > +  nsresult rv = sDBusServiceThread->Dispatch(pollTask, NS_DISPATCH_NORMAL);
> 
> You already have an 'nsresult rv' above, why not just declare it once?

I fixed this, but I don't like declaring variables without meaningful value. So I tend to declare and initialize them in the same statement.

> @@ +538,5 @@
> > +
> > +  if (sDBusThread) {
> > +    static const char data = DBUS_EVENT_LOOP_EXIT;
> > +    ssize_t wret = TEMP_FAILURE_RETRY(write(sDBusThread->mControlFdW.get(),
> > +                                            &data, sizeof(char)));
> 
> 'sizeof(data)' is better.

Fixed.

> @@ +564,5 @@
> > +  return true;
> > +}
> > +
> > +nsresult
> > +DispatchToDBusThread(nsIRunnable *event, uint32_t flags)
> 
> It looks like you always pass NS_DISPATCH_NORMAL as the flags here... Any
> reason not to just eliminate the second arg and always pass
> NS_DISPATCH_NORMAL in the call to Dispatch below? I don't think you'd ever
> want to allow any other flags here.

I just wanted to build a generic interface. I'd remove the extra parameter, but how about using NS_DISPATCH_NORMAL as default parameter?

...

Comment 68

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

(In reply to ben turner [:bent] from comment #65)
> Comment on attachment 729599 [details] [diff] [review]
> All-in-one patch (for review only)
> 
> Review of attachment 729599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> ::: ipc/dbus/DBusThread.h
> @@ +6,5 @@
> >  
> >  #ifndef mozilla_ipc_dbus_gonk_dbusthread_h__
> >  #define mozilla_ipc_dbus_gonk_dbusthread_h__
> >  
> > +#include "nsError.h"
> 
> You probably want "nscore.h" here. You don't specifically need any error
> code in this header, right?

Right! Fixed.

> @@ +35,5 @@
> > + * Dispatch an event to the DBus thread
> > + *
> > + * @param event An nsIRunnable to run in the DBus thread
> > + * @param flags Flags for dispatching the event
> > + * @return NS_OK on success, or an error cod eotherwise
> 
> Nit: "code otherwise"

Fixed.

> ::: ipc/dbus/DBusUtils.cpp
> @@ +22,5 @@
> > +#include "mozilla/Monitor.h"
> > +#include "nsAutoPtr.h"
> > +#include "nsThreadUtils.h"
> > +#include "DBusThread.h"
> > +#include "DBusUtils.h"
> 
> You should always (well, I guess there are a few exceptions, but not here)
> make your own header the first include line in your cpp file so that you
> know your header compiles on its own and can be included elsewhere.

Fixed.

I normally don't use include guards for code I write in my spare time, and I don't put include statements into headers. It's very old-style, I guess. From the days when compilers didn't optimize include guards. This kind of explains why I put the dependent headers last.

> @@ +52,5 @@
> >    }
> >    dbus_error_free((err));
> >  }
> >  
> > +class DBusConnectionSendRunnableBase : public nsRunnable
> 
> What is the mechanism that ensures mConnection here is always valid? Is
> there some higher-order logic that guarantees it?

These runnables run in the DBus thread, which is stopped in StopDBus(). Afterwards the connection get's closed in the same function. This should ensure that all runnables have been canceled or executed before the connection gets closed.

> @@ +55,5 @@
> >  
> > +class DBusConnectionSendRunnableBase : public nsRunnable
> > +{
> > +public:
> > +  virtual ~DBusConnectionSendRunnableBase()
> 
> In general all refcounted classes should have private or protected
> destructors to prevent people from deleting them (the destructor should
> should only run within the Release() method). Since this is meant to be a
> base class it should be protected.

Fixed.

> @@ +61,3 @@
> >  
> > +protected:
> > +  DBusConnectionSendRunnableBase(DBusConnection* aConnection,
> 
> What thread is this supposed to run on?

All runnables in this file are supposed to run on the DBus thread. I added a comment to each and an assert to the Run() methods.

> @@ +72,5 @@
> > +  DBusConnection   *mConnection;
> > +  DBusMessageRefPtr mMessage;
> > +};
> > +
> > +class DBusConnectionSendSyncRunnable : public DBusConnectionSendRunnableBase
> 
> There's a deadlock hazard here. Our condition variables do not stay
> "signaled" for any period of time.
> 
> In the normal way you use this thing you do something like this:
> 
>   nsRefPtr<SyncRunnable> t = new DerivedSyncRunnable();
>   DispatchToDBusThread(t);
>   t->WaitForCompletion();
> 
> Then, on the DBus thread, you basically have:
> 
>   // Do something blocking
>   Completed(success);
> 
> In the normal case the blocking operation on the DBus thread will take
> longer than the main thread and you'll be fine. But this ordering is also
> possible:
> 
>   1. Main thread -> DispatchToDBusThread()
>   2. Context switch
>   3. DBus thread -> // Blocking stuff
>   4. DBus thread -> Completed()
>   5. Context switch
>   6. Main thread -> WaitForCompletion()
> 
> In this case you will deadlock.
> 
> The usual pattern we use (something I forgot to cover in Half Moon Bay!) is
> like this:

It's fixed now, but TBH this comment makes me think that the Monitor sucks. What about PRConvVar? Would that be a saner interface here?

>   class DBusConnectionSendSyncRunnable
>   {
>     // ...
>     Monitor mMonitor;
>     bool mSuccess;
>     bool mCompleted;
> 
>   public:
>     bool WaitForCompletion()
>     {
>       MonitorAutoLock autoLock(mMonitor);
>       while (!mCompleted) {
>         mMonitor.Wait();
>       }
>       return mSuccess;
>     }
> 
>   protected:
>     void Completed(bool aSuccess)
>     {
>       MonitorAutoLock autoLock(mMonitor);
>       MOZ_ASSERT(!mCompleted);

MonitorAutoLock guarantees us that the other thread is blocked at this point, right? Otherwise we'd need a write barrier between the next two statements:

>       mSuccess = aSuccess;
>       mCompleted = true;

>       mMonitor.Notify();
>     }
>   };
> 
> You'll need an additional state bit like that to know if you should wait or
> not.
> 
> @@ +78,5 @@
> > +public:
> > +  virtual ~DBusConnectionSendSyncRunnable()
> > +  { }
> > +
> > +  bool WaitForCompletion()
> 
> Since the purpose of this method is to block please assert that it never
> happens on the main thread.

Fixed.

> @@ +86,5 @@
> > +    return mSuccess;
> > +  }
> > +
> > +protected:
> > +  DBusConnectionSendSyncRunnable(DBusConnection* aConnection,
> 
> What thread is this supposed to run on?

Fixed as above.

> @@ +99,5 @@
> > +  void Completed(bool aSuccess)
> > +  {
> > +    MonitorAutoLock autoLock(mCompleted);
> > +    mSuccess = aSuccess;
> > +    mCompleted.NotifyAll();
> 
> Since there's only one thread that should ever be waiting on this runnable a
> single Notify() should be sufficient.

Fixed.

> @@ +110,5 @@
> > +
> > +class DBusConnectionSendRunnable : public DBusConnectionSendSyncRunnable
> > +{
> > +public:
> > +  DBusConnectionSendRunnable(DBusConnection* aConnection,
> 
> What thread is this supposed to run on?

Fixed as above.

...

Comment 69

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

(In reply to ben turner [:bent] from comment #65)
> Comment on attachment 729599 [details] [diff] [review]

> @@ +117,5 @@
> > +  : DBusConnectionSendSyncRunnable(aConnection, aMessage),
> > +    mSerial(aSerial)
> > +  { }
> > +
> > +  NS_METHOD Run()
> 
> All these Run methods need to be NS_IMETHOD, not NS_METHOD.

Fixed. But what is the difference? NS_IMETHOD marks an impementation while NS_METHOD marks an interface? And in general, why are they important?

> And what thread is this supposed to run on?

Fixed as above.

> 
> @@ +120,5 @@
> > +
> > +  NS_METHOD Run()
> > +  {
> > +    dbus_bool_t success = dbus_connection_send(mConnection, mMessage, mSerial);
> > +    Completed(success == TRUE);
> 
> This is a little awkward. You're supposed to pass a bool to Completed() so
> this == is probably the most correct. But then whenever you call
> WaitForCompletion() (which returns this same bool) you invariably end up
> having to convert it back to TRUE/FALSE. Can you just store dbus_bool_t?

The only function where this value gets converted back is dbus_func_send. I cleaned up this function instead to make the code more friendly.

Generally speaking, I never liked how many C libraries introduce their own types for simple things like booleans and even add macros like TRUE and FALSE. C has integers for that. Hence, I don't think we should use these library types if not absolutely necessary. The public interface to DBusUtils.cpp needs an overhaul anyway; so those dbus_bool_t return values will hopefully go away soon.

> @@ +133,5 @@
> > +};
> > +
> > +dbus_bool_t
> > +dbus_func_send(DBusConnection *aConnection, dbus_uint32_t *aSerial,
> > +               DBusMessage *aMessage)
> 
> This is more of a nit, but you've got the * on the left in a bunch of places
> in this file, and then you've got them on the right in others. Either way is
> fine but you should be consistent.

I'd prefer to fix this as part of a general cleanup of the file's interface.

> @@ +137,5 @@
> > +               DBusMessage *aMessage)
> > +{
> > +  nsRefPtr<DBusConnectionSendRunnable> t(
> > +    new DBusConnectionSendRunnable(aConnection, aMessage, aSerial));
> > +  if (!t) {
> 
> We've switched over to an infallible malloc/new (by default, you can opt out
> at the call site), so this branch is dead code. There are lots of places
> where you check the result of new below so all of them need updating.

Fixed.

> @@ +147,5 @@
> > +
> > +  nsresult rv = DispatchToDBusThread(t, NS_DISPATCH_NORMAL);
> > +
> > +  if (NS_FAILED(rv)) {
> > +    if (aMessage) {
> 
> You assert in the DBusConnectionSendRunnableBase constructor that the passed
> in message is never null. If that's the case then this check seems
> unnecessary, right?

The Dispatch() method if the thread might return an error value. I think I read that this shouldn't happen in practice, but I'd rather have the code prepared for it. I now updated DispatchToDBusThread() to actually return error values from Dispatch().

> @@ +164,4 @@
> >  }
> >  
> > +static dbus_bool_t
> > +dbus_func_args_send_valist(DBusConnection *aConnection,
> 
> Looks like you can assert a lot of stuff about the arguments here. And the
> thread that this is supposed to run on?

This function never touches any of the values. It just forwards them to some other functions. I don't think we should put asserts in code that doesn't really need them. The called functions might work fine with NULL pointers in their arguments.

> @@ +178,5 @@
> > +                                                      aInterface,
> > +                                                      aFunction);
> > +  if (message == NULL) {
> > +    LOG("Could not allocate D-Bus message object!");
> > +    goto done;
> 
> In general "goto" is highly discouraged. Here it indicates the need for some
> kind of smart pointer for DBusMessage. Can we switch all this to use
> nsAutoRef? I see that someone made DBusMessageRefPtr too, maybe that would
> be better? Let's try to remove goto everywhere in this file.

This goto thing creeps in from the C programming that I'm used to. :) I agree with what you write, but I would prefer to not make that change as part of this patch set. The DBusMessageRefPtr does not work here because it takes a reference to the message in its constructor.

> @@ +197,5 @@
> > +  return FALSE;
> > +}
> > +
> > +dbus_bool_t
> > +dbus_func_args_send(DBusConnection *aConnection,
> 
> This needs lots of assertions too.

But again it doesn't touch the values.

> @@ +222,5 @@
> > +
> > +class DBusConnectionSendWithReplyRunnable : public DBusConnectionSendRunnableBase
> > +{
> > +public:
> > +  DBusConnectionSendWithReplyRunnable(DBusConnection* aConnection,
> 
> You should be able to assert a non-null callback here, right? And what
> thread is this supposed to run on?

I added some comments and asserts for clarifying the threads.

The code handles callback-functions of null. I would prefer to leave it like this for now. IIRC the original code worked this way and I didn't want to change the semantics for no reason.

> @@ +233,5 @@
> > +    mData(aData),
> > +    mTimeout(aTimeout)
> > +  { }
> > +
> > +  NS_METHOD Run()
> 
> What thread is this supposed to run on?
> 
> @@ +251,5 @@
> > +    success = dbus_pending_call_set_notify(call, Notify, data, NULL);
> > +    NS_ENSURE_TRUE(success == TRUE, NS_ERROR_FAILURE);
> > +
> > +    data.forget();
> > +    dbus_message_unref(mMessage);
> 
> Hm, mMessage is a DBusMessageRefPtr, and it automatically calls
> dbus_message_unref. Will this be a double-unref?

At this point, the message will have a refcount of 3. One from it's creation, one from DBusMessageRefPtr, and one from dbus_connection_send_with_reply. The latter ones are handled automatically, here we're releasing the ref from the message's creation.

> @@ +258,5 @@
> > +  };
> > +
> > +private:
> > +
> > +  class NotifyData
> 
> It looks like this class is kind of redundant. You could just add an extra
> reference to DBusConnectionSendWithReplyRunnable and pass 'this' to
> dbus_pending_call_set_notify, right?

After we return from the Run method, the runnable is gone, right? DBus executes the Notify method when it receives the reply to out message, which might be much later. Maybe we can use the runnable as data by modifying its ref counter, but NotifyData seemed like the clean and obvious solution. If we want to change this, can we do it in a separate patch?

> @@ +268,5 @@
> > +    { }
> > +
> > +    void RunNotifyCallback(DBusMessage *aMessage)
> > +    {
> > +      if (mCallback) {
> 
> Seems like this should always be non-null, right?

Could have been null in the old implementation.

> @@ +278,5 @@
> > +    void (*mCallback)(DBusMessage*, void*);
> > +    void *mData;
> > +  };
> > +
> > +  static void Notify(DBusPendingCall *aCall, void *aData)
> 
> What thread is this supposed to run on?

Added a comment and an assertion.

> @@ +280,5 @@
> > +  };
> > +
> > +  static void Notify(DBusPendingCall *aCall, void *aData)
> > +  {
> > +    nsAutoPtr<NotifyData> data(reinterpret_cast<NotifyData*>(aData));
> 
> static_cast when casting from void*.

Fixed.

> @@ +286,5 @@
> > +    // This is guaranteed to be non-NULL, because this function is
> > +    // called only once, when the remote method invokation returns.
> > +    DBusMessage *reply = dbus_pending_call_steal_reply(aCall);
> > +
> > +    if (reply) {
> 
> The comment above says "guaranteed to be non-NULL", so why are you checking
> here?

This comment is actually incorrect. If the timeout has been reached before a reply was received, the return value of dbus_pending_call_steal_reply() is NULL.

> @@ +381,5 @@
> >    va_end(lst);
> >    return ret;
> >  }
> >  
> > +class DBusConnectionSendAndBlockRunnable : public DBusConnectionSendSyncRunnable
> 
> So what's the difference between DBusConnectionSendRunnable and
> DBusConnectionSendAndBlockRunnable? They both inherit
> DBusConnectionSendSyncRunnable and they both block (although
> DBusConnectionSendRunnable only does so if it has a "serial"?).
> 
> In general it looks like you have three different leaf classes, and their
> main differences are:
> 
>   1. DBusConnectionSendWithReplyRunnable - lock free, callback happens on
> DBus thread?
>   2.  DBusConnectionSendRunnable - maybe blocks if a serial is passed, no
> callback
>   3. DBusConnectionSendAndBlockRunnable - always blocks, waits for reply, no
> callback
> 
> Did I get that right? If so, those major differences are difficult to
> discern based on the names of the classes...

Yes, you're right. The class names refer to similar-named DBus functions. Any class that has Base in its name is just a base class. I left a comment on each of the leaf classes to say what it does.

Comment 70

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

Attachment: [01] Cleanup DBus EventLoop function (v3)

Comment 71

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

Attachment: [02] Added DBusPollTask (v3)

Comment 72

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

Attachment: [03] Use DBusPollTask (v3)

Comment 73

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

Attachment: [04] Added dispatch function for DBus thread (v3)

Comment 74

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

Attachment: [05] Setup asyncronous DBus messages in DBus thread (v3)

Comment 75

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

Attachment: [06] Cleanup DBusConnectionSendWithReplyTask (v3)

Comment 76

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

Attachment: [07] Execute DBus send operation in DBus thread (v3)

Comment 77

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

Attachment: [08] Replace dbus_connection_send by dbus_func_send (v3)

Comment 78

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

Attachment: [09] Send blocking DBus messages from within DBus thread (v3)

Comment 79

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

Attachment: [10] Replace calls to dbus_connection_send_with_reply_and_block (v3)

Comment 80

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

Attachment: [11] Don't block DBus while waiting for replies (v3)

Comment 81

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

Attachment: All-in-one patch (v3, for review only)

Comment 82

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

Hey Kyle, Ben!

I updated the patch set according to your reviews. I tried to leave a reply for each review comment. I also updated the all-in-one patch for Ben. Please review again.

Thomas

Comment 83

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

Kyle, Ben, please review the attached patches. Thanks!

Comment 84

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

Comment on attachment 735815 [details] [diff] [review]
[01] Cleanup DBus EventLoop function (v3)

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

::: ipc/dbus/RawDBusConnection.h
@@ +39,5 @@
>  protected:
>    Scoped<ScopedDBusConnectionPtrTraits> mConnection;
> +
> +private:
> +  static Mutex mDBusIsInitLock;

Nit: static members get s prefix

Comment 85

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

Comment on attachment 735818 [details] [diff] [review]
[03] Use DBusPollTask (v3)

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

::: ipc/dbus/DBusThread.cpp
@@ +512,5 @@
> +  rv = gDBusServiceThread->Dispatch(pollTask, NS_DISPATCH_NORMAL);
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  gDBusThread = dbusThread;
> +  dbusThread.forget();

Nit: I think this can be one line: dbusThread.forget(gDBusThread)

Comment 86

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

Comment on attachment 735820 [details] [diff] [review]
[05] Setup asyncronous DBus messages in DBus thread (v3)

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

::: ipc/dbus/DBusUtils.cpp
@@ +57,5 @@
>  } dbus_async_call_t;
>  
> +void dbus_func_args_async_callback(DBusPendingCall *call, void *data)
> +{
> +  nsAutoPtr<dbus_async_call_t> req(reinterpret_cast<dbus_async_call_t*>(data));

This still needs to be a static_cast?

@@ +62,2 @@
>  
> +  // This is guaranteed to be non-NULL, because this function is

Can you fix this comment, since you mentioned dbus_pending_call_steal_reply actually can be null?

Comment 87

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

Comment on attachment 735824 [details] [diff] [review]
[08] Replace dbus_connection_send by dbus_func_send (v3)

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2310,5 @@
>      BT_WARNING("%s: Couldn't append arguments to dbus message.", __FUNCTION__);
>      errorStr.AssignLiteral("Couldn't append arguments to dbus message.");
>      result = false;
>    } else {
> +    result = dbus_func_send(mConnection, NULL, reply);

nit: nullptr instead of NULL, here and below.

Comment 88

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

Comment on attachment 735828 [details] [diff] [review]
[11] Don't block DBus while waiting for replies (v3)

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

::: ipc/dbus/DBusUtils.cpp
@@ +411,5 @@
> +private:
> +  static void Notify(DBusPendingCall *aCall, void *aData)
> +  {
> +    DBusConnectionSendAndBlockRunnable *runnable(
> +        reinterpret_cast<DBusConnectionSendAndBlockRunnable*>(aData));

static_cast

@@ +446,5 @@
>    NS_IMETHOD Run()
>    {
>      MOZ_ASSERT(!NS_IsMainThread());
>  
> +    DBusPendingCall *call = NULL;

nit: nullptr

@@ +465,3 @@
>      }
>  
> +    success = dbus_pending_call_set_notify(call, Notify, this, NULL);

nit: nullptr

Comment 89

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

Comment on attachment 735815 [details] [diff] [review]
[01] Cleanup DBus EventLoop function (v3)

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

::: ipc/dbus/DBusThread.cpp
@@ +363,5 @@
> +                                                            reinterpret_cast<DBusWatchToggledFunction>(nullptr),
> +                                                            reinterpret_cast<void*>(nullptr),
> +                                                            reinterpret_cast<DBusFreeFunction>(nullptr));
> +  if (success != TRUE) {
> +    NS_WARNING("dbus_connection_set_watch_functions failedA");

nit: A at the end can be removed.

Comment 90

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

Thanks Kyle. I updated my patches from your review. Hopefully I get Ben's review any time soon, so I can post the next version of the patch set.

Comment 91

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 735831 [details] [diff] [review]
All-in-one patch (v3, for review only)

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

I didn't finish here because I realized we're now writing to our control socket on the main thread in a potentially blocking fashion. Is there some guarantee I'm not aware of that small writes to a domain socket cannot block? We need to figure out a plan here.

::: ipc/dbus/DBusThread.cpp
@@ +293,5 @@
>  
>  }
>  
>  bool
> +DBusThread::SetUp()

You can assert that this happens on the main thread. Same for the other functions here.

@@ +316,5 @@
> +  mControlFdR.rwget() = sockets[0];
> +  mControlFdW.rwget() = sockets[1];
> +
> +  pollfd *p = mPollData.AppendElement();
> +  NS_ENSURE_TRUE(!!p, false);

This can't fail.

@@ +338,5 @@
>  
> +  dbus_bool_t success =
> +    dbus_connection_set_watch_functions(mConnection, AddWatch, RemoveWatch,
> +                                        ToggleWatch, this,
> +                                        reinterpret_cast<DBusFreeFunction>(nullptr));

You've got a lot of 'reinterpret_casts<Foo>(nullptr)' in this file, are they really needed? I think nullptr should work by itself...

@@ +339,5 @@
> +  dbus_bool_t success =
> +    dbus_connection_set_watch_functions(mConnection, AddWatch, RemoveWatch,
> +                                        ToggleWatch, this,
> +                                        reinterpret_cast<DBusFreeFunction>(nullptr));
> +  NS_ENSURE_TRUE(success == TRUE, NS_ERROR_FAILURE);

This function has a bool return.

@@ +392,5 @@
>    return true;
>  }
>  
>  bool
> +DBusThread::CleanUp()

Nit: This function should just go away, it doesn't do anything useful, and it always returns the same result.

@@ +399,5 @@
> +  return true;
> +}
> +
> +bool
> +DBusThread::WakeUp()

Isn't this doing the same thing as DBusWakeup? Can you just call that?

@@ +403,5 @@
> +DBusThread::WakeUp()
> +{
> +  static const char control = DBUS_EVENT_LOOP_WAKEUP;
> +
> +  ssize_t rv = TEMP_FAILURE_RETRY(write(mControlFdW.get(), &control, sizeof(control)));

Sigh, I'm just now realizing that we're calling write on the main thread here...

@@ +505,5 @@
>    MOZ_ASSERT(!NS_IsMainThread());
> +  NS_ENSURE_TRUE(!gDBusThread, true);
> +
> +  nsAutoPtr<DBusThread> dbusThread(new DBusThread());
> +  NS_ENSURE_TRUE(dbusThread, false);

This can't fail.

::: ipc/dbus/DBusThread.h
@@ +37,5 @@
> + * @param event An nsIRunnable to run in the DBus thread
> + * @return NS_OK on success, or an error code otherwise
> + */
> +nsresult
> +DispatchToDBusThread(nsIRunnable *event);

Nit: * on the left, 'aEvent'.

::: ipc/dbus/RawDBusConnection.cpp
@@ +17,5 @@
>  
>  RawDBusConnection::~RawDBusConnection() {
>  }
>  
> +nsresult RawDBusConnection::EstablishDBusConnection()

I don't understand why you need to do the locking here. Can this ever be called on multiple threads simultaneously?

I really think init/shutdown stuff like this should live elsewhere, maybe StartDBus?

@@ +19,5 @@
>  }
>  
> +nsresult RawDBusConnection::EstablishDBusConnection()
> +{
> +  if (!mDBusIsInit) {

Please don't use the the double-checked locking thing. It's not guaranteed to work and this code path shouldn't be that hot.

Also, the docs say it's safe to call dbus_threads_init_default() multiple times, though if we can avoid it easily I don't see why we should.

::: ipc/dbus/RawDBusConnection.h
@@ +40,5 @@
>    Scoped<ScopedDBusConnectionPtrTraits> mConnection;
> +
> +private:
> +  static Mutex mDBusIsInitLock;
> +  static bool  mDBusIsInit; // Protected by mDBusIsInitLock

Nit: We try to use 's' for static members.

Comment 92

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

> ::: ipc/dbus/DBusThread.cpp
> @@ +293,5 @@
> >  
> >  }
> >  
> >  bool
> > +DBusThread::SetUp()
> 
> You can assert that this happens on the main thread. Same for the other
> functions here.

This happens on the bluetooth thread. I added an assertion for this.

> @@ +316,5 @@
> > +  mControlFdR.rwget() = sockets[0];
> > +  mControlFdW.rwget() = sockets[1];
> > +
> > +  pollfd *p = mPollData.AppendElement();
> > +  NS_ENSURE_TRUE(!!p, false);
> 
> This can't fail.

Removed.

> 
> @@ +338,5 @@
> >  
> > +  dbus_bool_t success =
> > +    dbus_connection_set_watch_functions(mConnection, AddWatch, RemoveWatch,
> > +                                        ToggleWatch, this,
> > +                                        reinterpret_cast<DBusFreeFunction>(nullptr));
> 
> You've got a lot of 'reinterpret_casts<Foo>(nullptr)' in this file, are they
> really needed? I think nullptr should work by itself...

Fixed for everything but one case, where the cast is required.

> @@ +339,5 @@
> > +  dbus_bool_t success =
> > +    dbus_connection_set_watch_functions(mConnection, AddWatch, RemoveWatch,
> > +                                        ToggleWatch, this,
> > +                                        reinterpret_cast<DBusFreeFunction>(nullptr));
> > +  NS_ENSURE_TRUE(success == TRUE, NS_ERROR_FAILURE);
> 
> This function has a bool return.

Fixed.

> 
> @@ +392,5 @@
> >    return true;
> >  }
> >  
> >  bool
> > +DBusThread::CleanUp()
> 
> Nit: This function should just go away, it doesn't do anything useful, and
> it always returns the same result.

Fixed.

> @@ +399,5 @@
> > +  return true;
> > +}
> > +
> > +bool
> > +DBusThread::WakeUp()
> 
> Isn't this doing the same thing as DBusWakeup? Can you just call that?

DBusWakeup is just a call back for the C interface of the DBus library. I implemented it with DBusThread::WakeUp now.

> @@ +403,5 @@
> > +DBusThread::WakeUp()
> > +{
> > +  static const char control = DBUS_EVENT_LOOP_WAKEUP;
> > +
> > +  ssize_t rv = TEMP_FAILURE_RETRY(write(mControlFdW.get(), &control, sizeof(control)));
> 
> Sigh, I'm just now realizing that we're calling write on the main thread
> here...

I added a poll here to check if we can write to the socket without blocking. If not, the wakeup function just returns. If we cannot write to the file descriptor, the DBus thread will eventuelly read the data from the socket and then execute our task as well.

> @@ +505,5 @@
> >    MOZ_ASSERT(!NS_IsMainThread());
> > +  NS_ENSURE_TRUE(!gDBusThread, true);
> > +
> > +  nsAutoPtr<DBusThread> dbusThread(new DBusThread());
> > +  NS_ENSURE_TRUE(dbusThread, false);
> 
> This can't fail.

Removed.

> ::: ipc/dbus/DBusThread.h
> @@ +37,5 @@
> > + * @param event An nsIRunnable to run in the DBus thread
> > + * @return NS_OK on success, or an error code otherwise
> > + */
> > +nsresult
> > +DispatchToDBusThread(nsIRunnable *event);
> 
> Nit: * on the left, 'aEvent'.

Fixed.

> ::: ipc/dbus/RawDBusConnection.cpp
> @@ +17,5 @@
> >  
> >  RawDBusConnection::~RawDBusConnection() {
> >  }
> >  
> > +nsresult RawDBusConnection::EstablishDBusConnection()
> 
> I don't understand why you need to do the locking here. Can this ever be
> called on multiple threads simultaneously?
> 
> I really think init/shutdown stuff like this should live elsewhere, maybe
> StartDBus?

Some classes in our Bluetooth implementation inherit from RawDBusConnection and call this method. Removing the init code here will certainly cause us bug sometime. All initialization is done in the bluetooth thread, so i removed the locking from the class. I just hope this doesn't fall on our feet later on.

> @@ +19,5 @@
> >  }
> >  
> > +nsresult RawDBusConnection::EstablishDBusConnection()
> > +{
> > +  if (!mDBusIsInit) {
> 
> Please don't use the the double-checked locking thing. It's not guaranteed
> to work and this code path shouldn't be that hot.

This pattern is called test-and-(test-and-set). The outer test is just for not not grabbing a bus lock unnecessarily. How can this fail here?

> Also, the docs say it's safe to call dbus_threads_init_default() multiple
> times, though if we can avoid it easily I don't see why we should.

The function is not thread safe; the lock has taken care of this. I left in the state variable, so we don't call dbus_threads_init_default more than once.

> ::: ipc/dbus/RawDBusConnection.h
> @@ +40,5 @@
> >    Scoped<ScopedDBusConnectionPtrTraits> mConnection;
> > +
> > +private:
> > +  static Mutex mDBusIsInitLock;
> > +  static bool  mDBusIsInit; // Protected by mDBusIsInitLock
> 
> Nit: We try to use 's' for static members.

Fixed.

Thanks so far for the review. I'll attach the updated patches soon. Let me know if you see problems with the poll in the Wakeup method.

Comment 93

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

> This pattern is called test-and-(test-and-set). The outer test is just for
> not not grabbing a bus lock unnecessarily. How can this fail here?

Only one 'not' here :)

Comment 94

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'd like to see the poll stuff, yeah. I didn't r+ because I wasn't finished yet, just figured that the changes might be substantial enough that it wasn't worth continuing until they were addressed.

(In reply to Thomas Zimmermann [:tzimmermann] from comment #92)
> > This pattern is called test-and-(test-and-set). The outer test is just for
> > not not grabbing a bus lock unnecessarily. How can this fail here?

Hm, I have always known it as double-checked locking. Anyway, here's a good paper on the subject:

  http://erdani.com/publications/DDJ_Jul_Aug_2004_revised.pdf

There's also this on 'benign' data races:

  http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 95

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

Attachment: [01] Cleanup DBus EventLoop function (v4)

Comment 96

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

Attachment: [02] Added DBusPollTask (v4)

Comment 97

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

Attachment: [03] Use DBusPollTask (v4)

Comment 98

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

Attachment: [04] Added dispatch function for DBus thread (v4)

Comment 99

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

Attachment: [05] Setup asyncronous DBus messages in DBus thread (v4)

Comment 100

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

Attachment: [06] Cleanup DBusConnectionSendWithReplyTask (v4)

Comment 101

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

Attachment: [07] Execute DBus send operation in DBus thread (v4)

Comment 102

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

Attachment: [08] Replace dbus_connection_send by dbus_func_send (v4)

Comment 103

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

Attachment: [09] Send blocking DBus messages from within DBus thread (v4)

Comment 104

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

Attachment: [10] Replace calls to dbus_connection_send_with_reply_and_block (v4)

Comment 105

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

Attachment: [11] Don't block DBus while waiting for replies (v4)

Comment 106

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

Attachment: All-in-one patch (v4, for review only)

V4 should fix all the comments about v3. The write in the main loop is covered by a poll, and we only write if this doesn't block.

Kyle, I'll get back to you once I got an r+ from Ben. So you don't have to continuously review all these patches.

Comment 107

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 750496 [details] [diff] [review]
All-in-one patch (v4, for review only)

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

r=me with these addressed. Thanks for your work and patience here :)

::: ipc/dbus/DBusThread.cpp
@@ +385,5 @@
>    mWatchData.Clear();
> +}
> +
> +bool
> +DBusThread::WakeUp()

As far as I can tell you never check this return value. Should you just make it void, or should you fix the callers?

@@ +397,5 @@
> +  };
> +
> +  int nfds = TEMP_FAILURE_RETRY(poll(&fds, 1, 0));
> +  NS_ENSURE_TRUE(nfds == 1, false);
> +  NS_ENSURE_TRUE(fds.revents == POLLOUT, false);

Hm, do you want to try again later maybe? Just bailing out could mean that you never process some events if the system gets overloaded.

@@ +455,5 @@
> +              HandleWatchRemove(mConnection);
> +              // don't increment i, or we'll skip one element
> +              continue;
> +            case DBUS_EVENT_LOOP_WAKEUP:
> +              while (NS_ProcessNextEvent(currentThread, false)) {}

Rather than calling this in a loop just use NS_ProcessPendingEvents.

Comment 108

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

Thanks a lot for the review.

> 
> ::: ipc/dbus/DBusThread.cpp
> @@ +385,5 @@
> >    mWatchData.Clear();
> > +}
> > +
> > +bool
> > +DBusThread::WakeUp()
> 
> As far as I can tell you never check this return value. Should you just make
> it void, or should you fix the callers?

There is nothing we can do if this function returns false, so I just removed the return value.

> @@ +397,5 @@
> > +  };
> > +
> > +  int nfds = TEMP_FAILURE_RETRY(poll(&fds, 1, 0));
> > +  NS_ENSURE_TRUE(nfds == 1, false);
> > +  NS_ENSURE_TRUE(fds.revents == POLLOUT, false);
> 
> Hm, do you want to try again later maybe? Just bailing out could mean that
> you never process some events if the system gets overloaded.

I think we only fail here if the socket's write buffer has filled up. Our runnable has already been dispatched at this point, so the DBus thread will process it when it reads from the socket pair's read end. If writing fails for some other reason than blocking, it's certain a bug somewhere else.

> @@ +455,5 @@
> > +              HandleWatchRemove(mConnection);
> > +              // don't increment i, or we'll skip one element
> > +              continue;
> > +            case DBUS_EVENT_LOOP_WAKEUP:
> > +              while (NS_ProcessNextEvent(currentThread, false)) {}
> 
> Rather than calling this in a loop just use NS_ProcessPendingEvents.

Fixed

Comment 109

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

Attachment: [01] Cleanup DBus EventLoop function (v5)

Comment 110

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

Attachment: [02] Added DBusPollTask (v5)

Comment 111

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

Attachment: [03] Use DBusPollTask (v5)

Comment 112

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

Attachment: [04] Added dispatch function for DBus thread (v5)

Comment 113

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

Attachment: [05] Setup asyncronous DBus messages in DBus thread (v5)

Comment 114

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

Attachment: [06] Cleanup DBusConnectionSendWithReplyTask (v5)

Comment 115

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

Attachment: [07] Execute DBus send operation in DBus thread (v5)

Comment 116

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

Attachment: [08] Replace dbus_connection_send by dbus_func_send (v5)

Comment 117

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

Attachment: [09] Send blocking DBus messages from within DBus thread (v5)

Comment 118

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

Attachment: [10] Replace calls to dbus_connection_send_with_reply_and_block (v5)

Comment 119

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

Attachment: [11] Don't block DBus while waiting for replies (v5)

Comment 120

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

Kyle, here is another set of patches for review.

Comment 121

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

Attachment: [01] Cleanup DBus EventLoop function (v5)

Comment 122

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

Attachment: [02] Added DBusPollTask (v5)

Comment 123

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

Attachment: [03] Use DBusPollTask (v5)

Comment 124

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

Attachment: [04] Added dispatch function for DBus thread (v5)

Comment 125

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

Attachment: [05] Setup asyncronous DBus messages in DBus thread (v5)

Comment 126

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

Attachment: [06] Cleanup DBusConnectionSendWithReplyTask (v5)

Comment 127

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

Attachment: [07] Execute DBus send operation in DBus thread (v5)

Comment 128

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

Attachment: [08] Replace dbus_connection_send by dbus_func_send (v5)

Comment 129

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

Attachment: [09] Send blocking DBus messages from within DBus thread (v5)

Comment 130

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

Attachment: [10] Replace calls to dbus_connection_send_with_reply_and_block (v5)

Comment 131

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

Attachment: [11] Don't block DBus while waiting for replies (v5)

Comment 132

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

Patch 3 isn't applying cleanly against birch. Haven't had time to check out why exactly.

Comment 133

[Alex Keybl [:akeybl]]

Moving back to leo? - can somebody reiterate why this is desirable for v1.1 at this point?

Comment 134

[Wayne Chang [:wchang]]

Triage - leo- as unknown user impact , Thomas or Fabrice can you please elaborate on the user impact and renominate this for leo+ if you feel the need to?

Comment 135

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

Actually I don't think this patch should in leo at this point. It doesn't improve the user experience directly.

This is the fix for bug 827888, the patch there is just a workaround. It will also allow us to remove most (or maybe all) of the blocking operations from the Bluetooth system, which is beneficial when we want to have lots of concurrent BT transfers.

Comment 136

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #132)
> Patch 3 isn't applying cleanly against birch. Haven't had time to check out
> why exactly.

I just updated to the latest birch and all patches apply cleanly. Do you have other patches on top of birch?

Comment 137

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

The latest commit is

Änderung:        132841:0c7354c06f21
Marke:           qparent
Nutzer:          Chia-hung Tai <[e-mail]>
Datum:           Fri May 24 10:20:47 2013 +0800
Zusammenfassung: Bug 873145 - B2G MMS: remove redundant X-MMS-Message-Size header field from outgoing requests. r=vyang

Comment 138

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

Ok, just looks like my whitespace resolution handling was wrong. Fixed that, everything applies. Doing a test build, then will push to birch.

Comment 139

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

Comment on attachment 753139 [details] [diff] [review]
[01] Cleanup DBus EventLoop function (v5)

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

Compile errors on desktop. Once this is fixed, should be landable.

::: ipc/dbus/DBusThread.cpp
@@ +330,5 @@
> +  // Due to the fact that mPollData and mWatchData have to match, we
> +  // push a null to the front of mWatchData since it has the control
> +  // fd in the first slot of mPollData.
> +
> +  mWatchData.AppendElement(reinterpret_cast<DBusWatch*>(nullptr));

Ok, this line doesn't compile on b2g desktop for me. From the reinterpret_case documentation:

"Note that the null pointer constant nullptr or any other value of type std::nullptr_t cannot be converted to a pointer with reinterpret_cast: implicit conversion or static_cast should be used for this purpose."

So, we can just switch it out with static_cast and it at least compiles.

Comment 140

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

Attachment: [01] Cleanup DBus EventLoop function (v6)

Argghhh.... I left out this specific instance when I fixed the reinterpret_cast statements, because there was a compiler warning. Doesn't seem to happen now. I changed this to a static_cast. Let's hope the rest of the patches build without problems.

Comment 141

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

https://tbpl.mozilla.org/?tree=Try&rev=98ac9bb76248

Comment 142

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

Comment on attachment 753140 [details] [diff] [review]
[02] Added DBusPollTask (v5)

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

::: ipc/dbus/DBusThread.cpp
@@ +533,5 @@
> +              nsCString warning("unknown command ");
> +              warning.AppendInt(data);
> +              NS_WARNING(warning.get());
> +              break;
> +#endif

In what may be a first in B2G development... This was apparently only compiled in DEBUG, because without this block, production compilation fails.

Fix break position, and throw it at try (Do you have L1s? I didn't see a bug for it?). Get a clean try and I'll land. :)

Comment 143

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

Attachment: [02] Added DBusPollTask (v6)

I fixed the break statement and removed the unused variable.

Comment 144

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

https://tbpl.mozilla.org/?tree=Try&rev=d5b0db962ef1

Comment 145

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

http://hg.mozilla.org/projects/birch/rev/93044ea22c6d
http://hg.mozilla.org/projects/birch/rev/41fbfc8a1d29
http://hg.mozilla.org/projects/birch/rev/a4a9e95ef22a
http://hg.mozilla.org/projects/birch/rev/f3536ccef114
http://hg.mozilla.org/projects/birch/rev/e6d4f6af9cb7
http://hg.mozilla.org/projects/birch/rev/8f293286c7f6
http://hg.mozilla.org/projects/birch/rev/b519b2af56a0
http://hg.mozilla.org/projects/birch/rev/ca22a8f84a42
http://hg.mozilla.org/projects/birch/rev/0965dae1c283
http://hg.mozilla.org/projects/birch/rev/118a6876b202
http://hg.mozilla.org/projects/birch/rev/6e0a3783458b

Comment 146

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

OMG, I can't believe this finally happend!!! Thank you very much for taking care of this, Kyle!

*leaves for a bottle of champagne*

Comment 147

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

Yeah, wait until merged to m-c and closed before celebrating too much. :)

Comment 148

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e0a3783458b
https://hg.mozilla.org/mozilla-central/rev/118a6876b202
https://hg.mozilla.org/mozilla-central/rev/0965dae1c283
https://hg.mozilla.org/mozilla-central/rev/ca22a8f84a42
https://hg.mozilla.org/mozilla-central/rev/b519b2af56a0
https://hg.mozilla.org/mozilla-central/rev/8f293286c7f6
https://hg.mozilla.org/mozilla-central/rev/e6d4f6af9cb7
https://hg.mozilla.org/mozilla-central/rev/f3536ccef114
https://hg.mozilla.org/mozilla-central/rev/a4a9e95ef22a
https://hg.mozilla.org/mozilla-central/rev/41fbfc8a1d29
https://hg.mozilla.org/mozilla-central/rev/93044ea22c6d

Comment 149

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

Comment on attachment 755251 [details] [diff] [review]
[02] Added DBusPollTask (v6)

Forgot to mark this. :)