telepathy-idle 0.2 unit test fixes

[detrout]

While trying to prepare a release I ran make check and a few things didn't work.

These solutions work, though there might be some better alternatives. See the commit messages for more details

[gkiagia]

That looks suspicious. I wouldn't change it so fast just to make the unit test work.

[detrout]

Right. Impatience is the route to poor misunderstood fixes.

So I did find this error in the dbus logs

h=/org/freedesktop/Telepathy/Connection/idle/irc/test_40127_2e0_2e0_2e10x558a89c
9d200; interface=org.freedesktop.Telepathy.Connection; member=Disconnect
error time=1509901413.759072 sender=org.freedesktop.DBus -> destination=:1.1 error_name=org.freedesktop.DBus.Error.ServiceUnknown reply_serial=16
   string "The name :1.2 was not provided by any .service files"

And

telepathy-idle[28715] trap int3 ip:7f567fb2a911 sp:7ffd78300f90 error:0 in libglib-2.0.so.0.5400.1[7f567fada000+111000]```

In my dmesg logs. so that is suggestive of something crashing

Though I'm not sure how to get telepathy-idle into a debugger as the printed pid is for the test dbus session

[detrout]

Dug further. from src/idle-connection.c around line 800

static void sconn_disconnected_cb(IdleServerConnection *sconn, IdleServerConnectionStateReason reason, IdleConnection *conn) {
	IdleConnectionPrivate *priv = conn->priv;
	TpConnectionStatusReason tp_reason;

	/* cancel scheduled forced disconnect since we are now disconnected */
	if (priv->force_disconnect_id) {
                g_source_remove(priv->force_disconnect_id);
		priv->force_disconnect_id = 0;
	}

The crash is when g_source_remove() is called.

The documentation for g_source_remove says "It is a programmer error to attempt to remove a non-existent source."

If I comment out the g_source_remove it doesn't crash for this test (and the test gets a dbus-return, method="Disconnect".)

But I'm having trouble figuring out how to tell if the g_source_remove call is unneeded.

Several obvious functions like g_source_is_destroyed or g_main_context_find_source_by_id all say its illegal to call if the GSource is non-existent.

The force_timeout_id is created in line 624 with the call

	/* schedule forceful disconnect for 2 seconds if the remote server doesn't
	 * respond or disconnect before then */
	priv->force_disconnect_id = g_timeout_add_seconds(2, _force_disconnect, conn);

The docs say that the timeout is automatically destroyed when it returns FALSE.

What about setting force_disconnect_id to 0 in _force_disconnect?

@@ -600,6 +600,7 @@ _force_disconnect (gpointer data)
 
        IDLE_DEBUG("gave up waiting, forcibly disconnecting");
        idle_server_connection_force_disconnect(priv->conn);
+       priv->force_disconnect_id = 0;
        return FALSE;
 }

That avoids any crashes and the all the tests pass with and the ones I changed, changed back to dbus-return, method="Disconnect".

There may be a resource leak but I'm not sure how to check?

[detrout]

Ok I force pushed a new, probably more correct fix.

The docs say the timer function is cleaned up after returning FALSE, and if I explained it correctly that means the GSource has already been destroyed. so I set the force_disconnect_id to 0 to indicate its already been cleaned up.

[gkiagia]

Looks good to me. Now the only part that I don't get is why the "ctcp: ... " commit appears again in this PR? It is already committed both in master and the 0.2 branch.

[okias]

@detrout ping?

[Kaffeine]

Looks good to me. Now the only part that I don't get is why the "ctcp: ... " commit appears again in this PR? It is already committed both in master and the 0.2 branch.

This is because we messed up the target branch. If I got this right, It should be telepathy-idle-0.2 instead of master.
If nobody argue against, I'll re-target this PR to telepathy-idle-0.2 and merge this in a few days.