Created attachment 253815 [details] [review]
manager: fix deadlock when registering XSMP clients at login

gnome-session will switch to the next phase whenever the list of
applications pending registration is emptied.
Previously, for XSMP clients, the code in GsmXSMPClient would signal
that an application was registered before sending the reply through
SmsRegisterClientReply().
As GnomeIdleMonitor will now call into Mutter through DBus, and the
RUNNING phase of the session will initialize its presence component that
uses the monitor, we will end up in a situation where Mutter is waiting
for the XSMP reply from gnome-session, and gnome-session is waiting for
a synchronous DBus reply from Mutter, effectively locking up the login
process until the DBus timeout occurs.

This commit fixes the bug by signalling XSMP app registration only after
the reply has been sent over the wire.

Review of attachment 253815 [details] [review]:

First: *thank you* for finding this bug!  It's been annoying me a while in gnome-ostree.  Great commit message diagnosis.  

The fix looks plausible, but the regression happened when gnome-desktop's GnomeIdleMonitor was changed to synchronously call into mutter, and I think we should also fix it there.

(Man, the relationship between gnome-session, gnome-settings-daemon, and gnome-shell is such a tangled mess...)

I'd like to see either Giovanni or Ray sign off on this patch too.

But the IdleMonitor DBus API is completely async, how is it possible to deadlock?

(In reply to comment #3)
> But the IdleMonitor DBus API is completely async, how is it possible to
> deadlock?

At least gnome_idle_monitor_get_idletime is a synchronous call.

(In reply to comment #4)
> (In reply to comment #3)
> > But the IdleMonitor DBus API is completely async, how is it possible to
> > deadlock?
> 
> At least gnome_idle_monitor_get_idletime is a synchronous call.

Right, we need to fix that instead.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > But the IdleMonitor DBus API is completely async, how is it possible to
> > > deadlock?
> > 
> > At least gnome_idle_monitor_get_idletime is a synchronous call.
> 
> Right, we need to fix that instead.

Or even better, now that I think of it, would be to change the semantics of add_idle_watch() to fire immediately if the watch is for a lower idle time than current. That way, we don't need to check the idletime before installing the watch (which is inherently racy)

(In reply to comment #2)

> First: *thank you* for finding this bug!  It's been annoying me a while in
> gnome-ostree.  Great commit message diagnosis.  

Hehe yeap this one took a while to find! 

> The fix looks plausible, but the regression happened when gnome-desktop's
> GnomeIdleMonitor was changed to synchronously call into mutter, and I think we
> should also fix it there.

I agree that the sync call in gnome-desktop should probably be fixed with a better solution, but I think this patch is actually correct in any case. The process waiting for registration (in this case Mutter) won't move to the next phase until the reply is received, and gnome-session shouldn't move to the next phase until it sent replies to all the clients waiting, especially since switching session phase will potentially execute a lot of code before we get to sending the reply.

Created attachment 253900 [details] [review]
GsmPresence: don't check the idletime after adding an idle watch

Mutter now does it internally. And this way we avoid a sync dbus
call (which can happen at bad times, such as when mutter is blocking
on us to reply XSMP or DBus)

This one is the other side of the story, and avoids gnome-session
blocking on mutter (which could still block on gnome-session for
other reasons). Depends on bug 707302.
Still I agree it makes sense to merge both.

Review of attachment 253900 [details] [review]:

makes sense to me pending the mutter change.

Comment on attachment 253815 [details] [review]
manager: fix deadlock when registering XSMP clients at login

Attachment 253815 [details] pushed as 6c24fcc - manager: fix deadlock when registering XSMP clients at login

Is there more that we want to commit here, for 3.10 ?

(In reply to comment #11)
> Is there more that we want to commit here, for 3.10 ?

I think getting the mutter fix and the gnome-session counterpart are worthwhile, it avoids a sync dbus call, which is always a good thing.

Attachment 253900 [details] pushed as 8d60892 - GsmPresence: don't check the idletime after adding an idle watch