Closed Bug 744349 Opened 12 years ago Closed 12 years ago

Create message distribution mechanism for DBus Bluetooth Signals

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 9 obsolete files)

41.27 KB, patch
cjones
: review+
Details | Diff | Splinter Review
The DBusThread object needs to be able to distribute messages to all objects that might be waiting for them
Attached patch WIP: DBus Signal Manager (obsolete) — Splinter Review
This is getting blocked by the bluetooth manager object move, so putting a WIP patch just in case.
Attached patch WIP V2: DBus Signal Manager (obsolete) — Splinter Review
Attachment #622241 - Attachment is obsolete: true
Attached patch WIP V3: DBus Signal Manager (obsolete) — Splinter Review
Attachment #625317 - Attachment is obsolete: true
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?
Attachment #625320 - Flags: feedback?(jones.chris.g)
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.
Attachment #625320 - Flags: feedback?(jones.chris.g) → feedback+
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.
Please to be documenting.
Attached patch WIP V4: DBus Signal Manager (obsolete) — Splinter Review
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.
Attachment #625320 - Attachment is obsolete: true
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.
Attachment #627437 - Attachment is obsolete: true
Attachment #628460 - Flags: review?(jones.chris.g)
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.
Attachment #628460 - Flags: review?(jones.chris.g) → review-
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.
Attachment #628923 - Attachment is obsolete: true
Attachment #628927 - Flags: review?(jones.chris.g)
Removed a bunch of unused stuff in the patch
Attachment #628927 - Attachment is obsolete: true
Attachment #628927 - Flags: review?(jones.chris.g)
Attachment #628958 - Flags: review?(jones.chris.g)
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.
Attachment #628958 - Flags: review?(jones.chris.g)
Above concerns addressed, plus some additional commenting and reworking of the event relay system to use full variant types instead of just strings.
Attachment #628958 - Attachment is obsolete: true
Attachment #629301 - Flags: review?(jones.chris.g)
Forgot to fix DBusUtils.h formatting
Attachment #629301 - Attachment is obsolete: true
Attachment #629301 - Flags: review?(jones.chris.g)
Attachment #629435 - Flags: review?(jones.chris.g)
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
Attachment #629435 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/mozilla-central/rev/5f9bf688b9ac
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.