This backend is using NetworkManager. I think we should probably extend it to also handle online/offline and get ride of the other NetworkManager service we have in the tree. This is currently only working with NetworkManager 0.9. I will write another patch to make it work with NetworkManager 0.8. Depending on how much work it is, I will make it work with 0.7 or just make sure it doesn't run the instance with 0.7.

Comment on attachment 583299 [details] [diff] [review] Patch v1 Given the history of incompatible changes to the NetworkManager D-Bus Interface, I assume we need to check for the Interface version and only run this code against that version, or other existing known compatible versions, perhaps with a simple change for: "A few new device states have been added, and all device states have been renumbered for flexibility." http://projects.gnome.org/NetworkManager/developers/api/09/ref-migrating.html#id428973 Some lines over 80 columns can be wrapped. Spaces around binary operators would make the code easier to read. >+void moz_ptr_array_unref(GPtrArray* ptr) { >+#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22 >+ g_ptr_array_unref(ptr); >+#else >+ g_ptr_array_free(ptr, true); >+#endif >+} I don't think it is worth having this function. g_ptr_array_free will compile and run with the intended purpose on all versions. The clients already assume they are the exclusive owners (because they release the strings). But a future user of the method may expect an unref, given the function name, but get a free. Use "TRUE" instead of "true" for gboolean parameters. I wonder whether DBusUtils.h should be called DBusGLibUtils.h. >+ * eDeviceState_Connected should have the same value as NM_DEVICE_STATE_ACTIVATED. >+ * TODO: We should include NetworkManager headers to do that cleanly. >+ * That would require to have it installed in the build bots but >+ * doesn't make it mandatory to run the build. >+ */ >+ enum State { >+ eDeviceState_Disconnected, >+ eDeviceState_Connected = 100 Mention NM_DEVICE_STATE_ACTIVATED is 100 only for NM D-Bus Interface Specification version 0.9. >+ char* mPath; >+ DBusGProxy* mProxy; >+ Type mType; >+ State mState; >+ DBusGProxy* mSpecializedProxy; >+ double mSpeed; dbus_g_proxy_get_path can be used instead of mPath, which would save some memory management. Please document the units of mSpeed. Would using nsAutoRef for mProxy and mSpecializedProxy simplify things? Consider an intelligent destructor for DeviceInfo and a copy constructor that transfers ownership. An object oriented design would have DeviceInfo listening for state and property changes. Allocating each DeviceInfo separately could simplify some things, removing the need for copying, but, if you prefer to coalesce into one allocation, the an array index could be passed through the signal handler user data and GetInstance() used when needing to UpdateNetworkInfo(). When a DBusGProxy object will be destroyed, there is no need to explicitly disconnect from the signal. Given the restriction that dbus_g_proxy_add_signal can only be called once on a proxy, it seems safe to assume that nothing else is holding references to the DBusGProxys. >+ enum Notify { >+ eNotify, >+ eDontNotify >+ }; Avoiding boolean parameters is nice. Swapping the order of eNotify and eDontNotify would mean that eNotify is true. >+ * Clear mDevices and reset mDevicesCount. Disconnect to signal and free memory. "Disconnect from signal" >+ * The DBus proxy object to upower. Needs updating for network manager. >+ /** >+ * Number of tracked devices. >+ */ >+ guint mDevicesCount; >+ >+ /** >+ * Information of all tracked devices. >+ */ >+ DeviceInfo* mDevices; Would nsTArray be helpful here? >+ sInstance = new NetworkManagerClient(); Can sInstance be deleted during shutdown? Currently it is leaked. >+ nsAutoRef<GPtrArray> devicesNM; >+ if (!dbus_g_proxy_call(mNMProxy, "GetDevices", &error, G_TYPE_INVALID, >+ typeGPtrArray, &devicesNM, G_TYPE_INVALID)) { The implicit reinterpret casts from nsAutoRef<T> to GPtrArray* and GHashTable* to access the nsAutoRefs' private member variables is nasty. The nsAutoRef is not really necessary for devicesNM, because there are no early returns. How about the approach proposed bug 698003 comment 1 for hashTable? There is no need to release in the error patch as the documentation for dbus_g_proxy_end_call says: All D-Bus method calls can fail with a remote error. If this occurs, the error will be set and this function will return FALSE. Otherwise, the "out" parameters and return value of the method are stored in the provided varargs list. >+ guint type = g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "DeviceType"))); g_value_get_uint() does not necessarily check that the GValue is of type G_TYPE_UINT, so skipping the G_VALUE_HOLDS_UINT() check is placing some faith in the stability of the interface. Given history, I'm not sure that is well placed faith, but it looks like libnm-glib makes the same assumptions, and I guess we have to only run this code against version 0.9 anyway, so I guess this is expected. I think it is still worth adding a null check for the GValue pointer, in case the property is not there. >+ // Note: Bitrate is in Kb/s according to the doc but nm-applet itself >+ // shows it in Mb/s and does /1000 so there must be something wrong >+ // somewhere and it seems better to be consistent with what nm-applet >+ // shows. I don't follow this comment. Is that about the K instead of k? The documentation says kilobits so they mean kb/s. Or is there a 1024/1000 distinction somewhere? >+ devices[i].mSpeed = g_value_get_uint(&value) / 1000.0; Use mSpeed /= instead of a second g_value_get_uint call. >+ dbus_g_proxy_add_signal(devices[i].mSpecializedProxy, "PropertiesChanged", >+ dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE), >+ G_TYPE_INVALID); dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE) need only be called once in InitializeDeviceList, though this call would be appropriate if the DeviceInfo were managing the signals. >+NetworkManagerClient::GetSpeedForDevice(const DeviceInfo& aDevice) const This contains duplicate code, but may not be necessary anyway. See below. >+ // optimized sureley. "surely" >+ if (aNewState == DeviceInfo::eDeviceState_Connected) { >+ devices[i].mState = DeviceInfo::eDeviceState_Connected; >+ } else { >+ devices[i].mState = DeviceInfo::eDeviceState_Disconnected; >+ devices[i].mSpeed = 0; >+ } mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect. Apparently this is expecting a PropertyChanged on the specialized proxy. Can we be sure to get that? It seems safe to me to not set the speed to 0, as we check mState in UpdateNetworkInfo anyway. >+ if (!g_hash_table_lookup(aProperties, "Speed")) { >+ // That means we are not connected yet but it seems that sometimes >+ // we don't get "Speed" even when connected. In that case, reading >+ // the value is working as expected. I expect the non-existence of "Speed" in the hashtable should mean that the speed didn't change, and there should be no need to get the speed again when the property hasn't changed. Is this fallout from setting speed to 0 above? >+ newSpeed = g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(aProperties, "Speed"))); Please call g_hash_table_lookup once only for each property.

Sorry for the time it took me to answer to that review. I had different stuff to do. (In reply to Karl Tomlinson (:karlt) from comment #1) > Would using nsAutoRef for mProxy and mSpecializedProxy simplify things? Not really. That would save us 2 lines in the dtor. > >+ sInstance = new NetworkManagerClient(); > > Can sInstance be deleted during shutdown? Currently it is leaked. Isn't that how all singleton are done? The memory is released during shutdown. > >+ if (aNewState == DeviceInfo::eDeviceState_Connected) { > >+ devices[i].mState = DeviceInfo::eDeviceState_Connected; > >+ } else { > >+ devices[i].mState = DeviceInfo::eDeviceState_Disconnected; > >+ devices[i].mSpeed = 0; > >+ } > > mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect. > Apparently this is expecting a PropertyChanged on the specialized proxy. > Can we be sure to get that? It seems safe to me to not set the speed to 0, > as > we check mState in UpdateNetworkInfo anyway. > > >+ if (!g_hash_table_lookup(aProperties, "Speed")) { > >+ // That means we are not connected yet but it seems that sometimes > >+ // we don't get "Speed" even when connected. In that case, reading > >+ // the value is working as expected. > > I expect the non-existence of "Speed" in the hashtable should mean that the > speed didn't change, and there should be no need to get the speed again when > the property hasn't changed. Is this fallout from setting speed to 0 above? Unfortunately not. If I plug an ethernet cable, I don't get a change event unless I use that code. Even if I don't set mSpeed to 0. All other comments have been applied in the recently attached patch.

Comment on attachment 605389 [details] [diff] [review] Patch v2 Review of attachment 605389 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMClassInfo.cpp @@ +4061,5 @@ > DOM_CLASSINFO_MAP_END > > DOM_CLASSINFO_MAP_BEGIN(MozConnection, nsIDOMMozConnection) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozConnection) > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) Forget that part of the patch. It will be handled by bug 735261.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > > Can sInstance be deleted during shutdown? Currently it is leaked. > > Isn't that how all singleton are done? The memory is released during > shutdown. Singletons are explicitly released during shutdown (at least in debug builds) before leak checks run before the program terminates.

(In reply to Karl Tomlinson (:karlt) from comment #6) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > > > Can sInstance be deleted during shutdown? Currently it is leaked. > > > > Isn't that how all singleton are done? The memory is released during > > shutdown. > > Singletons are explicitly released during shutdown (at least in debug > builds) before leak checks run before the program terminates. Isn't that for XPCOM objects? AFAIK we don't check other leaks (I might be wrong though). It's not the first singleton I write in m-c like that.

The Lk results use trace-malloc, which detects all leaks from malloc and memalign. I assume this also includes new. (valgrind does something similar but I'm not sure of the state of our valgrind tests.) See also NS_FREE_PERMANENT_DATA http://hg.mozilla.org/mozilla-central/annotate/c71845b3b2a6/xpcom/base/nscore.h#l318 If we don't try to free our data, then leak detection tools become unhelpful.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > > >+ if (aNewState == DeviceInfo::eDeviceState_Connected) { > > >+ devices[i].mState = DeviceInfo::eDeviceState_Connected; > > >+ } else { > > >+ devices[i].mState = DeviceInfo::eDeviceState_Disconnected; > > >+ devices[i].mSpeed = 0; > > >+ } > > > > mSpeed is set to 0 on disconnect, but not explicitly reinstated on connect. > > Apparently this is expecting a PropertyChanged on the specialized proxy. > > Can we be sure to get that? It seems safe to me to not set the speed to 0, > > as > > we check mState in UpdateNetworkInfo anyway. I didn't find an answer to my question about the PropertyChanged event, but I guess there will be a corresponding PropertyChanged event for the property State. Why is mSpeed set to zero here? > > > > >+ if (!g_hash_table_lookup(aProperties, "Speed")) { > > >+ // That means we are not connected yet but it seems that sometimes > > >+ // we don't get "Speed" even when connected. In that case, reading > > >+ // the value is working as expected. > > > > I expect the non-existence of "Speed" in the hashtable should mean that the > > speed didn't change, and there should be no need to get the speed again when > > the property hasn't changed. Is this fallout from setting speed to 0 above? > > Unfortunately not. If I plug an ethernet cable, I don't get a change event > unless I use that code. Even if I don't set mSpeed to 0. I see nm-device-ethernet.c has no calls to g_object_notify for NM_DEVICE_ETHERNET_SPEED. However, nm-device-wifi.c has a call for NM_DEVICE_WIFI_BITRATE, so there shouldn't be a problem there. Perhaps this is because ethernet device speeds are not expected to change. I'm not sure. On my system, this command returns 0 for my tg3 802-3-ethernet device until the cable is first plugged. Then is continues to report 100 after unplugging the cable. > dbus-send --print-reply --system --dest=org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/Devices/1 org.freedesktop.DBus.Properties.Get string:org.freedesktop.NetworkManager.Device.Wired string:Speed Getting the speed after every property change (which BTW includes org.freedesktop.NetworkManager.Device properties) seems a bit much. How about, for ethernet devices, getting the speed on StateChanged when new_state becomes NM_DEVICE_STATE_ACTIVATED?

Perhaps PropertiesChanged could even handle changes to the State property and StateChanged would not be necessary, nor mProxy, nor the VOID:UINT,UINT,UINT marshall.

I will attach a new patch including some other changes (which will all be in inter diff).

I didn't know that was possible to get changes from properties the interface was inheriting from...

That patch should include all the requested changes made after I've attached the version 2. See inter diffs for more details.

(In reply to Karl Tomlinson (:karlt) from comment #9) > Perhaps this is because ethernet device speeds are not expected to change. > I'm not sure. On my system, this command returns 0 for my tg3 802-3-ethernet > device until the cable is first plugged. Then is continues to report 100 > after unplugging the cable. > > > dbus-send --print-reply --system --dest=org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/Devices/1 org.freedesktop.DBus.Properties.Get string:org.freedesktop.NetworkManager.Device.Wired string:Speed On my system, after unplugging, this is returning 0. Anyway, the new version of the patch is reducing the number of explicit read of Speed/Bitrate and should handle both cases.

Comment on attachment 605792 [details] [diff] [review] Patch v3 >+ gpointer key, gvalue; >+ if (!g_hash_table_lookup_extended(aHashTable, aKey, &key, &gvalue)) { >+ return false; Why use the the _extended function here? Is there a reason why there should be a null GValue* in the hash table? If there is a reason for using the _extended function, then pass NULL for unused orig_key. >+#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22 >+ g_ptr_array_unref(ptr); >+#else >+ g_ptr_array_free(ptr, TRUE); >+#endif Why bother with g_ptr_array_unref here? (Comment 1) >+NetworkManagerClient::DeviceInfo::DeviceInfo() >+ : mDBusConnection(nsnull) >+ , mType(eDeviceType_Unknown) >+ , mState(eDeviceState_Disconnected) >+ , mProxy(nsnull) >+ , mSpeed(0) mType and mState don't actually need to be initialized here. >+ // There is no reason why getting the DBus connection object should fail >+ // because the NetworkManagerClient already has a connection object if we >+ // happen to be here. >+ mDBusConnection = dbus_g_bus_get(DBUS_BUS_SYSTEM, nsnull); >+ >+ // Make sure we do not exit the entire program if DBus connection get lost. >+ dbus_connection_set_exit_on_disconnect( >+ dbus_g_connection_get_connection(mDBusConnection), FALSE); Given this is a preexisting connection, there is no need to call dbus_connection_set_exit_on_disconnect here. >+ nsAutoRef<DBusGProxy> proxy(dbus_g_proxy_new_for_name( >+ mDBusConnection, >+ NM_DBUS_SERVICE, >+ dbus_g_proxy_get_path(mProxy), >+ DBUS_INTERFACE_PROPERTIES)); How about dbus_g_proxy_new_from_proxy(mProxy, DBUS_INTERFACE_PROPERTIES, NULL)? Then mDBusConnection may be unnecessary. >+ // Sometimes, the hash table has a Speed or Bitrate field with the new >+ // speed but tests shows that it's not always the case so it's better to >+ // always explicitly request the speed instead of guessing when we >+ // should do that. Isn't this only an issue with the Wired Speed? (Comment 9) There should be notification for the wireless Bitrate, right? (even if it may not be delivered at the same time as the change to State.) The get_property implementation returns priv->rate and rate is only set when notifying NM_DEVICE_WIFI_BITRATE. >+ case DeviceInfo::eDeviceType_Ethernet: { >+ guint speed; >+ // If "Speed" is not in the hash table that means the speed didn't change. >+ if (GetValueFromHashTable<guint, g_value_get_uint>(aProperties, "Speed", &speed)) { >+ newSpeed = speed; "Speed" is never in the hashtable, so the comment is not accurate here. The whole case block is unnecessary, but I guess it could be kept in case NM implements this one day. >+NetworkManagerClient::DeviceInfo::ListenForChanges() >+{ >+ dbus_g_proxy_connect_signal(mProxy, "PropertiesChanged", >+ G_CALLBACK (PropertiesChanged), this, nsnull); >+} Please add some warnings that the copy constructor must not be used after this method is called, or find a way to remove the copy constructor altogether (and replace it with a private declaration). >+ GPtrArray* devicesNM = nsnull; I'm assuming devicesNM doesn't need to be initialized here. (GError and GValue seem to be special.) But it still needs to be released. >+ nsAutoRef<DBusGProxy> deviceProxy(dbus_g_proxy_new_for_name(mDBusConnection, >+ NM_DBUS_SERVICE, >+ devicePath, >+ NM_DBUS_INTERFACE_DEVICE)); deviceProxy is not really used. >+ if (type != DeviceInfo::eDeviceType_Ethernet && >+ type != DeviceInfo::eDeviceType_Wifi) { >+ continue; >+ } >+ const gchar* interface = nsnull; >+ >+ switch (device.mType) { >+ case DeviceInfo::eDeviceType_Ethernet: >+ interface = NM_DBUS_INTERFACE_DEVICE_WIRED; >+ break; >+ case DeviceInfo::eDeviceType_Wifi: >+ interface = NM_DBUS_INTERFACE_DEVICE_WIRELESS; >+ break; >+ default: >+ MOZ_ASSERT(false); >+ } >+ >+ device.mProxy = dbus_g_proxy_new_for_name(mDBusConnection, >+ NM_DBUS_SERVICE, >+ dbus_g_proxy_get_path(deviceProxy), >+ interface); By moving the switch statement (and mProxy initialization) to the type check above, they can merge into one test and |interface| need not be null-initialized. >+ const gchar* version = g_value_get_string(&value); >+ return version[0] == '0' && version[1] == '.' && version[2] == '9'; Better to use a string comparison here. This would match against 0.95 for example. There are also (possibly valid) assumptions here that the memory is accessible even if the version string is short, but that may flag uninitialized-read warnings in valgrind. >+ if (!IsVersionSupported()) { >+ // We do not support this version, we should stop here. >+ dbus_connection_remove_filter( >+ dbus_g_connection_get_connection(mDBusConnection), >+ ConnectionSignalFilter, this); Looks like there will be problems if ConnectionSignalFilter runs before this point. Would it be better to call dbus_connection_add_filter after checking the version? Or is it necessary to add checks that the connection is still up? >+ // This should happen rarely enough to do something costly: we clear >+ // everything we know about devices and re-initialize this. That could be >+ // optimized surely. Perhaps add a comment here that mDevices can't be adjusted while the proxies have offsets into the array.