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

[Thiago Melo <tmdmelo@gmail.com>]

On Thu, May 25, 2023 at 7:12 AM Eli Zaretskii <eliz@gnu.org> wrote:
> I'm trying to establish if there's anything we could do in the
> cairo-xcb configuration to make the crashes more rare, or even prevent
> them altogether.

Regarding this, before posting at the cairo mailing list, I searched
better their archives to check if this issue was already properly
addressed. I found this:

https://lists.cairographics.org/archives/cairo/2018-November/028791.html

Title: cairo_xcb_surface_create() segfaults on second call with
different xcb info

Where Uli Schlachter (libxcb contributor and main maintainer of
Cairo-XCB nowadays) discusses the issue we're having here, the design
of Cairo-XCB, how to use it with multiple surfaces and after reopening
the display. To highlight, Uli says:

| Cairo has to get quite some information from the X11 server. [...]
|
| [...] querying this all the time would be slow. Thus, cairo caches
| this information. Namely, there is an instance of cairo_device_t. This
| instance is kept around even when all surfaces using this device are
| destroyed. [...]
|
| [...] when you call xcb_disconnect(),
| the cache now contains a dangling pointer. The next call to
| xcb_connect() might very well allocate an xcb_connection_t* with the
| same pointer. Thus, you now get a cache hit even though there is a new
| XCB connection. Bad things happen afterwards

Thus, it doesn't matter if Emacs destroys all cairo-xcb surfaces
before closing the display, the device reference will always linger
around together with all its cairo cache by design. It simply doesn't
work like Cairo-XLib at all.

Then, Uli says:

| If you want to keep the device around for later (i.e. have multiple
| surface using the same device), you can save a pointer via:
|
|   cairo_device_t *device = cairo_device_reference(....);
|
| Now, you have to later call cairo_device_destroy() when you no longer
| need the reference, but you get a pointer to the cairo_device_t
| independent of a cairo xcb surface.
|
| Oh and: You have to finish the device before you call xcb_disconnect().

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));
+    }
       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
+
   /* Normally, the display is available...  */
   if (dpyinfo->display)
     {
#+end_src