changeset 4968:4d35e52790f8

fix crash in glyph-cachels -------------------- ChangeLog entries follow: -------------------- src/ChangeLog addition: 2010-02-03 Ben Wing <ben@xemacs.org> * frame.c (Fmake_frame): * glyphs.c: * glyphs.c (NUM_PRECACHED_GLYPHS): * glyphs.c (get_glyph_cachel_index): * glyphs.c (FROB): * glyphs.c (mark_glyph_cachels_as_not_updated): * redisplay.c (regenerate_window): * redisplay.c (redisplay_window): When creating a frame, call reset_glyph_cachels on the minibuffer window as well as the root window. Fixes a crash due to other glyphs (e.g. the gutter glyph) getting in the glyph cachel before the pre-cached glyphs that are supposed to have fixed indices (continuation-glyph, truncation-glyph, etc.). Add a bunch of asserts to make sure that the glyph cachels always properly contain the pre-cached glyphs.
author Ben Wing <ben@xemacs.org>
date Wed, 03 Feb 2010 21:06:14 -0600
parents 0d4c9d0f6a8d
children cbe181529c34
files src/ChangeLog src/frame.c src/glyphs.c src/redisplay.c
diffstat 4 files changed, 110 insertions(+), 18 deletions(-) [+]
line wrap: on
line diff
--- a/src/ChangeLog	Wed Feb 03 20:51:18 2010 -0600
+++ b/src/ChangeLog	Wed Feb 03 21:06:14 2010 -0600
@@ -1,3 +1,22 @@
+2010-02-03  Ben Wing  <ben@xemacs.org>
+
+	* frame.c (Fmake_frame):
+	* glyphs.c:
+	* glyphs.c (NUM_PRECACHED_GLYPHS):
+	* glyphs.c (get_glyph_cachel_index):
+	* glyphs.c (FROB):
+	* glyphs.c (mark_glyph_cachels_as_not_updated):
+	* redisplay.c (regenerate_window):
+	* redisplay.c (redisplay_window):
+	When creating a frame, call reset_glyph_cachels on the minibuffer
+	window as well as the root window.  Fixes a crash due to other
+	glyphs (e.g. the gutter glyph) getting in the glyph cachel before
+	the pre-cached glyphs that are supposed to have fixed indices
+	(continuation-glyph, truncation-glyph, etc.).
+
+	Add a bunch of asserts to make sure that the glyph cachels always
+	properly contain the pre-cached glyphs.
+
 2010-02-03  Ben Wing  <ben@xemacs.org>
 
 	* device-x.c (x_get_resource_prefix):
--- a/src/frame.c	Wed Feb 03 20:51:18 2010 -0600
+++ b/src/frame.c	Wed Feb 03 21:06:14 2010 -0600
@@ -585,6 +585,8 @@
 
   update_frame_window_mirror (f);
 
+  /* #### Do we need to be calling reset_face_cachels here, and then again
+     down below? */
   if (initialized && !DEVICE_STREAM_P (d))
     {
       if (!NILP (f->minibuffer_window))
@@ -642,8 +644,19 @@
 	 things. */
       init_frame_toolbars (f);
 #endif
+      /* Added this assert recently (2-1-10); seems there should be only
+	 two windows, root and minibufer.  Probably we should just be
+	 calling reset_*_cachels on the root window directly instead of the
+	 selected window, but I want to make sure they are always the
+	 same. --ben */
+      assert (EQ (FRAME_SELECTED_WINDOW (f), f->root_window));
       reset_face_cachels (XWINDOW (FRAME_SELECTED_WINDOW (f)));
       reset_glyph_cachels (XWINDOW (FRAME_SELECTED_WINDOW (f)));
+      if (!NILP (f->minibuffer_window))
+	{
+	  reset_face_cachels (XWINDOW (f->minibuffer_window));
+	  reset_glyph_cachels (XWINDOW (f->minibuffer_window));
+	}
 
       change_frame_size (f, f->height, f->width, 0);
     }
--- a/src/glyphs.c	Wed Feb 03 20:51:18 2010 -0600
+++ b/src/glyphs.c	Wed Feb 03 21:06:14 2010 -0600
@@ -4276,9 +4276,19 @@
 
 
 /*****************************************************************************
- *                     glyph cachel functions	     *
+ *                          glyph cachel functions                           *
  *****************************************************************************/
 
+#define NUM_PRECACHED_GLYPHS 6
+#define LOOP_OVER_PRECACHED_GLYPHS			\
+  FROB (Vcontinuation_glyph, CONT_GLYPH_INDEX)		\
+  FROB (Vtruncation_glyph, TRUN_GLYPH_INDEX)		\
+  FROB (Vhscroll_glyph, HSCROLL_GLYPH_INDEX)		\
+  FROB (Vcontrol_arrow_glyph, CONTROL_GLYPH_INDEX)	\
+  FROB (Voctal_escape_glyph, OCT_ESC_GLYPH_INDEX)	\
+  FROB (Vinvisible_text_glyph, INVIS_GLYPH_INDEX)
+
+
 /* #### All of this is 95% copied from face cachels.  Consider
   consolidating.
 
@@ -4352,6 +4362,27 @@
   Dynarr_add (w->glyph_cachels, new_cachel);
 }
 
+#ifdef ERROR_CHECK_GLYPHS
+
+/* The precached glyphs should always occur in slots 0 - 5, with each glyph in the
+   slot reserved for it.  Meanwhile any other glyphs should always occur in slots
+   6 or greater. */
+static void
+verify_glyph_index (Lisp_Object glyph, glyph_index idx)
+{
+  if (0)
+    ;
+#define FROB(glyph_obj, gindex)			\
+  else if (EQ (glyph, glyph_obj))		\
+    assert (gindex == idx);
+  LOOP_OVER_PRECACHED_GLYPHS
+  else
+    assert (idx >= NUM_PRECACHED_GLYPHS);
+#undef FROB
+}
+
+#endif /* ERROR_CHECK_GLYPHS */
+
 glyph_index
 get_glyph_cachel_index (struct window *w, Lisp_Object glyph)
 {
@@ -4367,6 +4398,9 @@
 
       if (EQ (cachel->glyph, glyph) && !NILP (glyph))
 	{
+#ifdef ERROR_CHECK_GLYPHS
+	  verify_glyph_index (glyph, elt);
+#endif /* ERROR_CHECK_GLYPHS */
 	  update_glyph_cachel_data (w, glyph, cachel);
 	  return elt;
 	}
@@ -4381,12 +4415,10 @@
 reset_glyph_cachels (struct window *w)
 {
   Dynarr_reset (w->glyph_cachels);
-  get_glyph_cachel_index (w, Vcontinuation_glyph);
-  get_glyph_cachel_index (w, Vtruncation_glyph);
-  get_glyph_cachel_index (w, Vhscroll_glyph);
-  get_glyph_cachel_index (w, Vcontrol_arrow_glyph);
-  get_glyph_cachel_index (w, Voctal_escape_glyph);
-  get_glyph_cachel_index (w, Vinvisible_text_glyph);
+#define FROB(glyph_obj, gindex)			\
+  get_glyph_cachel_index (w, glyph_obj);
+  LOOP_OVER_PRECACHED_GLYPHS
+#undef FROB
 }
 
 void
@@ -4394,19 +4426,18 @@
 {
   int elt;
 
+  /* A previous bug resulted from the glyph cachels never getting reset
+     in the minibuffer window after creation, and another glyph added before
+     we got a chance to add the six normal glyphs that should go first, and
+     we got called with only one glyph present. */
+  assert (Dynarr_length (w->glyph_cachels) >= NUM_PRECACHED_GLYPHS);
   /* We need to have a dirty flag to tell if the glyph has changed.
      We can check to see if each glyph variable is actually a
      completely different glyph, though. */
 #define FROB(glyph_obj, gindex)						\
   update_glyph_cachel_data (w, glyph_obj,				\
-			      Dynarr_atp (w->glyph_cachels, gindex))
-
-  FROB (Vcontinuation_glyph, CONT_GLYPH_INDEX);
-  FROB (Vtruncation_glyph, TRUN_GLYPH_INDEX);
-  FROB (Vhscroll_glyph, HSCROLL_GLYPH_INDEX);
-  FROB (Vcontrol_arrow_glyph, CONTROL_GLYPH_INDEX);
-  FROB (Voctal_escape_glyph, OCT_ESC_GLYPH_INDEX);
-  FROB (Vinvisible_text_glyph, INVIS_GLYPH_INDEX);
+			    Dynarr_atp (w->glyph_cachels, gindex));
+  LOOP_OVER_PRECACHED_GLYPHS
 #undef FROB
 
   for (elt = 0; elt < Dynarr_length (w->glyph_cachels); elt++)
@@ -4449,7 +4480,7 @@
 
 
 /*****************************************************************************
- *                     subwindow cachel functions	     *
+ *                        subwindow cachel functions                         *
  *****************************************************************************/
 /* Subwindows are curious in that you have to physically unmap them to
    not display them. It is problematic deciding what to do in
@@ -4546,7 +4577,7 @@
 }
 
 /*****************************************************************************
- *                              subwindow exposure ignorance                    *
+ *                           subwindow exposure ignorance                    *
  *****************************************************************************/
 /* when we unmap subwindows the associated window system will generate
    expose events. This we do not want as redisplay already copes with
--- a/src/redisplay.c	Wed Feb 03 20:51:18 2010 -0600
+++ b/src/redisplay.c	Wed Feb 03 21:06:14 2010 -0600
@@ -5471,6 +5471,17 @@
   Dynarr_reset (dla);
   w->max_line_len = 0;
 
+  /* Added 2-1-10 -- we should never have empty face or glyph cachels
+     because we initialized them at startup and the only way to reduce
+     their number is through calling reset_face_cachels() or
+     reset_glyph_cachels(), which as a side effect sets up a number of
+     standard entries */
+  assert (Dynarr_length (w->face_cachels));
+  assert (Dynarr_length (w->glyph_cachels));
+
+#if 0
+  /* #### Delete this code sometime later than 2-1-10 when we're sure it's
+     not needed */
   /* Normally these get updated in redisplay_window but it is possible
      for this function to get called from some other points where that
      update may not have occurred.  This acts as a safety check. */
@@ -5478,6 +5489,7 @@
     reset_face_cachels (w);
   if (!Dynarr_length (w->glyph_cachels))
     reset_glyph_cachels (w);
+#endif
 
   Fset_marker (w->start[type], make_int (start_pos), w->buffer);
   Fset_marker (w->pointm[type], make_int (point), w->buffer);
@@ -6288,10 +6300,22 @@
     }
   Fset_marker (w->pointm[DESIRED_DISP], make_int (pointm), the_buffer);
 
+  /* Added 2-1-10 -- we should never have empty face or glyph cachels
+     because we initialized them at startup and the only way to reduce
+     their number is through calling reset_face_cachels() or
+     reset_glyph_cachels(), which as a side effect sets up a number of
+     standard entries */
+  assert (Dynarr_length (w->face_cachels));
+  assert (Dynarr_length (w->glyph_cachels));
+
   /* If the buffer has changed we have to invalidate all of our face
      cache elements. */
   if ((!echo_active && b != window_display_buffer (w))
+#if 0
+  /* #### Delete this code sometime later than 2-1-10 when we're sure it's
+     not needed */
       || !Dynarr_length (w->face_cachels)
+#endif
       || f->faces_changed)
     reset_face_cachels (w);
   else
@@ -6301,7 +6325,12 @@
      the cache purely because glyphs have changed - this is now
      handled by the dirty flag.*/
   if ((!echo_active && b != window_display_buffer (w))
-      || !Dynarr_length (w->glyph_cachels) || f->faces_changed)
+#if 0
+  /* #### Delete this code sometime later than 2-1-10 when we're sure it's
+     not needed */
+      || !Dynarr_length (w->glyph_cachels)
+#endif
+      || f->faces_changed)
     reset_glyph_cachels (w);
   else
     mark_glyph_cachels_as_not_updated (w);