After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691987 - re-lock screen if we crashed and were restarted
re-lock screen if we crashed and were restarted
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 692627 697139 700992 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-01-18 01:08 UTC by Colin Walters
Modified: 2013-10-09 11:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch (7.43 KB, patch)
2013-01-18 01:09 UTC, Colin Walters
reviewed Details | Review
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch (7.25 KB, patch)
2013-03-13 17:49 UTC, Colin Walters
reviewed Details | Review
updated patch with unnecessary whitespace diff removed (7.13 KB, patch)
2013-03-21 16:37 UTC, Colin Walters
accepted-commit_now Details | Review
updated patch, now using GVariant (7.55 KB, patch)
2013-03-29 14:12 UTC, Colin Walters
reviewed Details | Review
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch (7.72 KB, patch)
2013-04-26 21:38 UTC, Colin Walters
reviewed Details | Review
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch (7.59 KB, patch)
2013-05-17 00:10 UTC, Colin Walters
committed Details | Review

Comment 1 Colin Walters 2013-01-18 01:09:37 UTC
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.
Comment 2 Giovanni Campagna 2013-01-27 11:20:40 UTC
*** Bug 692627 has been marked as a duplicate of this bug. ***
Comment 3 Matthias Clasen 2013-01-28 11:53:23 UTC
this would be nice to have for 3.8
Comment 4 drago01 2013-02-10 16:07:29 UTC
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.
Comment 5 Matthias Clasen 2013-03-04 12:59:51 UTC
Any further insight into the timeout issue ?
Comment 6 Colin Walters 2013-03-13 17:49:08 UTC
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.
Comment 7 Colin Walters 2013-03-13 17:54:24 UTC
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.
Comment 8 Florian Müllner 2013-03-15 16:26:52 UTC
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?
Comment 9 Matthias Clasen 2013-03-21 11:45:48 UTC
could we get this finished up ?
Comment 10 Colin Walters 2013-03-21 16:37:28 UTC
Created attachment 239478 [details] [review]
updated patch with unnecessary whitespace diff removed
Comment 11 Colin Walters 2013-03-21 16:39:15 UTC
(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?
Comment 12 Florian Müllner 2013-03-21 17:57:31 UTC
(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.
Comment 13 Matthias Clasen 2013-03-22 12:31:23 UTC
lets take this for 3.8.1
Comment 14 Matthias Clasen 2013-03-28 02:23:47 UTC
Review of attachment 239478 [details] [review]:

thats now
Comment 15 Colin Walters 2013-03-29 14:12:32 UTC
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.
Comment 16 Giovanni Campagna 2013-03-29 14:29:57 UTC
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...
Comment 17 Rui Matos 2013-04-02 19:48:16 UTC
*** Bug 697139 has been marked as a duplicate of this bug. ***
Comment 18 Matthias Clasen 2013-04-03 10:36:30 UTC
so, where did this end ? could we land the non-intrusive earlier patch for 3.8.1 ?
Comment 19 Colin Walters 2013-04-03 11:05:12 UTC
(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.
Comment 20 Matthias Clasen 2013-04-10 10:30:18 UTC
can we land the non-intrusive patch for 3.8.1 ?
Comment 21 Colin Walters 2013-04-10 10:58:39 UTC
(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.
Comment 22 Matthias Clasen 2013-04-10 12:47:19 UTC
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 ?
Comment 23 Florian Müllner 2013-04-10 18:54:46 UTC
(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.
Comment 24 Simon McVittie 2013-04-26 14:20:22 UTC
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?
Comment 25 Colin Walters 2013-04-26 21:23:05 UTC
(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.
Comment 26 Colin Walters 2013-04-26 21:38:06 UTC
Created attachment 242628 [details] [review]
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch

Updated for comments
Comment 27 Colin Walters 2013-05-15 20:14:11 UTC
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.
Comment 28 Jasper St. Pierre (not reading bugmail) 2013-05-15 20:16:45 UTC
(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.
Comment 29 Jasper St. Pierre (not reading bugmail) 2013-05-15 21:30:08 UTC
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.
Comment 30 Colin Walters 2013-05-17 00:10:41 UTC
Created attachment 244480 [details] [review]
0001-Re-lock-the-screen-if-we-re-restarted-from-a-previou.patch

Drop unnecessary include.
Comment 31 Colin Walters 2013-05-17 00:11:25 UTC
(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.
Comment 32 Jasper St. Pierre (not reading bugmail) 2013-05-23 17:37:26 UTC
Review of attachment 244480 [details] [review]:

OK.
Comment 33 Colin Walters 2013-05-23 20:11:36 UTC
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
Comment 34 drago01 2013-05-25 08:53:59 UTC
*** Bug 700992 has been marked as a duplicate of this bug. ***
Comment 35 Jeremy Bicha 2013-06-08 18:06:45 UTC
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
Comment 36 Serge Gavrilov 2013-10-09 10:39:05 UTC
It seems the patch causes the bug https://bugzilla.gnome.org/show_bug.cgi?id=662780
Comment 37 Florian Müllner 2013-10-09 11:00:07 UTC
(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.
Comment 38 Serge Gavrilov 2013-10-09 11:15:41 UTC
Yeah. You are right. I have reproduced the problem again. Sorry!