bug#77128: 29.4; Memory leak in visual bell (pgtk_flash) on pure GTK Wayland builds

[Eli Zaretskii]

Ping!

> Cc: 77128@debbugs.gnu.org, dmr@ethzero.com
> Date: Sat, 29 Mar 2025 14:35:14 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: Alessandro Di Marco <dmr@ethzero.com>,  77128@debbugs.gnu.org
> > Date: Thu, 20 Mar 2025 16:21:32 +0800
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> From: Alessandro Di Marco <dmr@ethzero.com>
> > >> Date: Wed, 19 Mar 2025 22:45:12 +0100
> > >> 
> > >> When Emacs is built with “pure GTK” internals for full Wayland
> > >> compatibility, repeated invocations of the visual bell (triggered via
> > >> pgtk_flash) result in a significant memory leak. The issue occurs
> > >> because pgtk_flash creates a new Cairo surface (cr_surface_visible_bell)
> > >> using cairo_surface_create_similar each time the visual bell is
> > >> activated. If flashes occur in rapid succession, a previously created
> > >> surface may still be stored when a new flash is initiated. Although a
> > >> timer (atimer_visible_bell) is set to call recover_from_visible_bell
> > >> after 50 ms (which in turn destroys the surface), the code does not
> > >> check whether an existing surface and its timer are active before
> > >> overwriting them.
> > >> 
> > >> This leads to the accumulation of unreleased surfaces if the visual bell
> > >> is triggered faster than the timer can clear them. To address this, the
> > >> patch attached here explicitly checks if cr_surface_visible_bell is
> > >> non‑NULL and, if so, calls cairo_surface_destroy to free it and cancels
> > >> any active atimer before assigning a new surface.
> > >> 
> > >> Signed-off-by: Alessandro Di Marco <dmr@ethzero.com>
> > >> ---
> > >> diff '--color=auto' -urN old/src/pgtkterm.c new/src/pgtkterm.c
> > >> --- old/src/pgtkterm.c 2025-03-19 09:46:10.652352642 +0100
> > >> +++ new/src/pgtkterm.c 2025-03-19 20:54:32.448191644 +0100
> > >> @@ -3782,6 +3782,18 @@
> > >>        cairo_fill (cr);
> > >>      }
> > >>  
> > >> +  if (FRAME_X_OUTPUT (f)->cr_surface_visible_bell != NULL)
> > >> +    {
> > >> +      cairo_surface_destroy (FRAME_X_OUTPUT 
> > >> (f)->cr_surface_visible_bell);
> > >> +      FRAME_X_OUTPUT (f)->cr_surface_visible_bell = NULL;
> > >> +    }
> > >> +
> > >> +  if (FRAME_X_OUTPUT (f)->atimer_visible_bell != NULL)
> > >> +    {
> > >> +      cancel_atimer (FRAME_X_OUTPUT (f)->atimer_visible_bell);
> > >> +      FRAME_X_OUTPUT (f)->atimer_visible_bell = NULL;
> > >> +    }
> > >> +  
> > >>    FRAME_X_OUTPUT (f)->cr_surface_visible_bell = surface;
> > >>  
> > >>    delay = make_timespec (0, 50 * 1000 * 1000);
> > >> --
> > >> 
> > >> Rationale: The patch ensures that if a previous flash’s surface (and
> > >> timer) exists, it is cleaned up before a new flash surface is
> > >> allocated. This prevents the accumulation of stale surfaces and
> > >> eliminates the memory leak observed on repeated visual bell invocations.
> > >
> > > Thanks.
> > >
> > > Po Lu, any comments?  If you agree with the patch, do you think it's
> > > safe enough to go into Emacs 30.2?
> > 
> > I believe that it is safe enough, and also that the leak impacts the
> > X11/Cairo configuration.  I'll install this change in both
> > configurations this weekend.
> 
> Did you have time to install it?  When you do, please close the bug.
> 
> 
> 
>