703612 - Make DBUS calls asynchronous to prevent slowness because of DBus daemon being overloaded in Battery API UPower backend

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Right now, we are using the synchronous Glib Dbus bindings which means the application will hang if DBus is overloaded. We could probably use the async calls and call into the event loop until we get a reply. It will prevent hanging the entire application because of DBus, as far as I understand it.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #0)
>  We could probably use the async
> calls and call into the event loop until we get a reply.

If DBus is used in main thread, then not probably, but must. Battery API shouldn't be enabled
in release builds if it can lock up the UI.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

Given Olli's comment, I tried to write a patch for this. It's still not working very well: for some reasons, refreshing the page might break stuff. I guess I'm running into some race conditions but I'm not really sure which ones.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 575514 [details] [diff] [review]
WIP Patch

Nested event loops cause trouble for clients (and perhaps even the implementation itself) because the callers do not usually expect random state to change during the call, and sometimes avoiding deep recursion can be difficult.

I expect it should be possible here to simply update state (and perhaps initiate another method call) when each response is received, calling hal::NotifyBatteryChange() when the necessary information has been received to indicate a state change.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 1

Unfortunately if UpdateTrackedDevice() is asynchronous, GetCurrentBatteryInformation() doesn't get right data in time.

But with this patch, at least the updates are read async.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 662514 [details] [diff] [review]
patch 1

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

r=me but I would like karlt to have a look too.

::: hal/linux/UPowerClient.cpp
@@ +36,5 @@
>  namespace hal_impl {
>  
> +class UPowerClient;
> +typedef void (*UPCDevicePropertiesCallback) (UPowerClient* aUPowerClient,
> +                                             GHashTable* aHashTable);

Why isn't that declared private in the class?

@@ +80,5 @@
>    /**
>     * Returns a hash table with the properties of aDevice.
>     * Note: the caller has to unref the hash table.
>     */
>    GHashTable* GetDeviceProperties(const gchar* aDevice);

Maybe you can rename this GetDevicePropertiesSync()?
Also, maybe you could rename UpdateTrackedDevice() to UpdateTrackedDeviceSync(). We could imagine that this method would be called via a callback later and we should make sure the caller knows it is sync.

@@ +377,5 @@
> +{
> +  GetDevicePropertiesData* data = static_cast<GetDevicePropertiesData*>(aData);
> +
> +  GError* error = nullptr;
> +  GHashTable* hashTable = nullptr;

Maybe you can do |nsAutoRef<GHashTable> = nullptr;| so you don't have to call g_hast_table_unref(hashTable).

@@ +390,5 @@
> +
> +  data->mCb(data->mClient, hashTable);
> +
> +  g_hash_table_unref(hashTable);
> +  g_object_unref(aProxy);

Is DBus going to delete |aCall| and |aData|?

@@ +403,5 @@
> +                                              aDevice,
> +                                              "org.freedesktop.DBus.Properties"));
> +
> +  GetDevicePropertiesData* data = new GetDevicePropertiesData();
> +  data->mClient =this;

nit: space before |this|.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 1b

Thanks for your review. This second patch is based on your comments.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 667906 [details] [diff] [review]
patch 1b

It looks like the UPowerClient is currently leaked.  That bug exists before
this patch, but, when that is fixed, I don't think this patch will handle a
reply received after the UPowerClient has been deleted.

The best solution I can think of it to keep the DBusGProxy for the device
properties on the UPowerClient.  That could then be created early, which
would mean that a little more of this code can be shared between async and
sync functions.  When that proxy is disposed, pending calls will be cancelled.

>+struct GetDevicePropertiesData
>+{
>+  UPowerClient* mClient;
>+  UPCDevicePropertiesCallback mCb;
>+};

This isn't needed because sInstance is available and the callback is always
DeviceChangedCallback.  DeviceChangedCallback can be merged into
GetDevicePropertiesCallback, which would help code readability because there
are a few similar callbacks here. 

(In reply to Mounir Lamouri (:mounir) from comment #5)
> > +class UPowerClient;
> > +typedef void (*UPCDevicePropertiesCallback) (UPowerClient* aUPowerClient,
> > +                                             GHashTable* aHashTable);
> 
> Why isn't that declared private in the class?

I don't think this was answered, but it won't be needed when
DeviceChangedCallback is removed.

>+  DBusGProxy* proxy(dbus_g_proxy_new_for_name(mDBusConnection,
>+                                              "org.freedesktop.UPower",
>+                                              aDevice,
>+                                              "org.freedesktop.DBus.Properties"));

dbus_g_proxy_new_from_proxy(mUPowerProxy, "org.freedesktop.DBus.Properties",
aDevice) is a little simpler.

>+  nsAutoRef<GHashTable> hashTable(nullptr);
>+  GType typeGHashTable = dbus_g_type_get_map("GHashTable", G_TYPE_STRING,
>+                                             G_TYPE_VALUE);
>+  if (!dbus_g_proxy_end_call(aProxy, aCall, &error, typeGHashTable,
>+                             &hashTable, G_TYPE_INVALID)) {

nsAutoRef<> doesn't have an operator& defined so this is a hidden nasty
reinterpret_cast<GHashTable*>(hashTable).  It might work today but nsAutoRef
is not expecting clients to access its private data this way, so it may not
work in the future.

I'd have to think about whether it may be an option to add an operator& to
nsAutoRef which would remove any existing references, or maybe a named method
would be better.  It is awkward for nsCountedRef because it doesn't
automatically add a reference.  See also bug 698003 comment 1.

Here the reference counting is not complex.  It's fine to just use GHashTable*
and g_hash_table_unref, because there is only one g_hash_table_unref required.

(If you feel like making further improvements here, we shouldn't be listening
for property changes on all devices ("DeviceChanged" on
org.freedesktop.UPower) but just changes on the device in which we are
interested ("Changed" on org.freedesktop.UPower.Device), but that might be
best in a separate patch.)

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 2

Karlt, can you review this second patch?

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 676108 [details] [diff] [review]
patch 2

> UPowerClient::~UPowerClient()
> {
>   NS_ASSERTION(!mDBusConnection && !mUPowerProxy && !mTrackedDevice,
>                "The observers have not been correctly removed! "
>                "(StopListening should have been called)");
>+
>+  // This will cancel any pending calls:
>+  while (mDeviceProxies.Length()) {
>+    g_free(mDeviceProxies[0].device);
>+    g_object_unref(mDeviceProxies[0].proxy);
>+    mDeviceProxies.RemoveElementAt(0);
>+  }

Until StopListening() actually deletes the UPowerClient, StopListening would
be the place to delete the device proxy because that is where the other
associated data is destroyed.  Only one device proxy will need to
be kept on the UPowerClient - see below.

>-    nsAutoRef<GHashTable> hashTable(GetDeviceProperties(devicePath));
>+    GHashTable* hashTable(GetDevicePropertiesSync(devicePath));
> 
>     if (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "Type"))) == sDeviceTypeBattery) {
>       UpdateSavedInfo(hashTable);
>-      mTrackedDevice = devicePath;
>+      mTrackedDevice = g_strdup(devicePath);
>+      g_hash_table_unref(hashTable);
>       break;
>     }
> 
>-    g_free(devicePath);
>+    g_hash_table_unref(hashTable);
>   }
> 
>-#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22
>-    g_ptr_array_unref(devices);
>-#else
>-    g_ptr_array_free(devices, true);
>-#endif
>+  g_ptr_array_free(devices, true);
> }

The use of nsAutoRef<GHashTable> here was correct, so please leave that.
(It didn't cast nsAutoRef<GHashTable>* to GHashTable*.)

Using the same code (g_ptr_array_free) for all builds is good, but it won't
free the pointers (devicePath) in the array.  That logic looked correct before.

>-  nsAutoRef<DBusGProxy> proxy(dbus_g_proxy_new_for_name(mDBusConnection,
>-                                                        "org.freedesktop.UPower",
>-                                                        aDevice,
>-                                                        "org.freedesktop.DBus.Properties"));
>+  DBusGProxy* proxy = GetDeviceProxy(aDevice);

We don't want to keep alive a DBusGProxy for every device on the system, but
only the DBusGProxy for the Device being watched.

Create the DBusGProxy in UpdateTrackedDevice() and pass it instead of
devicePath to GetDevicePropertiesSync.  If it is found to be the device that
is selected to watch, then save the DBusGProxy on the UPowerClient.

These parts of comment 7 weren't addressed:

[...] the callback is always
DeviceChangedCallback.  DeviceChangedCallback can be merged into
GetDevicePropertiesCallback, which would help code readability because there
are a few similar callbacks here. 

(In reply to Mounir Lamouri (:mounir) from comment #5)
> > +class UPowerClient;
> > +typedef void (*UPCDevicePropertiesCallback) (UPowerClient* aUPowerClient,
> > +                                             GHashTable* aHashTable);
> 
> Why isn't that declared private in the class?

I don't think this was answered, but it won't be needed when
DeviceChangedCallback is removed.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> Using the same code (g_ptr_array_free) for all builds is good, but it won't
> free the pointers (devicePath) in the array.  That logic looked correct
> before.

Hmm.  It wasn't correct before.
The "break" means that many of the strings are being leaked.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 3

About this patch:

1. mTrackedDevice and mTrackedDeviceProxy are created in UPowerClient::UpdateTrackedDeviceSync().

2. DeviceChangedCallback is the default callback for UPowerClient::GetDevicePropertiesCallback().

3. mTrackedDeviceProxy is unref in UPowerClient::StopListening()

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 676617 [details] [diff] [review]
patch 3

>+  g_object_unref(mTrackedDeviceProxy);
>+  mTrackedDeviceProxy = nullptr;

>+  // Reset the current tracked device proxy:
>+  g_object_unref(mTrackedDeviceProxy);
>+  mTrackedDeviceProxy = nullptr;

g_object_unref is not null-safe, so include null-checks.

>-      mTrackedDevice = devicePath;
>+      mTrackedDevice = g_strdup(devicePath);
>+      mTrackedDeviceProxy = proxy;
>       break;
>     }
> 
>-    g_free(devicePath);
>+    g_object_unref(proxy);

The g_strdup and the g_free removal make the current leak situation worse.

> UPowerClient::DeviceChanged(DBusGProxy* aProxy, const gchar* aObjectPath, UPowerClient* aListener)
> {
>+  if (!aListener->mTrackedDevice || !aListener->mTrackedDeviceProxy) {
>+    return;
>+  }

Only test mTrackedDevice because mTrackedDeviceProxy will always be set if
mTrackedDevice is set.

>+  static void DeviceChangedCallback(UPowerClient* aListener,
>+                                    GHashTable* aHashTable);

This function is no longer really a callback and is significantly different in
function to DeviceChanged() which is a callback.  As this function is just two
function calls, I would put them inline in GetDevicePropertiesCallback().
If you don't want to do that, then please use a different name for this that
covers what the function does.

Otherwise, this looks good thanks.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 4

Your comments should be completely covered by this patch. Thank you.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b55937c8352

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b55937c8352