Fix for Emacs Crash

Fix for Emacs Crash

[Ben Key]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

The following patch fixes a bug I experienced when running a build of GNU
Emacs I made on Windows XP SP1 on Windows 2000 SP2.  The crash occurred when
starting Emacs.

<patch>
--- _21.3/src/w32menu.c	2002-08-05 12:33:44.000000000 -0400
+++ 21.3/src/w32menu.c	2002-11-06 23:01:11.000000000 -0500
@@ -2215,9 +2215,11 @@
 	      info.fState = wv->selected ? MFS_CHECKED : MFS_UNCHECKED;
 	    }

-	  set_menu_item_info (menu,
-			      item != NULL ? (UINT) item : (UINT) wv->call_data,
-			      FALSE, &info);
+    set_menu_item_info (
+        menu,
+        item != NULL ? (UINT) item : (UINT) wv->call_data,
+        item != NULL ? FALSE : TRUE,
+        &info);
 	}
     }
   return return_value;
</patch>

The logic behind this patch is as follows:
* The documentation of SetMenuItemInfo from the MSDN Library is as follows:
  SetMenuItemInfo
  The SetMenuItemInfo function changes information about a menu item.

  BOOL SetMenuItemInfo(
    HMENU hMenu, // handle to menu
    UINT uItem, // identifier or position
    BOOL fByPosition, // meaning of uItem
    LPMENUITEMINFO lpmii // menu item information
    );
  Parameters
    hMenu
      [in] Handle to the menu that contains the menu item.
    uItem
      [in] Identifier or position of the menu item to change.
      The meaning of this parameter depends on the value of
      fByPosition.
    fByPosition
      [in] Value specifying the meaning of uItem. If this
      parameter is FALSE, uItem is a menu item identifier.
      Otherwise, it is a menu item position.
    lpmii
      [in] Pointer to a MENUITEMINFO structure that contains
      information about the menu item and specifies which menu
      item attributes to change.

  Return Values
    If the function succeeds, the return value is nonzero.

    If the function fails, the return value is zero. To get
    extended error information, use the GetLastError function.

* I interpreted the line
  item != NULL ? (UINT) item : (UINT) wv->call_data,
to mean if item is not NULL, use the specified menu identifier, otherwise
use the position specified by the call_data member of the wv structure.

* Based upon this interpretation, passing SetMenuItemInfo a value of FALSE
for the fByPosition parameter unconditionally is obviously incorrect.
Instead fByPosition should only be FALSE if item is non NULL.

Please let me know if my logic behind this fix is flawed.  I am not
absolutely certain that the call_data member of the wv structure is supposed
to the position index if the item in the menu because I have not yet
determined how its value is getting set.

I have tested this fix by successfully running a build of Emacs done on a
Windows XP machine using MSVC 6.0 on Windows 2000.

NOTE: I have no idea why this crash only occurred when running Emacs on a
version of Windows other than the one on which it was built.  All I know is
that when I remote debugged Emacs to determine why I could not run the build
I did on Windows XP on Windows 2000, Emacs crashed on this function call and
item at that point was NULL.


[-- Attachment #2: 21_3-w32menu.c.diff --]
[-- Type: application/octet-stream, Size: 542 bytes --]

--- _21.3/src/w32menu.c	2002-08-05 12:33:44.000000000 -0400
+++ 21.3/src/w32menu.c	2002-11-06 23:01:11.000000000 -0500
@@ -2215,9 +2215,11 @@
 	      info.fState = wv->selected ? MFS_CHECKED : MFS_UNCHECKED;
 	    }
 
-	  set_menu_item_info (menu,
-			      item != NULL ? (UINT) item : (UINT) wv->call_data,
-			      FALSE, &info);
+    set_menu_item_info (
+        menu,
+        item != NULL ? (UINT) item : (UINT) wv->call_data,
+        item != NULL ? FALSE : TRUE,
+        &info);
 	}
     }
   return return_value;

Re: Fix for Emacs Crash

[Jason Rumney]

"Ben Key" <Bkey1@tampabay.rr.com> writes:

> +    set_menu_item_info (
> +        menu,
> +        item != NULL ? (UINT) item : (UINT) wv->call_data,
> +        item != NULL ? FALSE : TRUE,
> +        &info);

> * I interpreted the line
>   item != NULL ? (UINT) item : (UINT) wv->call_data,
> to mean if item is not NULL, use the specified menu identifier, otherwise
> use the position specified by the call_data member of the wv structure.

I don't think call_data specifies a position, but it may be by chance
that things work correctly in many cases where item == NULL by
assuming it does.

You will have to figure out what wv->call_data and item represent by
studying the rest of the code. My memory is sketchy, but I think that
item can be NULL for menu titles and separator lines. In the title
case, wv->call_data might be NULL as well, so your modified code does
the right thing even though the assumptions behind it are wrong,
since titles are at position 0. But I am not sure what happens in the
separator case.

> * Based upon this interpretation, passing SetMenuItemInfo a value of FALSE
> for the fByPosition parameter unconditionally is obviously incorrect.
> Instead fByPosition should only be FALSE if item is non NULL.

I think this part of your interpretation is correct.

Thanks for tracking this down.

Re: Fix for Emacs Crash

[Stefan Monnier]

> > +        item != NULL ? FALSE : TRUE,

Aka          item == NULL

and in most cases, this is also equivalent to just `item'.


	Stefan

Re: Fix for Emacs Crash

[bkey1]

Jason Rumney [mailto:jasonr@gnu.org] wrote:

> > +    set_menu_item_info (
> > +        menu,
> > +        item != NULL ? (UINT) item : (UINT) wv->call_data,
> > +        item != NULL ? FALSE : TRUE,
> > +        &info);

> > * I interpreted the line
> >   item != NULL ? (UINT) item : (UINT) wv->call_data,
> > to mean if item is not NULL, use the specified menu identifier, otherwise
> > use the position specified by the call_data member of the wv structure.
> 
> I don't think call_data specifies a position, but it may be by chance
> that things work correctly in many cases where item == NULL by
> assuming it does.
> 
> You will have to figure out what wv->call_data and item represent by
> studying the rest of the code. My memory is sketchy, but I think that
> item can be NULL for menu titles and separator lines. In the title
> case, wv->call_data might be NULL as well, so your modified code does
> the right thing even though the assumptions behind it are wrong,
> since titles are at position 0. But I am not sure what happens in the
> separator case.

The line
  item != NULL ? (UINT) item : (UINT) wv->call_data,
was part of the original code.  That is what lead me to come to the conclusions 
I made about the role of wv->call_data.

However, I will do some further debugging tonight to determine if these 
conclusions are correct.