1:58 a.m.
Jerry James writes:
Okay, I propose this patch. If nobody objects, I will commit it in
a
day or two. Hmmm, upon looking at my changelog entry again, I can
suddenly hear Jeff Lynne singing, "Don't bring me down..."
I prefer Neil Young's classic live intro "This is a song that's
guaranteed to bring you right down. It's called 'Don't Let It Bring
You Down'".
data = compute_menubar_data (f, menubar, deep_p);
- assert (data && (data->next || data->contents));
+ if (!data || (!data->next && !data->contents))
+ return 0;
The problem with this simple patch is that you may just be pushing the
crash around. There's another call to compute_menubar_data later in
menubar-x.c at line 596; make sure it is similarly protected if
necessary. Grep says those are the only calls (it's static so all
calls are in menubar-x.c).
There probably also should be a comment on compute_menubar_data
warning that it can return NULL.
I wonder if it *should* be able to return NULL, or if there's a bug
elsewhere.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
10:18 p.m.
New subject: Issue 802: menubar-x.c assertion failure
On Wed, Jan 16, 2013 at 6:58 PM, Stephen J. Turnbull <stephen(a)xemacs.org> wrote:
The problem with this simple patch is that you may just be pushing
the
crash around. There's another call to compute_menubar_data later in
menubar-x.c at line 596; make sure it is similarly protected if
necessary. Grep says those are the only calls (it's static so all
calls are in menubar-x.c).
There probably also should be a comment on compute_menubar_data
warning that it can return NULL.
I wonder if it *should* be able to return NULL, or if there's a bug
elsewhere.
I believe that returning NULL is all it can do if it is handed a
broken menu specification. Ben's error wrapping stuff is right in the
middle of the call stack, so probably this code used to (unsafely)
throw Lisp errors right past compute_menubar_data, so the assert was
correct. With the current code structure, though, we need to keep
unraveling the stack upwards until we reach a point where the delayed
error can be rethrown and handled properly (by displaying it in
*Warnings*).
I think. At least it seems to work that way when handed the broken
ESS menu specification.
Try this patch on for size:
diff -r 8b5bdc8aebfd src/ChangeLog
--- a/src/ChangeLog Sat Jan 05 02:11:23 2013 +0900
+++ b/src/ChangeLog Fri Jan 18 15:14:06 2013 -0700
＠＠ -1,3 +1,9 ＠＠
+2013-01-16 Jerry James <james(a)xemacs.org>
+
+ * menubar-x.c (set_frame_menubar): when a menubar specification has an
+ error, don't fail an assert() and bring XEmacs down. Instead, return
+ 0 to just skip the faulty menu and show any errors in *Warnings*.
+
2013-01-04 Stephen J. Turnbull <stephen(a)xemacs.org>
* XEmacs 21.5.33 "horseradish" is released.
diff -r 8b5bdc8aebfd src/menubar-x.c
--- a/src/menubar-x.c Sat Jan 05 02:11:23 2013 +0900
+++ b/src/menubar-x.c Fri Jan 18 15:14:06 2013 -0700
＠＠ -516,6 +516,9 ＠＠
}
}
+/* Returns the converted menubar, or NULL if an error is encountered while
+ * converting the Lisp menu specification.
+ */
static widget_value *
compute_menubar_data (struct frame *f, Lisp_Object menubar, int deep_p)
{
＠＠ -573,7 +576,8 ＠＠
menubar_visible = !NILP (w->menubar_visible_p);
data = compute_menubar_data (f, menubar, deep_p);
- assert (data && (data->next || data->contents));
+ if (!data || (!data->next && !data->contents))
+ return 0;
if (!FRAME_X_MENUBAR_ID (f))
FRAME_X_MENUBAR_ID (f) = new_lwlib_id ();
＠＠ -594,6 +598,8 ＠＠
{
free_popup_widget_value_tree (data);
data = compute_menubar_data (f, menubar, 1);
+ if (!data || (!data->next && !data->contents))
+ return 0;
}
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
2:06 p.m.
New subject: Issue 802: menubar-x.c assertion failure
APPROVE
Your logic about returning NULL has sold me. Your original patch is
the right way to go as far as it goes, and the new comment addresses
the documentation issue. Ship it!
Steve
Jerry James writes:
I believe that returning NULL is all it can do if it is handed a
broken menu specification. Ben's error wrapping stuff is right in the
middle of the call stack, so probably this code used to (unsafely)
throw Lisp errors right past compute_menubar_data, so the assert was
correct. With the current code structure, though, we need to keep
unraveling the stack upwards until we reach a point where the delayed
error can be rethrown and handled properly (by displaying it in
*Warnings*).
I think. At least it seems to work that way when handed the broken
ESS menu specification.
Make sense.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
5:20 p.m.
New subject: Issue 802: menubar-x.c assertion failure
COMMIT 21.5
On Sat, Jan 19, 2013 at 7:06 AM, Stephen J. Turnbull <stephen(a)xemacs.org> wrote:
APPROVE
Your logic about returning NULL has sold me. Your original patch is
the right way to go as far as it goes, and the new comment addresses
the documentation issue. Ship it!
This patch has been committed.
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
4605
days inactive
4609
days old
3 comments
2 participants
participants (2)
-
Jerry James -
Stephen J. Turnbull