GNOME Bugzilla – Bug 691987
re-lock screen if we crashed and were restarted
Last modified: 2013-10-09 11:15:41 UTC
See https://bugs.launchpad.net/ubuntu/+source/gdm/+bug/1064584
Created attachment 233710 [details] [review] 0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch This patch needs work - the timeout is obviously broken, but I haven't yet figured out why we need it. Uploading patch so others can review.
*** Bug 692627 has been marked as a duplicate of this bug. ***
this would be nice to have for 3.8
Review of attachment 233710 [details] [review]: Code looks good to me modulo minor style issues ... not sure about the timeout thing though. ::: js/ui/main.js @@ +216,3 @@ + // suspect something is getting delayed until the workspaces + // changes, but even Meta.later_add(IDLE, ...) isn't enough. + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 250, function() { Hmm ... reading the code I can't find anything obvious why this is needed. ::: js/ui/screenShield.js @@ +964,3 @@ this._isActive = true; this._isLocked = true; + global.change_root_property(LOCKED_ATOM_STR, GLib.Variant.new('s', "locked")); We use single quotes for non translatable strings. @@ +970,3 @@ + // If the previous shell crashed, and gnome-session restarted us, then re-lock + lockIfWasLocked: function() { + let wasLocked = global.get_root_property(LOCKED_ATOM_STR, "s"); We use single quotes for non translatable strings.
Any further insight into the timeout issue ?
Created attachment 238809 [details] [review] 0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch Ok, the timeout isn't needed anymore, I think due to Jasper's reworking of the startup ordering. If we lock after the startup animation, there's a minor race condition, but it's hard to do better because when we're user-switched, the X server will block us until we're back anyways. This is at least better than nothing. Rebased to master, updated for comments.
Incidentally, I had a random idea: so we've discussed before problems like after restarting the shell, you may lose application association. Rather than abusing X window properties, it'd be cleaner to serialize our state as a file, automatically (e.g. every 5 seconds if it's changed), into /run/user/1000/gnome-shell-$DISPLAY.gvariant.
Review of attachment 238809 [details] [review]: It might make sense to split the patch between the API additions and implementing restoring the screen lock, dunno. For now I think that approach is fine, we can still reconsider using more elaborate state saving later. ::: js/ui/main.js @@ -198,3 @@ }); } - Unnecessary whitespace change ... ::: js/ui/screenShield.js @@ +31,3 @@ const LOCK_DELAY_KEY = 'lock-delay'; +const LOCKED_ATOM_STR = '_GS_LOCKED'; // Placed on X root window when locked Just wondering, why a string property that's either "locked" or null? What happened to abusing cardinals for booleans?
could we get this finished up ?
Created attachment 239478 [details] [review] updated patch with unnecessary whitespace diff removed
(In reply to comment #8) > > Just wondering, why a string property that's either "locked" or null? What > happened to abusing cardinals for booleans? Mmmm...yeah, would have been better, but I'm a bit wary of rewriting this patch now just for that and re-testing it. What if we do this for 3.8 and I'll promise to do the GVariant in /run thing for 3.9?
(In reply to comment #11) > Mmmm...yeah, would have been better, but I'm a bit wary of rewriting this patch > now just for that and re-testing it. I was really just wondering, not asking for intrusive changes at this point :-) Feel free to request the freeze break.
lets take this for 3.8.1
Review of attachment 239478 [details] [review]: thats now
Created attachment 240120 [details] [review] updated patch, now using GVariant So in the meantime, https://git.gnome.org/browse/mutter/commit/?id=edeac1de09473809debc4d0b8c416f79dde8f500 broke this patch. So I used the opportunity to do the GVariant thing, and it's cleaner anyways. But it needs re-review.
Review of attachment 240120 [details] [review]: This is one file for each state element. I don't think it is a good idea (especially if we're writing it sync every time we lock the screen) And given the comment in https://live.gnome.org/Wayland/GnomeShell, the purpose of avoiding X properties is probably moot anyway...
*** Bug 697139 has been marked as a duplicate of this bug. ***
so, where did this end ? could we land the non-intrusive earlier patch for 3.8.1 ?
(In reply to comment #16) > Review of attachment 240120 [details] [review]: > > This is one file for each state element. I don't think it is a good idea > (especially if we're writing it sync every time we lock the screen) This is to tmpfs though - it'd block if the data is paged out, but we block if other parts of the shell are paged out too... I had written it as one file originally, but then I decided, hey, why not let the filesystem be a hash table for me. > And given the comment in https://live.gnome.org/Wayland/GnomeShell, the purpose > of avoiding X properties is probably moot anyway... Still need non-X properties eventually.
can we land the non-intrusive patch for 3.8.1 ?
(In reply to comment #20) > can we land the non-intrusive patch for 3.8.1 ? Which one is non-intrusive? As I said earlier the X property patch no longer works because the mutter API was removed.
Oh, I didn't see that comment. Sadly, that probably means this is not a 3.8.1 candidate. Can we land the working patch on master, then ?
(In reply to comment #22) > Oh, I didn't see that comment. > Sadly, that probably means this is not a 3.8.1 candidate. > Can we land the working patch on master, then ? Master is 3.8.1, we'll branch after the release.
Review of attachment 240120 [details] [review]: ::: src/shell-global.c @@ +1805,3 @@ + * @global: a #ShellGlobal + * @property_name: Name of the property + * @variant: (allow-none): A #GVariant of type "s", or %NULL to unset Isn't this really a variant of arbitrary type? You're using it with type 'b' elsewhere. @@ +1825,3 @@ + g_file_replace_contents (path, g_variant_get_data (variant), size, + NULL, FALSE, G_FILE_CREATE_REPLACE_DESTINATION, + NULL, NULL, NULL); A serialized GVariant is not endian-neutral: the "structural" stuff (lengths of arrays, etc.) is always little-endian, but the payload (e.g. the integers in an array of 32-bit integers) is native-endian. The XDG_RUNTIME_DIR is defined to be machine-local anyway, but the fallback if that variable is unset is g_get_user_cache_dir() which might be shared between hosts. Perhaps just use runtime-state.:0 on little-endian systems and runtime-state-BE.:0 on big-endian systems, so LE and BE data can never collide and the issue never arises? (If I was designing a serialization format, I'd also be tempted to save values as a 'v' wrapping the real value - g_variant_new_variant(variant) in this case - so that they're somewhat self-validating on input.) @@ +1865,3 @@ + { + GBytes *bytes = g_mapped_file_get_bytes (mfile); + res = g_variant_new_from_bytes ((GVariantType*)property_type, bytes, TRUE); g_variant_new_from_bytes() doesn't really document what trusted=TRUE does, but g_variant_new_from_data() says "You should set trusted to FALSE if data is read from ... the user's home directory". The same probably applies to the user runtime directory? If res == NULL (a syntactically invalid variant), that probably deserves a g_warning() too?
(In reply to comment #24) > Review of attachment 240120 [details] [review]: > > Isn't this really a variant of arbitrary type? You're using it with type 'b' >elsewhere. Fixed. > A serialized GVariant is not endian-neutral: Yeah...for ostree i ended up deciding to always store BE, like ext4 does. But eh, let's go with your -LE and -BE idea here. > (If I was designing a serialization format, I'd also be tempted to save values > as a 'v' wrapping the real value - g_variant_new_variant(variant) in this case > - so that they're somewhat self-validating on input.) Mmm, maybe. Would require passing the format string again, which would be mildly annoying. > g_variant_new_from_bytes() doesn't really document what trusted=TRUE does, but > g_variant_new_from_data() says "You should set trusted to FALSE if data is read > from ... the user's home directory". The same probably applies to the user > runtime directory? The rationale for the home directory presumably is that you don't want to use trusted=TRUE for some blob the user downloaded from the Internet and some app dropped there. But if you have untrusted data in the runtime directory you're more likely screwed... > If res == NULL (a syntactically invalid variant), that probably deserves a > g_warning() too? That can't happen AFAICS, reading the code to current implementation of g_variant_new_from_bytes(), and nor is such a condition documented.
Created attachment 242628 [details] [review] 0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch Updated for comments
Jasper and I had some IRC discussion about this patch a while ago; he preferred X properties and felt that since in a Wayland world the compositor isn't allowed to crash, this code would just not exist. I still preferred GVariant since X properties are just a crappy API.
(In reply to comment #27) > Jasper and I had some IRC discussion about this patch a while ago; he preferred > X properties and felt that since in a Wayland world the compositor isn't > allowed to crash, this code would just not exist. That was more about window-specific properties, like desktop files.
Review of attachment 242628 [details] [review]: ::: src/shell-global.c @@ +27,3 @@ #include <meta/display.h> #include <meta/util.h> +#include <meta/errors.h> ?? @@ +1873,3 @@ + { + GBytes *bytes = g_mapped_file_get_bytes (mfile); + res = g_variant_new_from_bytes ((GVariantType*)property_type, bytes, TRUE); Should this be trusted? It's loaded from a runtime dir and could crash the shell if it's invalid.
Created attachment 244480 [details] [review] 0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch Drop unnecessary include.
(In reply to comment #29) > Should this be trusted? It's loaded from a runtime dir and could crash the > shell if it's invalid. Basically, yes; any process with the ability to corrupt the runtime directory could easily break many other things.
Review of attachment 244480 [details] [review]: OK.
Comment on attachment 244480 [details] [review] 0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch https://git.gnome.org/browse/gnome-shell/commit/?id=ccfa3d3be15b3a52bfcc37feee3abb2f2d4f66cb
*** Bug 700992 has been marked as a duplicate of this bug. ***
I assume you don't plan to do another gnome-shell 3.8 release but could you cherry-pick this to the 3-8 branch in case you end up doing another release? Thanks
It seems the patch causes the bug https://bugzilla.gnome.org/show_bug.cgi?id=662780
(In reply to comment #36) > It seems the patch causes the bug > https://bugzilla.gnome.org/show_bug.cgi?id=662780 Bug 662780 was filed more than a year before this bug, so unless bug 171940 was fixed in the meantime, this is not the case.
Yeah. You are right. I have reproduced the problem again. Sorry!