Created attachment 223458 [details] [review]
ScreenShield: wait for stage mapping before taking a grab

In gdm, we would attempt to become modal during the synchronous initialization,
and this would fail, as X prevents grabs on unmapped windows. Instead,
wait for the stage to be visible before becoming modal.

Review of attachment 223458 [details] [review]:

I thought the stage was mapped synchronously as the first thing in meta_run, before we ran any JS code.

I don't know, I didn't check through.
What I do know is that the sync pushModal always fails, whereas with this patch it works.
I thought it could be a timestamp problem, but even using get_current_time_roundtrip() doesn't fix it, so if I read the Xlib docs right, the next candidate is GrabNotViewable, ie. the stage is not mapped.

(Btw, I think it would flicker if we mapped the stage before the first paint)

(In reply to comment #2)
> Review of attachment 223458 [details] [review]:
> 
> I thought the stage was mapped synchronously as the first thing in meta_run,
> before we ran any JS code.

No it isn't.

From compositor.c:

  /*
   * Delay the creation of the overlay window as long as we can, to avoid
   * blanking out the screen. This means that during the plugin loading, the
   * overlay window is not accessible; if the plugin needs to access it
   * directly, it should hook into the "show" signal on stage, and do
   * its stuff there.
   */

We do connect to the show signal in xdndHandler.js for the exact same reason. We should do the same here instead of using Meta.LaterType.BEFORE_REDRAW for consistency with both the comment in compositor.c and what we do in other places (Xdndhandler).

Review of attachment 223458 [details] [review]:

::: js/ui/screenShield.js
@@ +620,3 @@
+        // Ensure that the stage window is mapped, before taking a grab
+        // otherwise X errors out
+        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() {

Connect to the stage's show signal instead.

(In reply to comment #5)
> Review of attachment 223458 [details] [review]:
> 
> ::: js/ui/screenShield.js
> @@ +620,3 @@
> +        // Ensure that the stage window is mapped, before taking a grab
> +        // otherwise X errors out
> +        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,
> function() {
> 
> Connect to the stage's show signal instead.

No way. If I use the show signal, I get a completely transparent screen (i.e. the lock dialog group is not shown)
I worked around it by commenting the pushModal part (and stuff still works because LoginDialog does another pushModal), but I feel this is wrong.

Accepted after IRC discussion.
Attachment 223458 [details] pushed as be290fa - ScreenShield: wait for stage mapping before taking a grab