Created attachment 186920 [details] [review]
Fix logic calling UnregisterClient

Ray, since you added this "Disconnected" code, could you review the patch? Thanks.

sure

Review of attachment 186920 [details] [review]:

I have no memory of writing the patch that caused this bug initially, so I can only guess at my initial motivations.

Looking through the git log I see:

commit 6371b7af4700c2ea359ede49bfa4371195cdcc05
...
                unregister_daemon_in_session (conn);
                gkd_main_quit ();
                return DBUS_HANDLER_RESULT_HANDLED;
+       } else if (dbus_message_is_signal (msg, DBUS_INTERFACE_LOCAL, "Disconnected"))
+               unregister_daemon_in_session (conn);
+               gkd_main_quit ();
+               return DBUS_HANDLER_RESULT_HANDLED;
        }

So my guess is my logic was something like "make this clause look like the EndSession and Stop clauses above it" without fully considering that the bus isn't around anymore.  Anyway, I don't think there's a good, conscious reason the call is there so taking it out seems right.

::: daemon/dbus/gkd-dbus-session.c
@@ +99,3 @@
 		g_return_if_reached ();
 
+	dbus_connection_send (conn, msg, NULL);

should probably dbus_message_set_no_reply before sending this.

@@ -132,3 @@
 		return DBUS_HANDLER_RESULT_HANDLED;
 	} else if (dbus_message_is_signal (msg, DBUS_INTERFACE_LOCAL, "Disconnected")) {
-		unregister_daemon_in_session (conn);

unregister_daemon_in_session also frees some memory, so it may make sense to free that memory here.  On the other hand, we're in the exit path anyway so probably doesn't matter...

Thanks Ray. Added the set_no_reply() call.

The global variables are cleaned up in a callback from main() (via egg_cleanup_perform()), so they're not really a worry.

Merged with master.