Created attachment 286228 [details] [review]
Do xwayland/wayland window association in a later, not an idle

g_idle_add() makes no guarantee about when it will be run - if Mutter
is busy drawing and blocking glXSwapBuffers() it could happen only
minutes later.  Use meta_later_add (META_LATER_BEFORE_REDRAW) instead -
this will deterministically be run after the Wayland socket is read
from but before the next frame is painted.

Created attachment 286229 [details] [review]
Cleanup xwayland/wayland window association from the "unmanage" signal

Windows can be freed at some point after they are unmanaged - because
there is an effect in progress, because a language binding is holding
a reference. Therefore, we need to clean up the later to associate
the xwayland and wayland windows deterministically in an "unamanaged"
handler.

Review of attachment 286228 [details] [review]:

Looks good.

::: src/tests/stacking/basic-x11.metatest
@@ +1,1 @@
+new_client a1 x11

Unrelated change?

Review of attachment 286229 [details] [review]:

"unamanaged" in the commit message

::: src/wayland/meta-xwayland.c
@@ +86,2 @@
 {
+  if (op->later_id)

if (op->later_id > 0)

is the usual style

@@ +88,3 @@
+    meta_later_remove (op->later_id);
+  g_signal_handlers_disconnect_by_func (op->window,
+                                        (gpointer) associate_window_with_surface_window_unmanaged,

Why the cast to gpointer?

(In reply to comment #4)
> Review of attachment 286229 [details] [review]:
> 
> "unamanaged" in the commit message
> 
> ::: src/wayland/meta-xwayland.c
> @@ +86,2 @@
>  {
> +  if (op->later_id)
> 
> if (op->later_id > 0)
> 
> is the usual style

screen.c:
  if (screen->check_fullscreen_later != 0)
  if (!screen->check_fullscreen_later)

stack-tracker.c:
  if (tracker->sync_stack_later)
  if (tracker->sync_stack_later == 0)

window.c:
          if (queue_pending[queuenum] == NULL && queue_later[queuenum] != 0)
          if (queue_later[queuenum] == 0)

prefs.c:
  if (changed_idle == 0)

utils.c:
  if (later->source)
          if (later->source == 0)

So rather a lack of consistency!, even for the same idle ID, but I don't see that particular pattern ;-) ... I'll change it to != 0, since I like that better than > 0.

> @@ +88,3 @@
> +    meta_later_remove (op->later_id);
> +  g_signal_handlers_disconnect_by_func (op->window,
> +                                        (gpointer)
> associate_window_with_surface_window_unmanaged,
> 
> Why the cast to gpointer?

g_signal_handlers_disconnect_by_func() was accidentally messed up to take a void * rather than a GCallback, so it's impossible to call it in a C standard compliant way. [the same applies to dlsym, so it will actually work fine on any POSIX platform] 

The (gpointer) cast does *not* make it any more C standard compliant. At this point, not sure why I started doing it:

 A) It made some compiler not warn about the construct. Hard to prove positive or negative at this point - current GCC doesn't seem to warn either way unless you add -pendantic and then it warns both ways. A compiler is certainly allowed to emit different diagnostics with and without the cast.
 B) It simply points out what is going on - the natural and nonsensical thing to do is to write G_CALLBACK (associate_window_with_surface_window_unmanage) and if you see people doing the above, you are made to investigate and not accidentally do that.

I thought functions (well, any single pointer type like char *) always had an implicit cast to void *.

(In reply to comment #6)
> I thought functions (well, any single pointer type like char *) always had an
> implicit cast to void *.

Since casting a function to a void * is not defined in standard C at all, there's also no implicit cast from function to void *. GCC allows it and allows the implicit cast.

Attachment 286228 [details] pushed as 9c465a2 - Do xwayland/wayland window association in a later, not an idle
Attachment 286229 [details] pushed as d6624b0 - Cleanup xwayland/wayland window association from the "unmanage" signal