Bug 32391

Summary: Correctly deal with removing name owner watches during dispatch of their callbacks
Product: Telepathy Reporter: Will Thompson <will>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: 0.12   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/012-noc-fixes
Whiteboard:
i915 platform: i915 features:

Description Will Thompson 2010-12-14 07:59:56 UTC 
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.
Comment 1 Will Thompson 2010-12-14 08:01:19 UTC 
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
Comment 2 Simon McVittie 2010-12-14 08:30:01 UTC 
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.
Comment 3 Simon McVittie 2010-12-14 08:33:31 UTC 
(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.
Comment 4 Will Thompson 2010-12-14 09:04:27 UTC 
(In reply to comment #3)
> or indeed in 0.12 since it's only test code.

it didn't exist in 0.12 :)
Comment 6 Will Thompson 2010-12-16 10:09:32 UTC 
(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.
Comment 7 Simon McVittie 2010-12-16 10:20:54 UTC 
(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.
Comment 8 Simon McVittie 2010-12-16 11:08:23 UTC 
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.