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)
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
Assignee | ||
Comment 1•12 years ago
|
||
This is getting blocked by the bluetooth manager object move, so putting a WIP patch just in case.
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #622241 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #625317 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 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•12 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.
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #628460 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
https://tbpl.mozilla.org/php/getParsedLog.php?id=12311576&tree=Try
Assignee | ||
Comment 20•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f9bf688b9ac
Target Milestone: --- → mozilla15
Comment 21•12 years ago
|
||
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.
Description
•