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

[Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Thiago Melo <tmdmelo@gmail.com> writes:

> Sorry for not being clear. I meant calls to `delete-terminal', when
> Emacs is built with Cairo-XCB, and regardless of the toolkit. It's a
> similar scenario to trigger the bug: launch Emacs daemon, open one or
> more graphical frames, call the elisp function`delete-terminal' (all
> graphical frames and the display are closed because of it), open a new
> graphical frame...
>
> My last patch doesn't fix it because it only works when
> `(delete-frame)' is called instead. This situation is specific for
> toolkitless + Cairo-XCB Emacs, since here it ends up calling
> `x_delete_terminal' -> `XCloseDisplay'. Other toolkits don't call
> `x_delete_terminal' here, due to the logic at `delete_frame'.
>
> I hope I was more clear this time.
>
> My opinion is that it's all the same issue, which boils down to
> Cairo-XCB requiring more manual memory management than Cairo-XLib by
> design or limitation. I think we should really consider the approach
> from my first patch, which is ensuring that the Cairo-XCB device is
> cleaned up before calling XCloseDisplay. Everything else is a
> workaround. If the patch is considered too unsafe or too big, then we
> must clarify the specifics of what makes it so, so the matter can be
> addressed in a better way.

I thought I explained what the problems with trying to fix this in Emacs
are.  The first is: there's a reference leak in Cairo somewhere, since
Emacs never allows displays to be closed without each frame being
destroyed, and destroying each frame will also dereference its Cairo
surface; thus, it's not actually Emacs's problem.

> Here's another patch, similar to the first one, but it only acts at
> `x_delete_terminal', and without storing global references. The
> strategy is similar to the one used at
> `ftcrfont_get_default_font_options'. It creates a dummy pixmap, then a
> dummy cairo xcb surface from it, then it extracts the cairo device
> from the surface, and then cleans up them all.
>
> #+begin_src diff
> --- a/src/xterm.c    2023-05-24 12:42:14.873824624 +0200
> +++ b/src/xterm.c    2023-05-24 13:45:23.798382193 +0200
> @@ -30841,6 +30841,30 @@
>       closing all the displays.  */
>        XrmDestroyDatabase (dpyinfo->rdb);
>  #endif
> +#ifdef USE_CAIRO_XCB_SURFACE
> +      /* Ensure that the cairo device 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)
> +    {
> +      cairo_device_t *cairo_device;
> +      cairo_device = cairo_device_reference (cairo_surface_get_device
> (surface));
> +      cairo_surface_destroy (surface);
> +      cairo_device_finish (cairo_device);
> +      cairo_device_destroy (cairo_device);
> +    }
> +      XFreePixmap (dpyinfo->display, drawable);
> +#endif
>  #ifdef USE_GTK
>        xg_display_close (dpyinfo->display);
>  #else
> #+end_src

The other problem occurs when `cairo_xcb_surface_create' creates a
different device from the one that was previously created for the
display.  So you have only destroyed one of several devices, any one of
which may rear its ugly head later.  This is also a bug in Cairo.

BTW, it's not necessary to call XFreePixmap, as all resources created
by the client will be destroyed per the close down mode set earlier.

Thanks.