The default bug view has changed. See this FAQ.

Create message distribution mechanism for DBus Bluetooth Signals

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

Trunk
mozilla15
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

5 years ago
The DBusThread object needs to be able to distribute messages to all objects that might be waiting for them
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 625317 [details] [diff] [review]
WIP V2: DBus Signal Manager
Attachment #622241 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 625320 [details] [diff] [review]
WIP V3: DBus Signal Manager
Attachment #625317 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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.
Duplicate of this bug: 757713
(Assignee)

Comment 9

5 years ago
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.
Attachment #625320 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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.
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-
(Assignee)

Comment 12

5 years ago
Created attachment 628923 [details] [diff] [review]
v6: Creating observer model for distributing DBusMessages
Attachment #628460 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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.
Attachment #628923 - Attachment is obsolete: true
Attachment #628927 - Flags: review?(jones.chris.g)
(Assignee)

Comment 14

5 years ago
Created attachment 628958 [details] [diff] [review]
v8: Creating observer model for distributing DBusMessages

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)
(Assignee)

Comment 16

5 years ago
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.
Attachment #628958 - Attachment is obsolete: true
Attachment #629301 - Flags: review?(jones.chris.g)
(Assignee)

Comment 17

5 years ago
Created attachment 629435 [details] [diff] [review]
v10: Creating observer model for distributing DBusMessages

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+
(Assignee)

Comment 19

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=12311576&tree=Try
(Assignee)

Comment 20

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f9bf688b9ac
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/5f9bf688b9ac
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.