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

[Thiago Melo <tmdmelo@gmail.com>]

> > > > > What is the kind of situations in which these crashes could happen?
> > > >
> > > > Precisely that described in this bug report: when displays are closed
> > > > and reopened within a short time period.
> > >
> > > What kind of user-level situations could cause this?  Is invoking
> > > emacsclient soon after deleting the last visible frame the only one?
> > > And what does "short time period" mean, quantitatively? milliseconds?
> > > seconds? minutes?
> >
> > Sorry, in my experience it seems that the time interval between
> > closing the display and opening it again doesn't matter. It seems to
> > be more about the amount of times that the display is closed and then
> > opened (which is often 3 times for me, for whatever reason).
> >
> > I'm testing it here again with Xvfb and an automation script, with a
> > 10 minutes delay after creating a single graphical frame, and another
> > 10 minutes delay after closing it and before creating a new one. I'll
> > report the results soon.
> >
> > Also, this bug seems more likely to happen when emacs is built without
> > a toolkit (which is was I've been testing so far), since the display
> > is always closed after the last graphical frame is closed. Which made
> > me realize, after looking at frame.c, that this bug might as well join
> > the family of Bug#5802, Bug#21509, Bug#23499, and Bug#27816.
>
> With 10 minutes intervals, I got the X errors previously mentioned by
> the 3rd time the display was opened, and then emacs crashed by the 5th
> time the display was opened. So, assuming that 10 minutes is close
> enough to infinity, we can say that the time interval doesn't matter.

So, trying to gather everything into a summary here.

To trigger the bug, all the following conditions must be met:

- Emacs built without a toolkit
- Emacs built with Cairo-XCB
- Emacs started in daemon mode
- The user closes all graphical frames and creates a new one (manually
or programatically, duration in between doesn't matter, amount of
times is not certain)

It goes into the `delete_frame' -> `Fdelete_terminal' ->
`x_delete_terminal' -> `XCloseDisplay' path, where cairo-xcb
references are not destroyed, leading to X errors and emacs crashing.

What else I've tested so far, that didn't trigger the bug:

- Closing the X Server
- xkill'ing graphical frames

These two cases go into the `x_connection_closed' ->
`Fdelete_terminal' -> `x_delete_terminal' path, where `XCloseDisplay'
is not called.

Other builds I've tested, that didn't trigger the bug:

- GTK + Cairo-XCB
- Lucid + Cairo-XCB

Which doesn't end up calling XCloseDisplay, since the terminal is not
deleted when the last graphical frame is closed (due to the infamous
longstanding GTK bug, Bug#5802, Bug#21509, Bug#23499, and Bug#27816).

Considering all the above, I propose this smaller (and potentially
temporary) patch:


#+begin_src diff
--- a/src/frame.c    2023-05-22 19:52:25.155145242 +0200
+++ b/src/frame.c    2023-05-22 20:13:41.548566364 +0200
@@ -2206,14 +2206,15 @@
     /* If needed, delete the terminal that this frame was on.
        (This must be done after the frame is killed.)  */
     terminal->reference_count--;
-#if defined (USE_X_TOOLKIT) || defined (USE_GTK)
+#if defined (USE_X_TOOLKIT) || defined (USE_GTK) || defined (USE_CAIRO_XCB)
     /* FIXME: Deleting the terminal crashes emacs because of a GTK
        bug.
        https://lists.gnu.org/r/emacs-devel/2011-10/msg00363.html */

     /* Since a similar behavior was observed on the Lucid and Motif
-       builds (see Bug#5802, Bug#21509, Bug#23499, Bug#27816), we now
-       don't delete the terminal for these builds either.  */
+       builds (see Bug#5802, Bug#21509, Bug#23499, Bug#27816), and builds
+       without a toolkit together with Cairo-XCB support (Bug#63589),
+       we now don't delete the terminal for these builds either.  */
     if (terminal->reference_count == 0
     && (terminal->type == output_x_window
         || terminal->type == output_pgtk))
#+end_src


Caveat: I've tested it, the errors and crash were gone, but it likely
introduces potential leaks in this build (which probably happens with
GTK and Lucid builds too anyway, from what I've seen in the wild). No
free lunch.