bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

[YAMAMOTO Mitsuharu]

Currently, xdialog_show in xmenu.c calls
free_menubar_widget_value_tree after calling create_and_show_dialog:

  /* Actually create and show the dialog.  */
  create_and_show_dialog (f, first_wv);

  /* Free the widget_value objects we used to specify the contents.  */
  free_menubar_widget_value_tree (first_wv);

But create_and_show_dialog may longjmp in the following case:

  (let (last-nonmenu-event)
    (y-or-n-p-with-timeout "test" 3 'default))
  ;; wait 3 seconds.

In this case, free_menubar_widget_value_tree is not called and the
memory allocated for `first_wv' is leaked.

The patch below uses record_unwind_protect as usual for avoiding this
memory leak.  I think xmenu_show has the same problem.  I'm not sure
about the W32 case.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

=== modified file 'src/xmenu.c'
*** src/xmenu.c	2011-05-09 11:13:02 +0000
--- src/xmenu.c	2011-10-22 02:14:19 +0000
***************
*** 1658,1663 ****
--- 1658,1674 ----
  
  #endif /* not USE_GTK */
  
+ static Lisp_Object
+ cleanup_widget_value_tree (Lisp_Object arg)
+ {
+   struct Lisp_Save_Value *p = XSAVE_VALUE (arg);
+   widget_value *wv = p->pointer;
+ 
+   free_menubar_widget_value_tree (wv);
+ 
+   return Qnil;
+ }
+ 
  Lisp_Object
  xmenu_show (FRAME_PTR f, int x, int y, int for_click, int keymaps,
  	    Lisp_Object title, char **error, EMACS_UINT timestamp)
***************
*** 1672,1677 ****
--- 1683,1690 ----
  
    int first_pane;
  
+   int specpdl_count = SPECPDL_INDEX ();
+ 
    if (! FRAME_X_P (f))
      abort ();
  
***************
*** 1866,1876 ****
    /* No selection has been chosen yet.  */
    menu_item_selection = 0;
  
    /* Actually create and show the menu until popped down.  */
    create_and_show_popup_menu (f, first_wv, x, y, for_click, timestamp);
  
!   /* Free the widget_value objects we used to specify the contents.  */
!   free_menubar_widget_value_tree (first_wv);
  
    /* Find the selected item, and its pane, to return
       the proper value.  */
--- 1879,1893 ----
    /* No selection has been chosen yet.  */
    menu_item_selection = 0;
  
+   /* Make sure to free the widget_value objects we used to specify the
+      contents even with longjmp.  */
+   record_unwind_protect (cleanup_widget_value_tree,
+ 			 make_save_value (first_wv, 0));
+ 
    /* Actually create and show the menu until popped down.  */
    create_and_show_popup_menu (f, first_wv, x, y, for_click, timestamp);
  
!   unbind_to (specpdl_count, Qnil);
  
    /* Find the selected item, and its pane, to return
       the proper value.  */
***************
*** 2064,2069 ****
--- 2081,2088 ----
    /* 1 means we've seen the boundary between left-hand elts and right-hand.  */
    int boundary_seen = 0;
  
+   int specpdl_count = SPECPDL_INDEX ();
+ 
    if (! FRAME_X_P (f))
      abort ();
  
***************
*** 2177,2187 ****
    /* No selection has been chosen yet.  */
    menu_item_selection = 0;
  
    /* Actually create and show the dialog.  */
    create_and_show_dialog (f, first_wv);
  
!   /* Free the widget_value objects we used to specify the contents.  */
!   free_menubar_widget_value_tree (first_wv);
  
    /* Find the selected item, and its pane, to return
       the proper value.  */
--- 2196,2210 ----
    /* No selection has been chosen yet.  */
    menu_item_selection = 0;
  
+   /* Make sure to free the widget_value objects we used to specify the
+      contents even with longjmp.  */
+   record_unwind_protect (cleanup_widget_value_tree,
+ 			 make_save_value (first_wv, 0));
+ 
    /* Actually create and show the dialog.  */
    create_and_show_dialog (f, first_wv);
  
!   unbind_to (specpdl_count, Qnil);
  
    /* Find the selected item, and its pane, to return
       the proper value.  */



In GNU Emacs 23.3.50.1 (x86_64-apple-darwin10.8.0, GTK+ Version 2.24.6)
 of 2011-10-20 on yamamoto-mitsuharu-no-iMac.local
Windowing system distributor `The X.Org Foundation', version 11.0.10402000
configured using `configure  '--enable-checking' 'LDFLAGS=-L/opt/local/lib' 'CPPFLAGS=-I/opt/local/include' 'CFLAGS=-g''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: ja_JP.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  blink-cursor-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

[Jan Djärv]

Hello.

22 okt 2011 kl. 06:35 skrev YAMAMOTO Mitsuharu:

> Currently, xdialog_show in xmenu.c calls
> free_menubar_widget_value_tree after calling create_and_show_dialog:
> 
>  /* Actually create and show the dialog.  */
>  create_and_show_dialog (f, first_wv);
> 
>  /* Free the widget_value objects we used to specify the contents.  */
>  free_menubar_widget_value_tree (first_wv);
> 
> But create_and_show_dialog may longjmp in the following case:
> 
>  (let (last-nonmenu-event)
>    (y-or-n-p-with-timeout "test" 3 'default))
>  ;; wait 3 seconds.
> 
> In this case, free_menubar_widget_value_tree is not called and the
> memory allocated for `first_wv' is leaked.
> 
> The patch below uses record_unwind_protect as usual for avoiding this
> memory leak.  I think xmenu_show has the same problem.  I'm not sure
> about the W32 case.
> 

You are correct.  Can you install the patch?
As for xmenu_show, is there a case where a menu can be interrupted like this?

	Jan D.


> 				     YAMAMOTO Mitsuharu
> 				mituharu@math.s.chiba-u.ac.jp
> 
> === modified file 'src/xmenu.c'
> *** src/xmenu.c	2011-05-09 11:13:02 +0000
> --- src/xmenu.c	2011-10-22 02:14:19 +0000
> ***************
> *** 1658,1663 ****
> --- 1658,1674 ----
> 
>  #endif /* not USE_GTK */
> 
> + static Lisp_Object
> + cleanup_widget_value_tree (Lisp_Object arg)
> + {
> +   struct Lisp_Save_Value *p = XSAVE_VALUE (arg);
> +   widget_value *wv = p->pointer;
> + 
> +   free_menubar_widget_value_tree (wv);
> + 
> +   return Qnil;
> + }
> + 
>  Lisp_Object
>  xmenu_show (FRAME_PTR f, int x, int y, int for_click, int keymaps,
>  	    Lisp_Object title, char **error, EMACS_UINT timestamp)
> ***************
> *** 1672,1677 ****
> --- 1683,1690 ----
> 
>    int first_pane;
> 
> +   int specpdl_count = SPECPDL_INDEX ();
> + 
>    if (! FRAME_X_P (f))
>      abort ();
> 
> ***************
> *** 1866,1876 ****
>    /* No selection has been chosen yet.  */
>    menu_item_selection = 0;
> 
>    /* Actually create and show the menu until popped down.  */
>    create_and_show_popup_menu (f, first_wv, x, y, for_click, timestamp);
> 
> !   /* Free the widget_value objects we used to specify the contents.  */
> !   free_menubar_widget_value_tree (first_wv);
> 
>    /* Find the selected item, and its pane, to return
>       the proper value.  */
> --- 1879,1893 ----
>    /* No selection has been chosen yet.  */
>    menu_item_selection = 0;
> 
> +   /* Make sure to free the widget_value objects we used to specify the
> +      contents even with longjmp.  */
> +   record_unwind_protect (cleanup_widget_value_tree,
> + 			 make_save_value (first_wv, 0));
> + 
>    /* Actually create and show the menu until popped down.  */
>    create_and_show_popup_menu (f, first_wv, x, y, for_click, timestamp);
> 
> !   unbind_to (specpdl_count, Qnil);
> 
>    /* Find the selected item, and its pane, to return
>       the proper value.  */
> ***************
> *** 2064,2069 ****
> --- 2081,2088 ----
>    /* 1 means we've seen the boundary between left-hand elts and right-hand.  */
>    int boundary_seen = 0;
> 
> +   int specpdl_count = SPECPDL_INDEX ();
> + 
>    if (! FRAME_X_P (f))
>      abort ();
> 
> ***************
> *** 2177,2187 ****
>    /* No selection has been chosen yet.  */
>    menu_item_selection = 0;
> 
>    /* Actually create and show the dialog.  */
>    create_and_show_dialog (f, first_wv);
> 
> !   /* Free the widget_value objects we used to specify the contents.  */
> !   free_menubar_widget_value_tree (first_wv);
> 
>    /* Find the selected item, and its pane, to return
>       the proper value.  */
> --- 2196,2210 ----
>    /* No selection has been chosen yet.  */
>    menu_item_selection = 0;
> 
> +   /* Make sure to free the widget_value objects we used to specify the
> +      contents even with longjmp.  */
> +   record_unwind_protect (cleanup_widget_value_tree,
> + 			 make_save_value (first_wv, 0));
> + 
>    /* Actually create and show the dialog.  */
>    create_and_show_dialog (f, first_wv);
> 
> !   unbind_to (specpdl_count, Qnil);
> 
>    /* Find the selected item, and its pane, to return
>       the proper value.  */
> 
> 
> 
> In GNU Emacs 23.3.50.1 (x86_64-apple-darwin10.8.0, GTK+ Version 2.24.6)
> of 2011-10-20 on yamamoto-mitsuharu-no-iMac.local
> Windowing system distributor `The X.Org Foundation', version 11.0.10402000
> configured using `configure  '--enable-checking' 'LDFLAGS=-L/opt/local/lib' 'CPPFLAGS=-I/opt/local/include' 'CFLAGS=-g''
> 
> Important settings:
>  value of $LC_ALL: nil
>  value of $LC_COLLATE: nil
>  value of $LC_CTYPE: nil
>  value of $LC_MESSAGES: nil
>  value of $LC_MONETARY: nil
>  value of $LC_NUMERIC: nil
>  value of $LC_TIME: nil
>  value of $LANG: ja_JP.UTF-8
>  value of $XMODIFIERS: nil
>  locale-coding-system: utf-8-unix
>  default enable-multibyte-characters: t
> 
> Major mode: Fundamental
> 
> Minor modes in effect:
>  tooltip-mode: t
>  mouse-wheel-mode: t
>  tool-bar-mode: t
>  menu-bar-mode: t
>  file-name-shadow-mode: t
>  blink-cursor-mode: t
>  auto-encryption-mode: t
>  auto-compression-mode: t
>  line-number-mode: t
>  transient-mark-mode: t
> 
> 
> 

bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 30 Oct 2011 18:41:40 +0100, Jan Djärv <jan.h.d@swipnet.se> said:

>> The patch below uses record_unwind_protect as usual for avoiding
>> this memory leak.  I think xmenu_show has the same problem.  I'm
>> not sure about the W32 case.
>> 

> You are correct.  Can you install the patch?

Done.

> As for xmenu_show, is there a case where a menu can be interrupted
> like this?

Yes.  For example,

  (with-timeout (3 'default)
    (x-popup-menu t '("Title" ("foo" "nonselectable"))))

Actually, y-or-n-p-with-timeout is implemented using with-timeout.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

[Jan Djärv]

Hello.

31 okt 2011 kl. 04:21 skrev YAMAMOTO Mitsuharu:

>>>>>> On Sun, 30 Oct 2011 18:41:40 +0100, Jan Djärv <jan.h.d@swipnet.se> said:
> 
>>> The patch below uses record_unwind_protect as usual for avoiding
>>> this memory leak.  I think xmenu_show has the same problem.  I'm
>>> not sure about the W32 case.
>>> 
> 
>> You are correct.  Can you install the patch?
> 
> Done.

I see you checked in this in the emacs-23 tree.  Is that still merged to the trunk?

> 
>> As for xmenu_show, is there a case where a menu can be interrupted
>> like this?
> 
> Yes.  For example,
> 
>  (with-timeout (3 'default)
>    (x-popup-menu t '("Title" ("foo" "nonselectable"))))
> 
> Actually, y-or-n-p-with-timeout is implemented using with-timeout.

I'm waiting for your fix to show up in the trunk before fixing this.  I don't think it makes sense to use the emacs-23 tree anymore.

	Jan D.

bug#9830: 23.3.50; y-or-n-p-with-timeout with GUI leaks memory

[Glenn Morris]

Jan Djärv wrote:

> I'm waiting for your fix to show up in the trunk before fixing this.

I merged it.

>  I don't think it makes sense to use the emacs-23 tree anymore.

I agree (in fact I think this point was reached months ago).