GNOME Bugzilla – Bug 749228
Ensure that we release the sleep inhibitor when VT switched away
Last modified: 2015-05-14 13:13:55 UTC
Since gdm switched to have its session always running I noticed that we'd always hit logind's default sleep timeout before actually suspending. The second patch here fixes that while the first one isn't strictly needed after the second is applied but I think it's a good to have sanity check. Note that this isn't specific to gdm and could happen as long as we had a gnome session running on a non-active VT.
Created attachment 303217 [details] [review] ScreenShield: ensure we don't leak logind inhibitors This could happen if we are VT switched away and an animated activation is requested because we're preparing to enter sleep. Since we don't animate in this case we'd never reach _completeLockScreenShown() before coming out of sleep, at which point we _inhibitSuspend() again and would leak the previous inhibitor.
Created attachment 303218 [details] [review] ScreenShield: only animate activations if we are the active session If we aren't the active session clutter isn't able to animate so we can't rely on the animation ending to reach _completeLockScreenShown() In fact, in that case, we'd never unhibit system sleep causing logind to reach its sleep timeout (5s by default) before actually suspending.
Review of attachment 303217 [details] [review]: ::: js/ui/screenShield.js @@ +669,3 @@ Lang.bind(this, function(inhibitor) { + if (this._inhibitor) + inhibitor.close(null); Is there a reason to do it like this instead of not calling inhibit() in the first place when we already got an inhibitor? Though I wonder if the fd isn't invalidated on suspend, in which case I'd expect code more like: if (this._inhibitor) this._inhibitor.close(null); this._inhibitor = inhibitor;
Review of attachment 303218 [details] [review]: OK
Created attachment 303330 [details] [review] ScreenShield: ensure we don't leak logind inhibitors -- (In reply to Florian Mullner from comment #3) > Is there a reason to do it like this instead of not calling > inhibit() in the first place when we already got an inhibitor? Since this is async I don't want to risk calling inhibit() more than once in a row and then leaking all but the last result. Note that I don't think this can happen currently but I'd still like to keep it safe. > Though I wonder if the fd isn't invalidated on suspend, in which > case I'd expect code more like: I don't think it is. > if (this._inhibitor) > this._inhibitor.close(null); > this._inhibitor = inhibitor; But this looks better so let's go with it.
Review of attachment 303330 [details] [review]: OK
Created attachment 303331 [details] [review] ScreenShield: tie the suspend inhibitor to our isActive property The whole point of holding a suspend inhibitor is to be able to lock before suspending. Currently, when resuming we immediately take the inhibitor without checking that we're locked which means that we won't be able to release this inhibitor if we don't unlock at least once. To prevent that and to better match the inhibitor's intention in the first place, we can tie the inhibitor with not being locked. In practice, we also want to let the locking animation finish before suspending, so we'll tie the inhibitor with not being active instead. -- I noticed that there was still a bug (described above) so I decided to re-work how we take/release the inhibitor to make it more robust.
Created attachment 303332 [details] [review] ScreenShield: only inhibit suspend if we're the active session If we aren't the active session clutter can't animate and thus we can't expect the shield to be shown before releasing the suspend inhibitor so we should release it immediately when becoming inactive. -- And this fixes the original issue that got me into this rabbit hole.
Review of attachment 303331 [details] [review]: ::: js/ui/screenShield.js @@ +675,3 @@ + _syncInhibitor: function() { + let inhibit = (!this._isActive && this._settings.get_boolean(LOCK_ENABLED_KEY)); Corner case (that I bet we didn't get right before either): _syncInhibitor() should be called when LOCK_ENABLED_KEY changes.
Review of attachment 303332 [details] [review]: OK
Created attachment 303362 [details] [review] ScreenShield: tie the suspend inhibitor to our isActive property -- (In reply to Florian Mullner from comment #9) > Corner case (that I bet we didn't get right before either): > _syncInhibitor() > should be called when LOCK_ENABLED_KEY changes. Duh, obvs. Does this look like gnome-3-16 material? At least the leak plug is but not entirely sure about these other ones.
Review of attachment 303362 [details] [review]: (In reply to Rui Matos from comment #11) > Does this look like gnome-3-16 material? Heh, I've been holding back 3.16.2 for this ;-) > At least the leak plug is but not entirely > sure about these other ones. I haven't really tested the patches (too lazy to do a scratch-build to run under a patched GDM), but they look fairly safe to me - in particular this patch is a nice cleanup, so I'm in favor of putting it in.
Ok all pushed. And yeah I did test all this both in gdm and in user sessions, that's how I found that other issue. Attachment 303330 [details] pushed as 15baa56 - ScreenShield: ensure we don't leak logind inhibitors Attachment 303332 [details] pushed as 182b1c1 - ScreenShield: only inhibit suspend if we're the active session Attachment 303362 [details] pushed as bbc8010 - ScreenShield: tie the suspend inhibitor to our isActive property