bug#63589: [PATCH] 29.0.91; crash after creating graphical frames via emacsclient when compiled with cairo-xcb

[Po Lu]

Thiago Melo <tmdmelo@gmail.com> writes:

> So, any application that uses Cairo-XCB with multiple surfaces and
> wants to reopen displays _must_ save a reference to the device and
> _must_ finish + destroy it before closing the display.
>
> With this, here's another try to improve the initial patch, this time
> storing the cairo xcb device for the display at `x_term_init':
>
> #+begin_src diff
> --- a/src/xterm.h    2023-05-25 09:43:50.943793850 +0200
> +++ b/src/xterm.h    2023-05-25 11:32:03.701771148 +0200
> @@ -883,6 +883,13 @@ struct x_display_info
>       clock, or 0 if unknown (if the difference is legitimately 0,
>       server_time_monotonic_p will be true).  */
>    int_fast64_t server_time_offset;
> +
> +#if defined USE_XCB && defined USE_CAIRO_XCB
> +  /* Cairo device associated with cairo surfaces in this display.
> +     Required for proper cleanup before closing display connection
> +     in cairo-xcb builds.  */
> +  cairo_device_t *cairo_device;
> +#endif
>  #endif
>  };
> #+end_src
>
> #+begin_src diff
> --- a/src/xterm.c    2023-05-25 09:37:24.811402435 +0200
> +++ b/src/xterm.c    2023-05-25 12:18:06.003572028 +0200
> @@ -5806,10 +5806,15 @@ x_begin_cr_clip (struct frame *f, GC gc)
>        cairo_surface_t *surface;
>  #ifdef USE_CAIRO_XCB_SURFACE
>        if (FRAME_DISPLAY_INFO (f)->xcb_visual)
> +    {
>      surface = cairo_xcb_surface_create (FRAME_DISPLAY_INFO 
> (f)->xcb_connection,
>                          (xcb_drawable_t) FRAME_X_RAW_DRAWABLE (f),
>                          FRAME_DISPLAY_INFO (f)->xcb_visual,
>                          width, height);

> +    if (cairo_surface_status (surface) == CAIRO_STATUS_SUCCESS)
> +      eassert (FRAME_DISPLAY_INFO (f)->cairo_device
> +           == cairo_surface_get_device (surface));

Did you build with checking?  Because when I last tried, this assert
triggered with the second frame created.

> +    }
>        else
>  #endif
>      surface = cairo_xlib_surface_create (FRAME_X_DISPLAY (f),
> @@ -30504,6 +30509,27 @@ x_term_init (Lisp_Object display_name, c
>
>    unblock_input ();
>
> +#ifdef USE_CAIRO_XCB_SURFACE
> +  /* Store reference to the cairo device for this display, to ensure
> +     that it is destroyed before closing connection (Bug#63589).
> +     For that, we create a drawable, an XCB surface for that drawable,
> +     and then we get the device reference from there.  */
> +  Pixmap drawable;
> +  cairo_surface_t *surface;
> +
> +  drawable = XCreatePixmap (dpyinfo->display, dpyinfo->root_window,
> +                1, 1, dpyinfo->n_planes);
> +  surface = cairo_xcb_surface_create (dpyinfo->xcb_connection, drawable,
> +                      dpyinfo->xcb_visual, 1, 1);
> +
> +  if (cairo_surface_status (surface) == CAIRO_STATUS_SUCCESS)
> +    {
> +      dpyinfo->cairo_device = cairo_device_reference
> (cairo_surface_get_device (surface));
> +      cairo_surface_destroy (surface);
> +    }
> +  XFreePixmap (dpyinfo->display, drawable);
> +#endif
> +
>  #if defined HAVE_XFIXES && defined USE_XCB
>    SAFE_FREE ();
>  #endif
> @@ -30783,6 +30809,17 @@ x_delete_terminal (struct terminal *term
>      xim_close_dpy (dpyinfo);
>  #endif
>
> +#ifdef USE_CAIRO_XCB_SURFACE
> +  /* Ensure that the cairo device is destroyed before closing
> +     connection (Bug#63589).  */
> +  if (dpyinfo->cairo_device)
> +    {
> +      cairo_device_finish (dpyinfo->cairo_device);
> +      cairo_device_destroy (dpyinfo->cairo_device);
> +      dpyinfo->cairo_device = NULL;
> +    }
> +#endif

If we are going down this route, I think we should save each distinct
device returned by `cairo_surface_get_device', and delete each of them
upon the terminal being deleted.

As I explained, I saw that function return different devices for the
same XCB connection, which is definitely a problem with Cairo.

But again, that's a hack.  I would rather just disable this misdesigned
and buggy interface by default.