1282433 - Battery API broken on linux

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

On linux no updates are received when the battery state changes.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: battery.patch

overholt: Do you know anyone about to review dbus code?

Comment 2

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

karlt? glandium?

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8765516 [details] [diff] [review]
battery.patch

Please provide a commit message.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: battery.patch

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8765760 [details] [diff] [review]
battery.patch

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

::: hal/linux/UPowerClient.cpp
@@ +235,5 @@
> +
> +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
> +                                           "org.freedesktop.UPower",
> +                                           "/org/freedesktop/UPower",
> +                                           DBUS_INTERFACE_PROPERTIES);

I already fixed the indentation here.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8765760 [details] [diff] [review]
battery.patch

> Bug 1282433 - UPowerClient should use dbus PropertiesChanged signal for retrieving battery status changes, r?=karlt

There are some things I don't understand here.
Can you explain for me, please?

Why listen for "PropertiesChanged" on /org/freedesktop/UPower
but get properties on the device path?

In dbus-monitor --system, I see messages like these

> signal time=1467196381.664556 sender=:1.11 -> destination=(null destination) serial=13464 path=/org/freedesktop/UPower/devices/battery_BAT0; interface=org.freedesktop.UPower.Device; member=Changed
> signal time=1467196381.664655 sender=:1.11 -> destination=(null destination) serial=13465 path=/org/freedesktop/UPower; interface=org.freedesktop.UPower; member=DeviceChanged
   string "/org/freedesktop/UPower/devices/battery_BAT0"

What does DeviceChanged indicate, if not changes in device properties?

Comment 7

[Andrea Marchesini [:baku]]

> What does DeviceChanged indicate, if not changes in device properties?

Can you tell me which version of UPower do you have?
This is not what happens on my laptop having ubuntu xenial and upower 0.99 and in other linux system tested by others.
Also the official documentation of upower says that we should listen for PropertiesChanged notification.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Andrea Marchesini (:baku) from comment #7)
> > What does DeviceChanged indicate, if not changes in device properties?
> 
> Can you tell me which version of UPower do you have?

I'm using 0.9.23.

> This is not what happens on my laptop having ubuntu xenial and upower 0.99
> and in other linux system tested by others.
> Also the official documentation of upower says that we should listen for
> PropertiesChanged notification.

It seems that 0.99 is using a different interface with the same name, as indicated in https://cgit.freedesktop.org/upower/commit/NEWS?id=384bae5af83fe05a91fc77c25e4d18eba9911cb1

So, listening for both DeviceChanged and PropertiesChanged does seem appropriate for now.  Please add a comment to explain why two different mechanisms are used to listen for notification of the same changes.

Comment 9

[Andrea Marchesini [:baku]]

Should I take this as a r+?

Comment 10

[Karl Tomlinson (:karlt)]

No, I haven't reviewed yet.  Still waiting for the answer to the first question in comment 6.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8765760 [details] [diff] [review]
battery.patch

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

::: hal/linux/UPowerClient.cpp
@@ +232,5 @@
>    dbus_g_proxy_connect_signal(mUPowerProxy, "DeviceChanged",
>                                G_CALLBACK (DeviceChanged), this, nullptr);
> +
> +
> +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,

This is DBUS proxy for PropertyChanged. I cannot use the other proxy because it points to a different interface/service/name.

@@ +239,5 @@
> +                                           DBUS_INTERFACE_PROPERTIES);
> +
> +  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,
> +                          dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +                          G_TYPE_STRV, G_TYPE_INVALID);

DBus works in such a way that, in order to connect a signal to a callback you must describe the 'prototype' of that signal. This signal receives a string + an hashtable + strv.
For hashtable you also need to describe what it contains. Here the reason why we have these 3 lines.
Similar to what we have for the DeviceChanged signal.

@@ +241,5 @@
> +  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,
> +                          dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +                          G_TYPE_STRV, G_TYPE_INVALID);
> +  dbus_g_proxy_connect_signal(mUPowerProxyProp, "PropertiesChanged",
> +                              G_CALLBACK (PropertiesChanged), this, nullptr);

Finally the connect :)

@@ +260,5 @@
>    dbus_g_proxy_disconnect_signal(mUPowerProxy, "DeviceChanged",
>                                   G_CALLBACK (DeviceChanged), this);
>  
> +  dbus_g_proxy_disconnect_signal(mUPowerProxyProp, "PropertiesChanged",
> +                                 G_CALLBACK (PropertiesChanged), this);

Disconnect is easy.

@@ +274,5 @@
>    g_object_unref(mUPowerProxy);
>    mUPowerProxy = nullptr;
>  
> +  g_object_unref(mUPowerProxyProp);
> +  mUPowerProxyProp = nullptr;

nullify stuff...

@@ +364,5 @@
> +                                UPowerClient* aListener)
> +{
> +  if (!aListener->mTrackedDevice) {
> +    return;
> +  }

If we don't have a TrackedDevice, the initialization is unfinished, or we are in shuttingdown.
Similar setup for DeviceChanged.

@@ +371,5 @@
> +  if (g_strcmp0(aObjectPath, "org.freedesktop.UPower")) {
> +#else
> +  if (g_ascii_strcasecmp(aObjectPath, "org.freedesktop.UPower")) {
> +#endif
> +    return;

The main difference between PropertiesChanged and DeviceChanged, is that we don't receive the name of the device in aObjectPath. Instead we receive a generic org.freedesktop.UPower. In theory we can remove this block, or have it in #ifdef DEBUG/#endif.

@@ +374,5 @@
> +#endif
> +    return;
> +  }
> +
> +  aListener->GetDevicePropertiesAsync(aListener->mTrackedDeviceProxy);

Here we continue as DeviceChanged does.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8765760 [details] [diff] [review]
battery.patch

Hmm.  I guess I was referring to the second question in comment 6.

(In reply to Andrea Marchesini (:baku) from comment #11)
> > +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
> 
> This is DBUS proxy for PropertyChanged. I cannot use the other proxy because
> it points to a different interface/service/name.

The object path here is "/org/freedesktop/UPower".  Presumably that means
the signal is sent when a property on this UPower object changes, and not
necessarily when a property changes on one of the device objects.

https://dbus.freedesktop.org/doc/dbus-specification.html

"If one or more properties change on an object, the
 org.freedesktop.DBus.Properties.PropertiesChanged signal may be emitted"

> > +  aListener->GetDevicePropertiesAsync(aListener->mTrackedDeviceProxy);
> 
> Here we continue as DeviceChanged does.

.. to get properties using a path describing a *device* object.

> If we don't have a TrackedDevice, the initialization is unfinished, or we
> are in shuttingdown.
> Similar setup for DeviceChanged.
> 
> @@ +371,5 @@
> > +  if (g_strcmp0(aObjectPath, "org.freedesktop.UPower")) {
> > +#else
> > +  if (g_ascii_strcasecmp(aObjectPath, "org.freedesktop.UPower")) {
> > +#endif
> > +    return;
> 
> The main difference between PropertiesChanged and DeviceChanged, is that we
> don't receive the name of the device in aObjectPath. Instead we receive a
> generic org.freedesktop.UPower. In theory we can remove this block, or have
> it in #ifdef DEBUG/#endif.

This check is cheap and so good, I think, to filter out changes on any other interfaces.
Note that the |interface_name| parameter is not an object path, so please name it appropriately.  Similarly, please name the other parameters consistently with the interface, or leave out the parameter names.

GLib 2.22 is required to build, so just use g_strcmp0.

A more significant difference is that DeviceChanged and PropertiesChanged are emitted on different objects.

DeviceChanged was emitted on the UPower object when the device changed.

The properties fetched are on the device object.
I assume the PropertiesChanged signal on the device object is the one of
interest?

Please also add a comment to explain in the code why two different mechanisms
are used to listen for notification of the same changes.

> Bug 1282433 - UPowerClient should use dbus PropertiesChanged signal for retrieving battery status changes, r?=karlt

Please use imperative mood in the first line of commit messages to describe
what the patch changes, rather than subjunctive describing what should be
done.

"use dbus PropertiesChanged signal for notification of battery status changes

From UPower 0.99, DeviceChanged has been removed from the UPower interface and
PropertiesChanged support added."

Comment 13

[Andrea Marchesini [:baku]]

Attachment: battery.patch

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8774228 [details] [diff] [review]
battery.patch

>+  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
>+                                               "org.freedesktop.UPower",
>+                                               "/org/freedesktop/UPower",
>+                                               DBUS_INTERFACE_PROPERTIES);
>+
>+  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,

This is the "UPower" object.  Its properties are described at
https://upower.freedesktop.org/docs/UPower.html#UPower.properties
and not useful AFAICS.

Listen to PropertiesChanged on the device object using mTrackedDeviceProxy, for changes in the properties used in UpdateSavedInfo():
https://upower.freedesktop.org/docs/Device.html#Device.properties

Comment 15

[Andrea Marchesini [:baku]]

Attachment: battery.patch

Both approaches have the same result. Here a patch with your comments applied.

Comment 16

[Andrea Marchesini [:baku]]

Karlt, news?

Comment 17

[Andrea Marchesini [:baku]]

BatteryAPI is not exposed to content anymore.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8775935 [details] [diff] [review]
battery.patch

This makes sense, thanks.

>+  static void PropertiesChanged(DBusGProxy* aProxy, const gchar* aInterfaceName,
>+                                GHashTable* aHashTable, char *word[],
>+                                UPowerClient* aListener);

Please either name the GHashTable* and char** parameters consistently
with the interface, or leave out the parameter names.

Please also comments in the code to explain why two different mechanisms
are used to listen for notification of the same changes.

>+    dbus_g_proxy_add_signal(mTrackedDeviceProxy, "PropertiesChanged", G_TYPE_STRING,
>+			    dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),

Please wrap to stay within 80 columns.

>+UPowerClient::PropertiesChanged(DBusGProxy* aProxy, const gchar* aInterfaceName,
>+                                GHashTable* aHashTable, char *aWord[],
>+                                UPowerClient* aListener)
>+{
>+  if (!aListener->mTrackedDevice) {
>+    return;
>+  }

mTrackedDevice is always set here, so remove the dead return statement.
An assert is fine if you like, but the member is not used anyway, so there's
no need.

Comment 19

[Andrea Marchesini [:baku]]

We are not removing this API.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: battery.patch

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8805930 [details] [diff] [review]
battery.patch

>+                                GHashTable*, char *aWord[],

Please remove the name of the |aWord| parameter and place the '*' beside the type.  The name is misleading because it is not a word, but a pointer to the first element of an array of names of invalidated_properties.  Either "gchar*[]" or "gchar**" is fine as they are equivalent [1].

[1] http://stackoverflow.com/questions/4810664/how-do-i-use-arrays-in-c/4810672#4810672

Comment 22

[Andrea Marchesini [:baku]]

Attachment: battery.patch

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03ccd2a9b30c
Use dbus PropertiesChanged signal for notification of battery status changes, r=karlt

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/03ccd2a9b30c