Merge lp://staging/~pitti/lightdm/cleanup-session into lp://staging/lightdm

I uploaded this to zesty as debian/patch/, as too many people complain about the weird behaviour (duplicate settings-daemon etc.)

Oops, I overwrote you change with the 1.21 upload. Sorry. In the future please directly commit any packaging changes to lp:lightdm - these are more important to be in sync with what is in Ubuntu than blocking on a review (changes outside debian/ should still go through review).

I was under the impression that the logind PAM module would clean up the session but it appears that it only calls ReleaseSession which doesn't kill the processes like TerminateSession does. So if that's the case, then we do need to call TerminateSession at some point (for misbehaving greeter sessions).

However, we should do it after we send SIGTERM to the greeter - it should have a chance to shutdown cleanly. So I think the correct behaviour is to:

1. Send SIGTERM to the greeter
2. Wait for the greeter to stop (session_watch_cb)
3. Call TerminateSession to clean up anything the greeter missed.
4. Wait for the logind session to close (to avoid a race)

There should also be tests to confirm the TerminateSession call is made.

It's probably best to resurrect that distro patch and get that into zesty then fix it properly on this branch.

lp:lightdm had a lot of commits and a newer upstream release than zesty, that's why I didn't just upload that directly.

> it appears that it only calls ReleaseSession which doesn't kill the processes like TerminateSession does.

Correct. logind *can* kill sessions on logout (KillUserProcesses=yes in logind.conf), but we don't do that by default as this would also kill e. g. "screen".

> we should do it after we send SIGTERM to the greeter - it should have a chance to shutdown cleanly.

We could use KillSession with SIGTERM first and wait -- but I don't see why we should bother. The "clean shutdown" part should already have happened from the greeter session itself (and e. g. unity-greeter actually does partially clean itself up), so this would just introduce another round of waiting and thus increase the time for login.

> 4. Wait for the logind session to close (to avoid a race)

I don't think that's necessary -- that can happen asynchronously in the background. The point is to make sure that things like settings-daemon disappear before starting the real user session; if the session itself is still "closing" (but without processes inside any more) for a few msecs that doesn't hurt at all.

> > 4. Wait for the logind session to close (to avoid a race)
>
> I don't think that's necessary -- that can happen asynchronously in the
> background. The point is to make sure that things like settings-daemon
> disappear before starting the real user session; if the session itself is
> still "closing" (but without processes inside any more) for a few msecs that
> doesn't hurt at all.

Can we assume all processes are gone by the time TerminateSession() is called?

"Perfect is the enemy of good". Works well enough so I've committed with a regression test.

> Can we assume all processes are gone by the time TerminateSession() is called?

Not synchronously, it'll still take some time for the processes to react and shut down. But so far this seems to be "good enough" to fix the login issues.

But indeed this is a good point, this should be improved, like waiting for the session-XX.scope unit to actually disappear.

Thanks for adding a test!