bug#63495: 28.2; menu crashes on macos

["Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> Alan Third <alan@idiocy.org> writes:
>
>> Can you please try this:
>>
>> modified   src/nsmenu.m
>> @@ -746,6 +746,8 @@ - (Lisp_Object)runMenuAt: (NSPoint)p forFrame: (struct frame *)f
>>    NSEvent *e, *event;
>>    long retVal;
>>  
>> +  needsUpdate = NO;
>> +
>>    /* p = [view convertPoint:p fromView: nil]; */
>>    p.y = NSHeight ([view frame]) - p.y;
>>    e = [[view window] currentEvent];
>>
>> At a guess, when the menu opens the first thing AppKit does is check if
>> it needs updated, and since a new menu starts with needsUpdate=YES, it
>> goes ahead and tries to do it, which overwrites some important
>> variables from the original "build" of the menu.
>>
>> The context menu is built and then displayed, as opposed to the
>> top-menu that is partially built, then when it's to be displayed is
>> updated and filled in.
>
> I tried it, unfortunately, that doesn't seem to solve the issue.  Emacs
> still crashes with a similar backtrace:
>
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x3)
>     frame #0: 0x00000001000af1fd emacs`AREF(array=0x0000000000000000, idx=0) at lisp.h:1941:10
>    1938	AREF (Lisp_Object array, ptrdiff_t idx)
>    1939	{
>    1940	  eassert (0 <= idx && idx < gc_asize (array));
> -> 1941	  return XVECTOR (array)->contents[idx];
>    1942	}
>    1943
>    1944	INLINE Lisp_Object *
> Target 0: (emacs) stopped.
> (lldb) bt
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x3)
>   * frame #0: 0x00000001000af1fd emacs`AREF(array=0x0000000000000000, idx=0) at lisp.h:1941:10
>     frame #1: 0x00000001000b079e emacs`find_and_return_menu_selection(f=0x00000001030b0c30, keymaps=true, client_data=0x0000000110277e60) at menu.c:985:11
>     frame #2: 0x000000010037cc1a emacs`-[EmacsMenu runMenuAt:forFrame:keymaps:](self=0x000060000179c9c0, _cmd="runMenuAt:forFrame:keymaps:", p=(x = 2, y = 411), f=0x00000001030b0c30, keymaps=true) at nsmenu.m:769:9
>     frame #3: 0x0000000100380ac0 emacs`ns_menu_show(f=0x00000001030b0c30, x=2, y=97, menuflags=1, title=0x0000000102416284, error=0x00007ff7bfefcef0) at nsmenu.m:1069:9
>     frame #4: 0x00000001000b23f7 emacs`x_popup_menu_1(position=0x0000000118583f83, menu=0x0000000118587313) at menu.c:1410:17
>     frame #5: 0x00000001000b27d2 emacs`Fx_popup_menu(position=0x0000000118583f83, menu=0x0000000118587313) at menu.c:1474:10
>     frame #6: 0x000000010024e7d7 emacs`funcall_subr(subr=0x0000000100403c28, numargs=2, args=0x0000000118028188) at eval.c:3049:15

One fundamental problem I see with the current logic (or maybe I'm
misunderstanding something) is that, menuNeedsUpdate: is called for both
the menubar and for context menus.  However, the ns_update_menubar
routine does not even use the NSMenu that AppKit passed to
menuNeedsUpdate:, it always does this:

id menu = [NSApp mainMenu];

This means that the menu variable will always be the menubar.  The code
in ns_update_menubar is only prepared to handle menubar updates, but as
this function updates menu_items, a data structure that is used later by
the context menu in find_and_return_menu_selection, Emacs crashes
because of inconsistencies.

Is there any reason we need to do something for context menus in
menuNeedsUpdate:?  Alan said that context menus are completely built in
advance (as opposed to the menubar, which is partially built), so I
propose the following patch (which does seem to fix the crash for me and
doesn't cause regressions with the menubar):

diff --git a/src/nsmenu.m b/src/nsmenu.m
index 2c1f575bdf2..ca367d1abd1 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -477,6 +477,16 @@ - (instancetype)initWithTitle: (NSString *)title
    call to ns_update_menubar.  */
 - (void)menuNeedsUpdate: (NSMenu *)menu
 {
+
+  /* The context menu is built and then displayed, as opposed to the
+     top-menu, which is partially built and then updated and filled in
+     when it's time to display it.  Therefore, we don't call
+     ns_update_menubar if a context menu is active. */
+  if (context_menu_value != 0)
+    {
+      return;
+    }
+
 #ifdef NS_IMPL_GNUSTEP
   static int inside = 0;
 #endif