Review Request 109693: Try to complete locking screen before suspend

[Oliver Henshaw]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109693/#review30050
-----------------------------------------------------------


Since this happens mostly on systemd machines, I'll try returning early from the dbus lock() call (as we do now) and taking a delay lock while the lock process sets itself up. This will avoid having to take any extra measures to avoid blocking on powerdevil.

As per https://bugs.kde.org/show_bug.cgi?id=313123#c33 reverting 21ae28ccd77f9a354cf0855a44b47e1cc6ae8dec is also an option - papering over the problem by making the lock window show itself before the lock process. Then we'd have a 10 second delay after suspend with just the black lock window showing before the lockscreen and dialog finally show up.

- Oliver Henshaw


On March 25, 2013, 3:23 p.m., Oliver Henshaw wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109693/
> -----------------------------------------------------------
> 
> (Updated March 25, 2013, 3:23 p.m.)
> 
> 
> Review request for Plasma and Solid.
> 
> 
> Description
> -------
> 
> Here's an attempt to get the screen lock to fully activate before resume starts.
> 
> 
> This is a problem on systems that use systemd/logind to suspend as it suspends immediately, so that there's unlikely to be time for the lock process to hide the screen before the system suspends. There shouldn't be a problem on systems that use upower, as it waits a second before suspending - there's no guarantee that the screen locks in time, but it should in practice. But there are reports of problems with upower (on ubuntu, at least) - this needs further investigation to see if there's something else going on.
> 
> It's also not clear why the lock process waits a further ten or so seconds after resume before hiding the screen.
> 
> 
> * Delay emitting org.freedesktop.ScreenSaver.ActiveChanged and replying to org.freedesktop.ScreenSaver.Lock until the lock process is ready. We can't have the ksmserver blocking in the .Lock call as this blocks the lock process (as QApplication constructs a QSessionManager which attempts to communicate with ksmserver over XSMP) so delay replying and complete the transaction when the lock process signal its readiness.
> 
> - The qml lockscreen still blocks on powerdevil as it queries supported suspend methods using the synchronous Solid::PowerManagement API. So disable this for now. This is harmless for the plasma desktop but does affect the plasma active qml locker.
> 
> - The plasma overlay locker seems to be broken. I think this is an unrelated breakage on my test system but can't be sure. It should be fine in theory, but might have a problem if there are any power-management plasmoids on it.
> 
> 
> Other approaches to breaking the powerdevil <-> lock process deadlock are:
> 
> * Have SuspendSession::lockScreenAndWait() call face.call(QDBus::BlockWithGui, "Lock") to return to the event loop. 
> - But does this mean that the entire call-chain to this point would need to be re-entrant with respect to the event loop?
> * Make powerdevil Actions into transactions that are queued and signal their completion (or failure)
> - Tricky and fairly wide-ranging. And probably not suitable for 4.10.
> * Have the kscreenlocker_greet lock process use asynchronous dbus calls to query powerdevil rather than the synchronous API. I'm not sure if there's already some prototype asynchronous API (for libsolid2) to cannibalise for this.
> * Have ksmserver or the lock process take a delay lock -  http://www.freedesktop.org/wiki/Software/systemd/inhibit - and return from org.freedesktop.ScreenSaver.Lock as soon as the lock process is running, as we do now.
> - This would solve the problem on systems that use logind to suspend, but do nothing on systems that suspend with upower. As mentioned above, it's unclear what the cause of the problem is on upower systems.
> 
> 
> The patch series is in my clone at kde:clones/kde-workspace/oliverhenshaw/kde-workspace in the branch review/screenlocker-waitforlock and comprises:
> 
> 
> [1/5] Represent lock state with an enum
> 
> 
> [2/5] Emit activeChanged() only when lock process ready
> 
> * synchronously call desktopResized in the screen locker app
>   (this causes the lock process windows to be created)
> * signal that the lock process is ready by calling
>   org.kde.screensaver.saverLockReady()
> * introduce a Waiting LockState
> * finish locking in lockProcessReady() when receive notification over
>   dbus - start locked timer and emit locked() here rather than earlier
> 
> This is based on 62c2c398611b2e6ef6e1e38ed79bc9540fc02ec9 and the old
> screensaver implementation (from 4.9 and earlier). But delaying the
> lock() qt signal, and thus delaying the activeChanged() dbus signal,
> until the lock prcess is ready is new behaviour.
> 
> 
> [3/5] Delay reply from dbus calls until screen is locked
> 
> This prevents e.g. suspending before the screen is properly locked.
> 
> Based on the old screensaver implementation (from 4.9 and earlier).
> 
> 
> [4/5] Print when lock process starts and when ready
> 
> Useful for timing slow lock process creation
> 
> 
> [5/5] Skip blocking Solid::PowerManagement call
> 
> Solid::PowerManagement API calls powerdevil synchronously, causing
> deadlock until dbus timeout.
> 
> 
> This addresses bug 313123.
>     http://bugs.kde.org/show_bug.cgi?id=313123
> 
> 
> Diffs
> -----
> 
>   ksmserver/screenlocker/greeter/CMakeLists.txt 11905152321377aa3135424fa2e09bb991878da5 
>   ksmserver/screenlocker/greeter/greeterapp.h f332bfc5c432b5b48b568c815904a12b4a74bd7a 
>   ksmserver/screenlocker/greeter/greeterapp.cpp b70c9d6c005aa66d2f85a42ef4f1dcb04ea44667 
>   ksmserver/screenlocker/greeter/main.cpp 180d64d58b069e2d5b8362fb43c63ec2e834fadb 
>   ksmserver/screenlocker/interface.h 018464cda6c1ea9511cd4553e615bee62a66949a 
>   ksmserver/screenlocker/interface.cpp 32acad1b8afc8f15403d9e7b92dd49c51d355bdc 
>   ksmserver/screenlocker/ksldapp.h 3047c00558d664ffac864582a4cb2c4f725a3cf4 
>   ksmserver/screenlocker/ksldapp.cpp 7f2e6715f0c12baa1586e41fa8f767707c9f83c5 
>   plasma/screensaver/shell/CMakeLists.txt 5c52da691acf24422efc420be9bdc71eb716b268 
>   plasma/screensaver/shell/main.cpp be151aa25dbff449d2e777ace9b43cac3de12f6d 
> 
> Diff: http://git.reviewboard.kde.org/r/109693/diff/
> 
> 
> Testing
> -------
> 
> Not enough
> 
> 
> Thanks,
> 
> Oliver Henshaw
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130328/2c4b433f/attachment-0001.html>