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.

(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.

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 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.

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 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|.

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

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.)

Karlt, can you review this second patch?

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.

(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.

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 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.

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