Created attachment 245309 [details] [review]
display: Ensure that we ignore our own focus events for focus predictions

When we set the input focus, we first set the predicted window,
and then try to process focus events. But as XI_FocusOut on the
existing window comes before XI_FocusIn on the new window, we'll
see the focus out on the old window and think the focus is going
to nothing, which makes mutter think the prediction failed.

This didn't really matter as nothing paid attention to the focus
window changing, but with gnome-shell's focus rework, we'll try
and drop keyboard focus in events like these.

Fix this by making sure that we ignore focus window changes of our
own cause when updating the focus window field, by ignoring all
focus events that have a serial the same as the focus request or
lower. Note that if mutter doens't make any requests after the
focus request, this could be racy, as another client could steal
the focus, but mutter would ignore it as the serial was the same.
Bump the serial by making a dummy ChangeProperty request to the
root window in this case.

Review of attachment 245309 [details] [review]:

To simply get a newer serial, there's no need for the ChangeProperty - the UngrabServer request itself will bump the serial.

It should be noted tthat we get a new serial from the UngrabServer alone. I suggested ChangeProperty because:

 * I didn't think of the fact that we could just use the UngrabServer as the request, so I thought we needed *some* request

 * The event we get in return could be useful. In particular if we fail to change the focus (which basically means someone else got the focus with a newer timestamp), then ""Earlier attempt to focus %s failed" code in event_callback() will run immediately rather than waiting for the next event we get. Doesn't matter a lot since we'll get an event soon enough - damage, user time notify, something, but it's a little nicer.

The code needs to use meta_display_grab()/meta_display_ungrab() which handles two details:

 * It is necessary to XFlush() after ungrabbing the server, to prevent the case where someone is stepping through the calling code in gdb without actually stepping into the grab, and the XGrabServer() gets flushed, but the XUngrabServer() doesn't. [Obviously less important for mutter than for other uses of Xlib, since you can't meaningfully gdb Mutter from within the same session, but good hygiene in any case]
 * If this code is called with the server already grabbed, it doesn't ungrab the outer grab. Hmm - but that means that we *can't* count on XUngrabServer() to increment the serial - so we *do* need something.

::: src/core/display.c
@@ +1965,3 @@
+    unsigned long data[1];
+
+    data[0] = meta_display_get_current_time (display);

This is a weird choice... it could be 0. If you want to always change, use the serial, or your own incrementing counter. But note

 "This event is reported to clients selecting PropertyChange on the window and is generated with state NewValue when a property of the window is changed using ChangeProperty or RotateProperties, even when adding zero-length data using ChangeProperty and when replacing all or part of a property with identical data using ChangeProperty or RotateProperties."

So there's no need to make it change. See meta_display_get_current_time_roundtrip().

@@ +1967,3 @@
+    data[0] = meta_display_get_current_time (display);
+
+    XChangeProperty (display->xdisplay, screen->xroot,

I'd rather you used display->timestamp_pinging_window - it's always better to have events generated on a private window than a root window when we can - other clients may be selecting on the root window and will be woken up to have an event delivered. You are going to have switch get_current_time_roundtrip() to use XIfEvent and check for an event with the right serial since the events we generate on the timestamp pinging window would break the use of XWindowEvent() (ugh), but on balance I still prefer that to using the root window or some other random window.

Created attachment 245482 [details] [review]
display: Ensure that we ignore our own focus events for focus predictions

When we set the input focus, we first set the predicted window,
and then try to process focus events. But as XI_FocusOut on the
existing window comes before XI_FocusIn on the new window, we'll
see the focus out on the old window and think the focus is going
to nothing, which makes mutter think the prediction failed.

This didn't really matter as nothing paid attention to the focus
window changing, but with gnome-shell's focus rework, we'll try
and drop keyboard focus in events like these.

Fix this by making sure that we ignore focus window changes of our
own cause when updating the focus window field, by ignoring all
focus events that have a serial the same as the focus request or
lower. Note that if mutter doens't make any requests after the
focus request, this could be racy, as another client could steal
the focus, but mutter would ignore it as the serial was the same.
Bump the serial by making a dummy ChangeProperty request to the
root window in this case.

Review of attachment 245482 [details] [review]:

In the commit message:

> Bump the serial by making a dummy ChangeProperty request to the
> root window in this case.

"root window"

::: src/core/display.c
@@ +1977,3 @@
                   timestamp);
+
+  serial = XNextRequest (display->xdisplay);

You want the serial of the SetInputFocus, right?

@@ +1982,3 @@
+    unsigned long data[1] = { 0 };
+
+    XChangeProperty (display->xdisplay, display->timestamp_pinging_window,

As I mentioned in my last review, using timsetamp_pinging_window is going to require adaption of the code to get roundtrip server time - that currently assumes that the *first* PropertyNotify on the pinging window is the one it looks for.

(In reply to comment #4)
> Review of attachment 245482 [details] [review]:
> 
> In the commit message:
> 
> > Bump the serial by making a dummy ChangeProperty request to the
> > root window in this case.
> 
> "root window"
> 
> ::: src/core/display.c
> @@ +1977,3 @@
>                    timestamp);
> +
> +  serial = XNextRequest (display->xdisplay);
> 
> You want the serial of the SetInputFocus, right?

No? The code is:

  if (display->server_focus_serial >= display->focus_serial)

So we want to make sure that we ignore FocusOut events that come from that serial. Either the serial is of the ChangeProperty (the serial after the SetInputFocus), *or* we change this to a >.

I'm fine with either, but wouldn't making the serial the same as before keep the bug alive?

Created attachment 245609 [details] [review]
display: Ensure that we ignore our own focus events for focus predictions

When we set the input focus, we first set the predicted window,
and then try to process focus events. But as XI_FocusOut on the
existing window comes before XI_FocusIn on the new window, we'll
see the focus out on the old window and think the focus is going
to nothing, which makes mutter think the prediction failed.

This didn't really matter as nothing paid attention to the focus
window changing, but with gnome-shell's focus rework, we'll try
and drop keyboard focus in events like these.

Fix this by making sure that we ignore focus window changes of our
own cause when updating the focus window field, by ignoring all
focus events that have a serial the same as the focus request or
lower. Note that if mutter doens't make any requests after the
focus request, this could be racy, as another client could steal
the focus, but mutter would ignore it as the serial was the same.
Bump the serial by making a dummy ChangeProperty request to a
mutter-controlled window in this case.

(In reply to comment #5)

> > ::: src/core/display.c
> > @@ +1977,3 @@
> >                    timestamp);
> > +
> > +  serial = XNextRequest (display->xdisplay);
> > 
> > You want the serial of the SetInputFocus, right?
> 
> No? The code is:
> 
>   if (display->server_focus_serial >= display->focus_serial)

True for that code. But we also have:

  if (event->xany.serial > display->focus_serial &&
      display->focus_window &&
      display->focus_window->xwindow != display->server_focus_window)
    {
      meta_topic (META_DEBUG_FOCUS, "Earlier attempt to focus %s failed\n",
                  display->focus_window->desc);

And that definitely wants display->focus_serial to be the serial of the request to change the focus. I think it's also confusion strange if when we get the focus event back for a change we make, we set display->server_focus_serial to display->focus_serial - 1.

So I think we want to change the check to be >, rather than using the higher serial.

Review of attachment 245609 [details] [review]:

I think it's pretty bizarre to change properties on the pinging window in two places, and in one place do:

      /* Using the property XA_PRIMARY because it's safe; nothing
       * would use it as a property. The type doesn't matter.
       */

And in the other place add a new _MUTTER_FOCUS_SET atom. I'm fine with any of three things:

 * Use XA_PRIMARY for one, and, say, XA_SECONDARY for the other
 * Use a special atom for both
 * Use XA_PRIMARY (or a special atom) for both, and check the serial in the XIfEvent call to find the event we are looking for

That, and the choice of the serial are the only remaining problems I see here.

Created attachment 245887 [details] [review]
display: Ensure that we ignore our own focus events for focus predictions

When we set the input focus, we first set the predicted window,
and then try to process focus events. But as XI_FocusOut on the
existing window comes before XI_FocusIn on the new window, we'll
see the focus out on the old window and think the focus is going
to nothing, which makes mutter think the prediction failed.

This didn't really matter as nothing paid attention to the focus
window changing, but with gnome-shell's focus rework, we'll try
and drop keyboard focus in events like these.

Fix this by making sure that we ignore focus window changes of our
own cause when updating the focus window field, by ignoring all
focus events that have a serial the same as the focus request or
lower. Note that if mutter doens't make any requests after the
focus request, this could be racy, as another client could steal
the focus, but mutter would ignore it as the serial was the same.
Bump the serial by making a dummy ChangeProperty request to a
mutter-controlled window in this case.

Review of attachment 245887 [details] [review]:

Looks good

Attachment 245887 [details] pushed as 7fdfbad - display: Ensure that we ignore our own focus events for focus predictions