Subject: Re: bug#72692: Emacs 31.05 (40eecd594ac) get SIGSEGV on Linux (Linux 6.6.45 Kde Wayland)

From: Pip Cet <[email-masked]>
Date: Mon, 26 Aug 2024 19:20:59 +0000

"Pip Cet" <[email-masked]> writes:

> "Eli Zaretskii" <[email-masked]> writes:
>
>>> Date: Mon, 26 Aug 2024 05:52:11 +0000
>>> From: Pip Cet <[email-masked]>
>>> Cc: Juri Linkov <[email-masked]>, [email-masked], [email-masked]
>>>
>>> >> -      f->face_change = true;
>>> >
>>> > Sorry about that.  I've now reverted that change; we will have to find
>>> > a cheaper solution for the original problem.
>>>
>>> This patch, moving down the setting of f->face_change to when we
>>> definitely know it's required, seems to avoid the
>>> always-in-the-foreground problem, but I'm not sure it fixes Juri's
>>> performance issue:
>>
>> If all we want is to set the face_change flag without causing
>> performance issues, then the patch below does it, and AFAIU should
>> prevent the segfaults in this bug (please check, as I'm unable to
>> trigger the segfaults):
>
> Will do.

That was quick.  With this patch:

diff --git a/src/fontset.c b/src/fontset.c
index 16d14669c89..41d845c9bc5 100644
--- a/src/fontset.c
+++ b/src/fontset.c
@@ -921,8 +921,6 @@ free_face_fontset (struct frame *f, struct face *face)
   eassert (! BASE_FONTSET_P (fontset));
   eassert (f == XFRAME (FONTSET_FRAME (fontset)));
   ASET (Vfontset_table, face->fontset, Qnil);
-  if (face->fontset < next_fontset_id)
-    next_fontset_id = face->fontset;
   if (! NILP (FONTSET_DEFAULT (fontset)))
     {
       int id = XFIXNUM (FONTSET_ID (FONTSET_DEFAULT (fontset)));
@@ -931,8 +929,6 @@ free_face_fontset (struct frame *f, struct face *face)
       eassert (!NILP (fontset) && ! BASE_FONTSET_P (fontset));
       eassert (f == XFRAME (FONTSET_FRAME (fontset)));
       ASET (Vfontset_table, id, Qnil);
-      if (id < next_fontset_id)
-	next_fontset_id = face->fontset;
     }
   face->fontset = -1;
 }
diff --git a/src/xfaces.c b/src/xfaces.c
index 684b6ccfac7..02fc934ee4d 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -733,9 +733,12 @@ recompute_basic_faces (struct frame *f)
 {
   if (FRAME_FACE_CACHE (f))
     {
+      bool was_empty = FRAME_FACE_CACHE (f)->used == 0;
       clear_face_cache (false);
       if (!realize_basic_faces (f))
 	emacs_abort ();
+      if (!was_empty)
+	f->face_change = true;
     }
 }

and this hibiscus.el:

(while t
  (insert (concat (make-string 1 (floor (random 132000)))))
  (set-frame-parameter nil 'alpha-background 1.0)
  (sit-for 1.0))

emacs -Q --load hibiscus.el works.  Without the last hunk, it segfaults
almost immediately.

And the patch makes sense, too: if the face cache was empty, there's no
need to set f->face_change to true.  Is that case sufficiently common to
result in acceptable performance?

Pip