Bug 32184 - May accidentally release the bus names of active connections
Summary: May accidentally release the bus names of active connections
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Will Thompson
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-12-07 07:56 UTC by Will Thompson
Modified: 2010-12-13 03:05 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Will Thompson 2010-12-07 07:56:57 UTC
With the fix for bug 10613, there's now an exciting bug whereby telepathy-glib may release the bus name for an active connection. I have a Gabble currently running with four open connections but only one corresponding bus name.

Here's what I think happened:

• I undocked my laptop. This got rid of the wired connection, but MC doesn't notice so the connections stayed open.
• I switched to Offline in Empathy. This made MC call Disconnect() on Gabble. THe connection released its bus name (because tp-glib does this in change_status(Disconnected)), but stayed alive waiting for some callbacks.
• I switched back to Online. A new connection was made; it claimed the same bus name.
• Finally the callbacks fired, and thus the old connection was unreffed and could be disposed. In _dispose(), TpBaseConnection drops its bus name again.

Patch to follow.
Comment 1 Will Thompson 2010-12-07 08:43:16 UTC
hello.

here is a branch! It fixes the TpBaseConnection bug, with no test case. It also fixes a few TpBaseClient and TpAccountManager nits I found while tracking this down.
Comment 2 Simon McVittie 2010-12-07 09:44:45 UTC
The actual bugfix (for the record, this regressed in 0.13.5) and the AM patch look fine, consider them r+ if you want to cherry-pick them. The AM patch could reasonably be applied to 0.12 too.

+      g_set_error (&error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+          "Connection %s doesn't seem to exist. (Maybe the CM doesn't own "
+          "the corresponding bus name?)", connection_path);
+      DEBUG ("Failed to create TpConnection: %s", error->message);

This is good in principle, but will segfault if error is NULL.
Comment 3 Will Thompson 2010-12-07 09:48:24 UTC
(In reply to comment #2)
> The actual bugfix (for the record, this regressed in 0.13.5) and the AM patch
> look fine, consider them r+ if you want to cherry-pick them. The AM patch could
> reasonably be applied to 0.12 too.
> 
> +      g_set_error (&error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
> +          "Connection %s doesn't seem to exist. (Maybe the CM doesn't own "
> +          "the corresponding bus name?)", connection_path);
> +      DEBUG ("Failed to create TpConnection: %s", error->message);
> 
> This is good in principle, but will segfault if error is NULL.

On the contrary: it won't crash *because* error is NULL. ;-) At the top of the function:

1505   GError *error = NULL;
Comment 4 Simon McVittie 2010-12-07 10:23:02 UTC
Oh, sorry, I was reading it as if error was a GError** parameter (without the &). review+, then.
Comment 5 Will Thompson 2010-12-13 03:05:48 UTC
Merged the two minor fixes to 0.12: <http://git.collabora.co.uk/?p=telepathy-glib.git;a=log;h=8ae5881>. They'll be in 0.12.7.

And merged 0.12 and the real fix to master: <http://git.collabora.co.uk/?p=telepathy-glib.git;a=shortlog;h=933247b>. So that'll be in 0.13.10.


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.