This is a pretty nasty bug. I'm just going to copy-paste the description from my test case: /* Here's a regression test for a bug where, if a name owner watch callback * removes itself, subsequent callbacks for the same change would not fire. * This was because the implementation was an array of callbacks, with an index * i, calling each in turn. So imagine we're dispatching three callbacks, * starting with 'foo': * * | foo | bar | baz | * i=0 * * If 'foo' cancels its own watch, it would be removed from the array. Then the * iteration would continue by incrementing i: * * | bar | baz | * i=1 * * and 'bar' has not been called! Gulp. The linked branch fixes this by deleting loads of code. 2085 % git diff --stat telepathy-glib-0.12 telepathy-glib/dbus-daemon.c | 193 +++++++------------- tests/dbus/dbus.c | 411 +++++++++++++++++++++++++----------------- 2 files changed, 308 insertions(+), 296 deletions(-) Aww, so close. But: 2086 % git diff --stat telepathy-glib-0.12 telepathy-glib telepathy-glib/dbus-daemon.c | 193 ++++++++++++++---------------------------- 1 files changed, 64 insertions(+), 129 deletions(-) I can live with that.
Oh, and here's a patch I ended up using while debugging. The linked branch is against 0.12; but this patch is against master. http://git.collabora.co.uk/?p=user/wjt/telepathy-glib.git;a=commitdiff;h=refs/heads/disable-sigalrm
One thing that worries me about this is that we're inventing quarks for bus names. In situations where connections randomize bus names (Idle?) or where we watch unique bus names (seeing whether something crashed), we'll end up allocating an unbounded number of quarks. I do like the idea of using signals for this, but perhaps we can't. For what it's worth, GDBus doesn't.
(In reply to comment #1) > Oh, and here's a patch I ended up using while debugging. The linked branch is > against 0.12; but this patch is against master. > http://git.collabora.co.uk/?p=user/wjt/telepathy-glib.git;a=commitdiff;h=refs/heads/disable-sigalrm Perhaps that function could return without either setting up the g_timeout or SIGALRM, if TP_TESTS_DISABLE_TIMEOUT is set? I'd be happy to merge that in master, or indeed in 0.12 since it's only test code.
(In reply to comment #3) > or indeed in 0.12 since it's only test code. it didn't exist in 0.12 :)
How about these? http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=refs/heads/timeout http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/012-noc-fixes
(In reply to comment #5) > How about these? > > http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=refs/heads/timeout ++ > http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/012-noc-fixes + for (i = 1; i <= array->len; i++) + { + _NameOwnerSubWatch *entry = &g_array_index (array, + _NameOwnerSubWatch, array->len - i); + + if (entry->callback != NULL) + continue; + + if (entry->destroy != NULL) + entry->destroy (entry->user_data); + + g_array_remove_index (array, array->len - i); + } As discussed in real life, this loop doesn't work; if you remove element 3 in the array, then element 2 will not be examined. The fix is to iterate like this: for (i = array->len; i > 0; i--) { _NameOwnerSubWatch *entry = &g_array_index (array, _NameOwnerSubWatch, i - 1); ... } The test can test this by doing something clever in its GDestroyNotify. The other loop that iterates backwards should also be changed to iterate like this. It's clearer.
(In reply to comment #6) > (In reply to comment #5) > > How about these? > > > > http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=refs/heads/timeout Merged.
Fixed in git for 0.12.7 and 0.13.10, release o'clock tomorrow probably.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.