Occasional hang in unity 8 dash on the phone

[Allison Karlitskaya]

The dash is sometimes hanging on the phone.

See the attached backtrace.

The issue is as follows: we are maintaining a client-side tree of all NetworkManager devices. When we see that a new device has been added, we query its properties in order to build our local proxy. Doing this means that we are making QtDBus calls from a signal handler. Due to a bug in QtDBus this is not safe.

libdbus-1 has a recursive lock on its DBusConnection. When the signal arrives, the lock is acquired and the sginal handler is run. The signal handler can make DBus calls (which also require the lock) because the lock is recursive. QtDBus also has a lock that is always acquired before the libdbus-1 connection lock is acquired.

Unfortuntaely, the QtDBus lock is not acquired in the case of the incoming call from DBus -- only the DBus lock is.

If another thread tries to do an unrelated DBus call meanwhile, it will acquire the QtDBus lock first and then the DBus lock (which is held because of the signal that arrived). Once the signal handler gets to the point of wanting to make a DBus call (in order to query the properties of the just-added network device) it will attempt to acquire the QtDBus lock. This is a lock-inversion deadlock.

There are at least three bugs here:

1) QtDBus should be fixed in order to avoid this sort of lock inversion problem. Even if we fix this one situation, it seems quite likely that issues like this will arise again.

2) QNetworkManager should avoid the issue in QtDBus until such a time as it is fixed. This could be done by dispatching to a worker thread on receipt of signals so that the queries are done in a fresh call stack (without the libdbus-1 lock held).

3) Apparently the only reason we are using NetworkManager at all is in order to know if we are on a 3G connection or not (in order to avoid too much data charges). This would be much easier to do if NetworkManager provided a property that we could watch directly instead of bringing up an entire tree of objects in order to answer a very simple question. To that end I've filed https://bugzilla.gnome.org/show_bug.cgi?id=739080 which we can hopefully get addressed relatively quickly.

I consider 3) to be the real fix as far as we are concerned, but we should probably ensure that the Qt people know about 1) and 2). Either of those could function as a workaround for us in the meantime, but the idea that we are creating this complex object tree to answer a simple question is ridiculous, so we should really fix that directly.

[Mathieu Trudel-Lapierre (cyphermox)]

Added a network-manager task, as we need it to export a PrimaryConnectionType property which unity8 could then use...

[Allison Karlitskaya (desrt)]

The patch already went upstream in NetworkManager and now we have an Ubuntu package landing soon.

The upshot is that we now have a simple DBus property:

  system
  org.freedesktop.NetworkManager
  /org/freedesktop/NetworkManager
  org.freedesktop.NetworkManager.PrimaryConnectionType

which is a string like '802-11-wireless' or 'bluetooth'. If there is no connection then it is an empty string.

We don't have normal D-Bus property change notification here but rather a custom-rolled signal from NetworkManager itself (PropertiesChanged).

[Albert Astals Cid (aacid)]

I would like to find who is creating those dbus connections to networkmanager. There are two candidates:

 a) QNetworkConfigurationManager used in src/CachingNetworkManagerFactory.cpp
 b) NetworkingStatus.limitedBandwith used in plugins/Dash/CardCreator.js

If it is only a) and not b) we should switch src/CachingNetworkManagerFactory.cpp use over to ubuntu::connectivity::NetworkingStatus that also has the hability to provide wheter if we are online or not

Of course the original Qt bug should be still fixed so that other people don't step on that problem.

[John McAleely (john.mcaleely)]

@mathieu - I've just duped #1381500, which we saw in the dogfooding of krillin often enough not to reject as a one-off. Do its logs match the data above? I took a quick look, and couldn't find data I could directly compare. I don't know what I'm looking for though.

[Lorn Potter (lorn-potter)]

QNAM uses QtBearer, which means those are handled by the NetworkManager backend, which I am currently trying to fix up.

There are two ways of diserning online status with QtNetwork.

1) QNetworkConfigurationManager has isOnline() method, this can be done on the defaultConfiguration().

2) QNetworkConfigurationManager has allConfigurations which can take an argument of
QNetworkConfiguration::Active which will give a list of all active (online) configurations.

Both of which are a bit heavy if only being used for discovering online status.

[Albert Astals Cid (aacid)]

I opened an upstream Qt bug about the dbus issue at https://bugreports.qt-project.org/browse/QTBUG-42189

[Albert Astals Cid (aacid)]

Saviq, Gerry, do you guys think we could try using ubuntu::connectivity::NetworkingStatus in src/CachingNetworkManagerFactory.cpp ?

[Albert Astals Cid (aacid)]

Ryan, upstream has questions on https://bugreports.qt-project.org/browse/QTBUG-42189 please can you answer them there?

[Albert Astals Cid (aacid)]

Ryan do you think can try to review https://codereview.qt-project.org/#/c/98353/ https://codereview.qt-project.org/#/c/98354/ https://codereview.qt-project.org/#/c/98355/ ?

I've had a look and from my "i know nothing about dbus" they look fine.

[Allison Karlitskaya (desrt)]

I'm not sure it would be very helpful for me to review this either -- I'm very unfamiliar with the codebase and even with how locking works in libdbus-1.

[Albert Astals Cid (aacid)]

The patches have been merged upstream. Timo can you have a look at merging them in our packages?

[Launchpad Janitor (janitor)]

This bug was fixed in the package network-manager - 0.9.8.8-0ubuntu34

---------------
network-manager (0.9.8.8-0ubuntu34) vivid; urgency=medium

  * debian/tests/wpa-dhclient: unbreak the tests for isc-dhcp 4.3.5: redirect
    stderr to stdout when calling dhclient -x.
  * debian/tests/control: allow messages to stderr in wpa-dhclient test.
 -- Mathieu Trudel-Lapierre <email address hidden> Wed, 12 Nov 2014 12:08:55 -0500

[Timo Jyrinki (timo-jyrinki)]

Please give ppa:ci-train-ppa-service/landing-027 (package versions 5.3.2+dfsg-4ubuntu4 ) a try on top of vivid and report back so that I can proceed on validating/testing the landing otherwise.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qtbase-opensource-src - 5.3.2+dfsg-4ubuntu5

---------------
qtbase-opensource-src (5.3.2+dfsg-4ubuntu5) vivid; urgency=medium

  * Rename qt5-qmake-cross-armhf to qt5-qmake-arm-linux-gnueabihf
 -- Zoltan Balogh <email address hidden> Thu, 20 Nov 2014 10:33:16 +0000

[Timo Jyrinki (timo-jyrinki)]

Reopening, there was just a changelog error in the recent upload.

The DBus patches brought unity8 restarts with them, so they couldn't be included in the uploaded. It would need more work apparently than just including those three patches.

[Timo Jyrinki (timo-jyrinki)]

For additional testing, https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/landing-027/+packages is updated with a newer package 'ubuntu6' that can be again installed. Although note that another qtbase landing is also brewing which will again use the same version number and may land within 24h.

[Timo Jyrinki (timo-jyrinki)]

(tsdgeos provided a new, fourth patch, the PPA has been updated twice since)

[Launchpad Janitor (janitor)]

This bug was fixed in the package qtbase-opensource-src - 5.3.2+dfsg-4ubuntu8

---------------
qtbase-opensource-src (5.3.2+dfsg-4ubuntu8) vivid; urgency=medium

  * Pick up one more DBus fix (LP: #1384776):
    - debian/patches/Break-after-handling-the-read-write.patch

qtbase-opensource-src (5.3.2+dfsg-4ubuntu7) vivid; urgency=medium

  * Pick up upstream 5.3 branch DBus fixes (LP: #1384776)
    - debian/patches/Always-lock-the-DBus-dispatcher-before-dbus_connecti.patch
    - debian/patches/Partially-revert-Fix-a-deadlock-introduced-by-the-ra.patch
    - debian/patches/QDBusConnection-Merge-the-dispatch-and-the-watch-and.patch
 -- Timo Jyrinki <email address hidden> Thu, 27 Nov 2014 13:11:36 +0000