Bug 32184

Summary:	May accidentally release the bus names of active connections
Product:	Telepathy	Reporter:	Will Thompson <will>
Component:	tp-glib	Assignee:	Will Thompson <will>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/wjt/telepathy-glib.git;a=shortlog;h=refs/heads/fd.o-32184
Whiteboard:	review+
i915 platform:		i915 features:

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.