bug#23386: Segfault when messing with font-backend

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 23386@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>
>> Date: Sat, 09 Jul 2016 16:02:30 -0400
>> 
>> > Here is a patch that attempts to fix the issue by resetting the font
>> > after the backend is changed.  It does let Emacs successfully open the
>> > frame with the new font (no funny box characters in the modeline), but
>> > I'm not sure if it's the best way of marking the font object invalid.
>> 
>> Definitely not the best way: it causes segfault on delete-frame.
>
> Backtrace from that segfault?

Hmm, looks like I just tried to kill the font twice, adding a NULL check
fixes it.

>From f405410bf424b0a24a0eac54d12eeed758af95e9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 9 Jul 2016 14:20:53 -0400
Subject: [PATCH v2] Don't segfault on font backend change (Bug #23386)

* src/font.c (font_finish_cache): Kill frame's font if it used the
driver we just turned off.
* src/frame.c (x_set_font_backend): Reset the frame's font if it's been
killed.
---
 src/font.c  |  7 +++++++
 src/frame.c | 16 ++++++++++++++--
 src/frame.h |  1 +
 src/xfns.c  |  2 +-
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/font.c b/src/font.c
index 2519599..21eb95a 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2587,6 +2587,13 @@ font_finish_cache (struct frame *f, struct font_driver 
*driver)
       font_clear_cache (f, XCAR (val), driver);
       XSETCDR (cache, XCDR (val));
     }
+
+  if (FRAME_FONT (f) && FRAME_FONT (f)->driver == driver)
+    {
+      /* Don't leave the frame's font pointing to a closed driver. */
+      store_frame_param(f, Qfont, Qnil);
+      FRAME_FONT (f) = NULL;
+    }
 }
 
 
diff --git a/src/frame.c b/src/frame.c
index 00f25f7..d7454d9 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -3677,6 +3677,8 @@ x_set_font (struct frame *f, Lisp_Object arg, Lisp_Object 
oldval)
 void
 x_set_font_backend (struct frame *f, Lisp_Object new_value, Lisp_Object 
old_value)
 {
+  Lisp_Object frame;
+
   if (! NILP (new_value)
       && !CONSP (new_value))
     {
@@ -3718,11 +3720,21 @@ x_set_font_backend (struct frame *f, Lisp_Object 
new_value, Lisp_Object old_valu
     }
   store_frame_param (f, Qfont_backend, new_value);
 
+  XSETFRAME (frame, f);
+
+  /* If closing the font driver killed the frame's font, we need to
+     get a new one.  */
+  if (!FRAME_FONT (f))
+    x_default_font_parameter (f, Fframe_parameters (frame));
+  if (!FRAME_FONT (f))
+    {
+      delete_frame (frame, Qnoelisp);
+      error ("Invalid frame font");
+    }
+
   if (FRAME_FONT (f))
     {
-      Lisp_Object frame;
 
-      XSETFRAME (frame, f);
       x_set_font (f, Fframe_parameter (frame, Qfont), Qnil);
       face_change = true;
       windows_or_buffers_changed = 18;
diff --git a/src/frame.h b/src/frame.h
index f0cdcd4..5b5349e 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -1356,6 +1356,7 @@ extern void x_set_scroll_bar_default_height (struct frame 
*);
 extern void x_set_offset (struct frame *, int, int, int);
 extern void x_wm_set_size_hint (struct frame *f, long flags, bool 
user_position);
 extern Lisp_Object x_new_font (struct frame *, Lisp_Object, int);
+extern void x_default_font_parameter (struct frame *f, Lisp_Object parms);
 extern void x_set_frame_parameters (struct frame *, Lisp_Object);
 extern void x_set_fullscreen (struct frame *, Lisp_Object, Lisp_Object);
 extern void x_set_line_spacing (struct frame *, Lisp_Object, Lisp_Object);
diff --git a/src/xfns.c b/src/xfns.c
index 7c1bb1c..1b9dd48 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -3071,7 +3071,7 @@ do_unwind_create_frame (Lisp_Object frame)
   unwind_create_frame (frame);
 }
 
-static void
+void
 x_default_font_parameter (struct frame *f, Lisp_Object parms)
 {
   struct x_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
-- 
2.8.0

bug-23386-patch-v1-backtrace.txt
Description: backtrace

>
> I think one idea that could be useful is to trace the creation of
> relevant objects when Emacs starts up, starting with the call to
> font_update_drivers, and then compare that with what happens in this
> case.  That could delineate the missing parts and the differences
> which could point the way to solving this cleanly.

Yeah, I'm still having trouble seeing the forest for the trees.