Last Comment Bug 744349 - Create message distribution mechanism for DBus Bluetooth Signals
: Create message distribution mechanism for DBus Bluetooth Signals
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Kyle Machulis [:kmachulis] [:qdot]
:
Mentors:
: 757713 (view as bug list)
Depends on:
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-04-11 02:38 PDT by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2012-06-03 12:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: DBus Signal Manager (12.95 KB, patch)
2012-05-08 18:16 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
WIP V2: DBus Signal Manager (16.88 KB, patch)
2012-05-18 17:39 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
WIP V3: DBus Signal Manager (16.94 KB, patch)
2012-05-18 17:43 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: feedback+
Details | Diff | Splinter Review
WIP V4: DBus Signal Manager (18.83 KB, patch)
2012-05-25 20:53 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
v5: Creating observer model for distributing DBusMessages (21.95 KB, patch)
2012-05-30 13:53 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: review-
Details | Diff | Splinter Review
v6: Creating observer model for distributing DBusMessages (39.87 KB, patch)
2012-05-31 15:32 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
v7: Creating observer model for distributing DBusMessages (37.71 KB, patch)
2012-05-31 15:38 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
v8: Creating observer model for distributing DBusMessages (35.33 KB, patch)
2012-05-31 16:29 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
v9: Creating observer model for distributing DBusMessages (41.30 KB, patch)
2012-06-01 12:50 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
v10: Creating observer model for distributing DBusMessages (41.27 KB, patch)
2012-06-01 21:24 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Kyle Machulis [:kmachulis] [:qdot] 2012-04-11 02:38:57 PDT
The DBusThread object needs to be able to distribute messages to all objects that might be waiting for them
Comment 1 Kyle Machulis [:kmachulis] [:qdot] 2012-05-08 18:16:29 PDT
Created attachment 622241 [details] [diff] [review]
WIP: DBus Signal Manager

This is getting blocked by the bluetooth manager object move, so putting a WIP patch just in case.
Comment 2 Kyle Machulis [:kmachulis] [:qdot] 2012-05-18 17:39:32 PDT
Created attachment 625317 [details] [diff] [review]
WIP V2: DBus Signal Manager
Comment 3 Kyle Machulis [:kmachulis] [:qdot] 2012-05-18 17:43:19 PDT
Created attachment 625320 [details] [diff] [review]
WIP V3: DBus Signal Manager
Comment 4 Kyle Machulis [:kmachulis] [:qdot] 2012-05-18 17:46:33 PDT
Comment on attachment 625320 [details] [diff] [review]
WIP V3: DBus Signal Manager

Mainly looking for feedback on the DBusMessageHandler/Manager setup. The plan is to change the in-constructor registration to object-level static Create() functions (so that objects are basically factories without having to worry about constructor failures). 

However, what should I have the DBusMessageManager store? I could have the Create() functions hand back RefPtrs I guess, but I'm worried about lifetime/destruction issues?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 16:19:10 PDT
Comment on attachment 625320 [details] [diff] [review]
WIP V3: DBus Signal Manager

>diff --git a/ipc/dbus/DBusThread.h b/ipc/dbus/DBusThread.h

>+// Add a message handler object to the message distribution system
>+void RegisterDBusMessageHandler(const char* aNodeName, DBusMessageHandler* aMsgHandler);
>+
>+// Remove a message handler objects from the message distribution
>+// system
>+void UnregisterDBusMessageHandler(const char* aNodeName);
>+

What's the threading model here?

The rest looks mostly ok on skim.
Comment 6 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 16:23:49 PDT
All DBusMessage handling runs on the main thread. When we get a DBusMessage in on the IOThread, a runnable is created with the DBusMessage struct and DBusSignalManager object (where the handlers are held) that's dispatched to the main thread. We assume DBusMessage handling to be a non-blocking operation.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 16:29:03 PDT
Please to be documenting.
Comment 8 Eric Chou [:ericchou] [:echou] 2012-05-25 00:58:34 PDT
*** Bug 757713 has been marked as a duplicate of this bug. ***
Comment 9 Kyle Machulis [:kmachulis] [:qdot] 2012-05-25 20:53:33 PDT
Created attachment 627437 [details] [diff] [review]
WIP V4: DBus Signal Manager

The last WIP for this iteration of the signal manager, as it's simply not going to work. What I'm basically doing here is reimplementing the ObserverManager idea in the HAL, except without the ability to keep lists of Observers. This means that if we have two applications started that use bluetooth and need the default adapter, we can't properly distribute messages to them, since this model doesn't reflect a one-to-many architecture.

This patch needs to be redone with one ObserverList/Manager per node name, managed by the DBusThread singleton object. This way we can have objects subscribe under the same name across multiple applications and distribute messages to all of them at once. How lifetime management is going to work with that is still up in the air, but I've got a meeting with Ben Turner on Tuesday morning to iron all of this out.
Comment 10 Kyle Machulis [:kmachulis] [:qdot] 2012-05-30 13:53:03 PDT
Created attachment 628460 [details] [diff] [review]
v5: Creating observer model for distributing DBusMessages

Instead of going with trying to write my own observer system, I now just use the SystemObserver service and have objects register themselves under their DBus node names. This seems to do the trick for the one-to-multi broadcasting. Since we know that notify observers will run to completion, we store the DBusMessage in a fetchable variable. The non-constantness of the fetch is something we'd have to deal with no matter what.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-30 22:54:33 PDT
Comment on attachment 628460 [details] [diff] [review]
v5: Creating observer model for distributing DBusMessages

r- for security vulnerability with DBusMessage (need to ref/unref to share between threads).

Would prefer to do fuller review on version with ObserverList<T>.  Looking forward to dbus-ery moving out of dom/bluetooth proper and into dom/bluetooth/dbus, but followup is fine.
Comment 12 Kyle Machulis [:kmachulis] [:qdot] 2012-05-31 15:32:44 PDT
Created attachment 628923 [details] [diff] [review]
v6: Creating observer model for distributing DBusMessages
Comment 13 Kyle Machulis [:kmachulis] [:qdot] 2012-05-31 15:38:55 PDT
Created attachment 628927 [details] [diff] [review]
v7: Creating observer model for distributing DBusMessages

Removed extra DOM stuff added in v6 to avoid DOM peer review on this issue.

Moved platform specifics to their own directory, removed all platform specificness from DOM code, changed observers to nsClassHashtable of ObserverList<T>'s, fixed DBusMessage ref/unref.
Comment 14 Kyle Machulis [:kmachulis] [:qdot] 2012-05-31 16:29:26 PDT
Created attachment 628958 [details] [diff] [review]
v8: Creating observer model for distributing DBusMessages

Removed a bunch of unused stuff in the patch
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-01 02:15:52 PDT
Comment on attachment 628958 [details] [diff] [review]
v8: Creating observer model for distributing DBusMessages

>diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp

>+BluetoothAdapter::~BluetoothAdapter()
>+{
>+  if(NS_FAILED(UnregisterBluetoothEventHandler(mName, this))) {

Nit: |if (|, and elsewhere.

>+BluetoothAdapter::Create(const nsCString& name) {

>+  if(NS_FAILED(RegisterBluetoothEventHandler(name, adapter.get()))) {

You shouldn't need explicit .get() here.  But if the C++ compiler
disagrees, ignore me.  Same for the uses elsewhere.

In the code here and elsewhere that has non-local Register/Unregister
(i.e. not paired by ctor/dtor), a strong invariant of the object is
that it's either registered or doesn't exist.  So make the ctor
private to ensure that only Create() is responsible for maintaining
that invariant.  And below.

>diff --git a/dom/bluetooth/BluetoothUtils.h b/dom/bluetooth/BluetoothUtils.h

>+/** 
>+ * Add a message handler object from message distribution observer.
>+ * Object must inherit nsISupportsWeakReference.
>+ *

Need to document threading model of this code, perhaps as summary
comment above this one.

>+ * @param aNodeName Node name of the object
>+ * @param aMsgHandler Weak pointer to the object
>+ *
>+ * @return NS_OK on successful addition to observer, NS_ERROR_FAILED otherwise
>+ */
>+nsresult RegisterBluetoothEventHandler(const nsCString& aNodeName, Observer<nsCString> *aMsgHandler);

Nit: please fit this on 80 columns.

Non-nit: let's pull out the Observer<T> stuff here into stronger types

  struct BluetoothMessage {
    nsCString ...;
    //...
  };

  typedef Observer<BluetoothMessage> BluetoothMessageObserver;

and use BluetoothMessageObserver instead of raw Observer<T>.

>diff --git a/dom/bluetooth/linux/BluetoothDBusUtils.cpp b/dom/bluetooth/linux/BluetoothDBusUtils.cpp

>+typedef Observer<nsCString> BTEventObserver;

Oh ... like you do here :).

>+typedef nsClassHashtable<nsCStringHashKey, ObserverList<nsCString> > BTEventObserverTable;

Let's also typedef ObserverList<T> for concision later.

>+struct DistributeDBusMessageTask : public nsRunnable {
>+
>+  DistributeDBusMessageTask(DBusMessage* aMsg) : mMsg(aMsg)
>+  {
>+  }

Let's make a Scoped<DBusMessage> (see mfbt/Scoped.h) to manage
ref/unref of these guys.  I see you use this a few more times below.
It'll make your life much easier.

>+bool 
>+StopBluetoothConnection()
>+{

>+  sBTEventObserverTable = NULL;

I don't believe this will free all the values in the table.  Please
double check.


>diff --git a/ipc/dbus/DBusUtils.cpp b/ipc/dbus/DBusUtils.cpp

>+dbus_bool_t dbus_func_args_async(                                 DBusConnection *conn,

Formatting is really hosed here.

>diff --git a/ipc/dbus/DBusUtils.h b/ipc/dbus/DBusUtils.h

>+dbus_bool_t dbus_func_args_async(
>+                                 DBusConnection *conn,

Nit: please drop the newline after '('.  And below.

>diff --git a/ipc/dbus/RawDBusConnection.cpp b/ipc/dbus/RawDBusConnection.cpp

>+void RawDBusConnection::ScopedDBusConnectionPtrTraits::release(DBusConnection* ptr) {

Brace on new line.

This is looking good.  Would like to make a quick pass over one more
version.
Comment 16 Kyle Machulis [:kmachulis] [:qdot] 2012-06-01 12:50:31 PDT
Created attachment 629301 [details] [diff] [review]
v9: Creating observer model for distributing DBusMessages

Above concerns addressed, plus some additional commenting and reworking of the event relay system to use full variant types instead of just strings.
Comment 17 Kyle Machulis [:kmachulis] [:qdot] 2012-06-01 21:24:14 PDT
Created attachment 629435 [details] [diff] [review]
v10: Creating observer model for distributing DBusMessages

Forgot to fix DBusUtils.h formatting
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-02 02:03:26 PDT
Comment on attachment 629435 [details] [diff] [review]
v10: Creating observer model for distributing DBusMessages

>diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp

>+#include "mozilla/ipc/DBusThread.h"

>+#include <dbus/dbus.h>
>+

This file has been de-D-Bus'd, right?  I don't think you need these.

>diff --git a/dom/bluetooth/BluetoothCommon.h b/dom/bluetooth/BluetoothCommon.h

>+struct BluetoothVariant
>+{
>+  uint32_t mUint32;
>+  nsCString mString;  

We can make a proper space-efficient ~type-safe discrimated union for
these using the IPDL compiler, but it's overkill for now.  Can revisit
when we start IPC-ifying.

>diff --git a/dom/bluetooth/BluetoothManager.cpp b/dom/bluetooth/BluetoothManag

>+#include "mozilla/ipc/DBusThread.h"
>+#include <dbus/dbus.h>

Don't think we need these either.

>+  mozilla::DebugOnly<nsresult> rv =

We're in namespace mozilla, so drop the mozilla:: qualification.

>diff --git a/ipc/dbus/DBusUtils.h b/ipc/dbus/DBusUtils.h

>+class ScopedDBusMessage

Hmm ... for what you're doing here, you need to take a ref, not just
releasing an existing resource, so I gave you bad advice: you want a
smart pointer.  I think this might be a bit simpler and easier to use

class DBusMessageRefPtr
{
public:
  ScopedDBusMessage(DBusMessage* aMsg) : mMsg(aMsg)
  {
    if (mMsg) dbus_message_ref(mMsg);
  }
  ~ScopedDBusMessage()
  {
    if (mMsg) dbus_message_unref(mMsg);
  }
  operator DBusMessage*() { return mMsg; }
  DBusMessage* get() { return mMsg; }
private:
  DBusMessage* mMsg;
};

r=me with that
Comment 19 Kyle Machulis [:kmachulis] [:qdot] 2012-06-02 11:19:58 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=12311576&tree=Try
Comment 20 Kyle Machulis [:kmachulis] [:qdot] 2012-06-02 11:22:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f9bf688b9ac
Comment 21 Phil Ringnalda (:philor) 2012-06-03 12:20:03 PDT
https://hg.mozilla.org/mozilla-central/rev/5f9bf688b9ac

Note You need to log in before you can comment on or make changes to this bug.