menu clone cleanup bug fix

5 files changed, 175 insertions, 73 deletions

diff --git a/ChangeLog b/ChangeLog
index 4377027..4c05741 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2003-07-15  Vince Darley <vincentdarley@users.sourceforge.net>
+
+	* generic/tkMenu.c
+	* generic/tkMenu.h
+	* generic/tkMenuDraw.c: 
+	* tests/menu.test: fixed complex bug in menu clone cleanup
+	[Bug#465324] and removed 'knownBug' from a test.
+        (This was applied to cvs head on 2003-05-03).
+	
 2003-07-09  Donal K. Fellows  <fellowsd@cs.man.ac.uk>
 
 	* tests/send.test: Strengthened constraints to stop accidental
diff --git a/generic/tkMenu.c b/generic/tkMenu.c
index 152e57f..3e4c54a 100644
--- a/generic/tkMenu.c
+++ b/generic/tkMenu.c
@@ -12,7 +12,7 @@
  * See the file "license.terms" for information on usage and redistribution
  * of this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tkMenu.c,v 1.20 2003/02/26 02:32:56 hobbs Exp $
+ * RCS: @(#) $Id: tkMenu.c,v 1.20.2.1 2003/07/15 13:59:06 vincentdarley Exp $
  */
 
 /*
@@ -496,7 +496,9 @@ MenuCmd(clientData, interp, objc, objv)
     }
 
     /*
-     * Initialize the data structure for the menu.
+     * Initialize the data structure for the menu.  Note that the
+     * menuPtr is eventually freed in 'TkMenuEventProc' in tkMenuDraw.c,
+     * when Tcl_EventuallyFree is called.
      */
 
     menuPtr = (TkMenu *) ckalloc(sizeof(TkMenu));
@@ -1186,7 +1188,9 @@ DestroyMenuInstance(menuPtr)
     TkpDestroyMenu(menuPtr);
     cascadePtr = menuPtr->menuRefPtr->parentEntryPtr;
     menuPtr->menuRefPtr->menuPtr = NULL;
-    TkFreeMenuReferences(menuPtr->menuRefPtr);
+    if (TkFreeMenuReferences(menuPtr->menuRefPtr)) {
+	menuPtr->menuRefPtr = NULL;
+    }
 
     for (; cascadePtr != NULL; cascadePtr = nextCascadePtr) {
     	nextCascadePtr = cascadePtr->nextCascadePtr;
@@ -1252,6 +1256,11 @@ DestroyMenuInstance(menuPtr)
     TkMenuFreeDrawOptions(menuPtr);
     Tk_FreeConfigOptions((char *) menuPtr, 
 	    menuPtr->optionTablesPtr->menuOptionTable, menuPtr->tkwin);
+    if (menuPtr->tkwin != NULL) {
+	Tk_Window tkwin = menuPtr->tkwin;
+	menuPtr->tkwin = NULL;
+	Tk_DestroyWindow(tkwin);
+    }
 }
 
 /*
@@ -1285,6 +1294,8 @@ TkDestroyMenu(menuPtr)
     	return;
     }
 
+    Tcl_Preserve(menuPtr);
+    
     /*
      * Now destroy all non-tearoff instances of this menu if this is a 
      * parent menu. Is this loop safe enough? Are there going to be
@@ -1292,30 +1303,38 @@ TkDestroyMenu(menuPtr)
      * we have to do a slightly more complex scheme.
      */
 
+    menuPtr->menuFlags |= MENU_DELETION_PENDING;
+    if (menuPtr->menuRefPtr != NULL) {
+	/*
+	 * If any toplevel widgets have this menu as their menubar,
+	 * the geometry of the window may have to be recalculated.
+	 */
+	topLevelListPtr = menuPtr->menuRefPtr->topLevelListPtr;
+	while (topLevelListPtr != NULL) {
+	    nextTopLevelPtr = topLevelListPtr->nextPtr;
+	    TkpSetWindowMenuBar(topLevelListPtr->tkwin, NULL);
+	    topLevelListPtr = nextTopLevelPtr;
+	}
+    }
     if (menuPtr->masterMenuPtr == menuPtr) {
-    	menuPtr->menuFlags |= MENU_DELETION_PENDING;
 	while (menuPtr->nextInstancePtr != NULL) {
 	    menuInstancePtr = menuPtr->nextInstancePtr;
 	    menuPtr->nextInstancePtr = menuInstancePtr->nextInstancePtr;
     	    if (menuInstancePtr->tkwin != NULL) {
-	     	Tk_DestroyWindow(menuInstancePtr->tkwin);
+		Tk_Window tkwin = menuInstancePtr->tkwin;
+		/* 
+		 * Note: it may be desirable to NULL out the tkwin
+		 * field of menuInstancePtr here:
+		 * menuInstancePtr->tkwin = NULL;
+		 */
+	     	Tk_DestroyWindow(tkwin);
 	    }
 	}
-    	menuPtr->menuFlags &= ~MENU_DELETION_PENDING;
     }
 
-    /*
-     * If any toplevel widgets have this menu as their menubar,
-     * the geometry of the window may have to be recalculated.
-     */
-
-    topLevelListPtr = menuPtr->menuRefPtr->topLevelListPtr;
-    while (topLevelListPtr != NULL) {
-         nextTopLevelPtr = topLevelListPtr->nextPtr;
-         TkpSetWindowMenuBar(topLevelListPtr->tkwin, NULL);
-    	 topLevelListPtr = nextTopLevelPtr;
-    }
     DestroyMenuInstance(menuPtr);
+
+    Tcl_Release(menuPtr);
 }
 
 /*
@@ -1326,6 +1345,10 @@ TkDestroyMenu(menuPtr)
  *	This entry is removed from the list of entries that point to the
  *	cascade menu. This is done in preparation for changing the menu
  *	that this entry points to.
+ *	
+ *	At the end of this function, the menu entry no longer contains
+ *	a reference to a 'TkMenuReferences' structure, and therefore
+ *	no such structure contains a reference to this menu entry either.
  *
  * Results:
  *	None
@@ -1352,6 +1375,8 @@ UnhookCascadeEntry(mePtr)
     
     cascadeEntryPtr = menuRefPtr->parentEntryPtr;
     if (cascadeEntryPtr == NULL) {
+	TkFreeMenuReferences(menuRefPtr);
+	mePtr->childMenuRefPtr = NULL;
     	return;
     }
     
@@ -1370,6 +1395,10 @@ UnhookCascadeEntry(mePtr)
 	     */
 	
 	    menuRefPtr->parentEntryPtr = NULL;
+	    /* 
+	     * The original field is set to zero below, after it is
+	     * freed.
+	     */
 	    TkFreeMenuReferences(menuRefPtr);
     	} else {
     	    menuRefPtr->parentEntryPtr = cascadeEntryPtr->nextCascadePtr;
@@ -1388,6 +1417,7 @@ UnhookCascadeEntry(mePtr)
     	    	break;
     	    }
         }
+	mePtr->nextCascadePtr = NULL;
     }
     mePtr->childMenuRefPtr = NULL;
 }
@@ -1435,7 +1465,40 @@ DestroyMenuEntry(memPtr)
      */
 
     if (mePtr->type == CASCADE_ENTRY) {
-        UnhookCascadeEntry(mePtr);
+	if (menuPtr->masterMenuPtr != menuPtr) {
+	    TkMenu *destroyThis = NULL;
+	    /* 
+	     * The menu as a whole is a clone.  We must delete the clone
+	     * of the cascaded menu for the particular entry we are
+	     * destroying.
+	     */
+	    TkMenuReferences *menuRefPtr = mePtr->childMenuRefPtr;
+	    if (menuRefPtr != NULL) {
+		destroyThis = menuRefPtr->menuPtr;
+		/* 
+		 * But only if it is a clone.  What can happen is that
+		 * we are in the middle of deleting a menu and this
+		 * menu pointer has already been reset to point to the
+		 * original menu.  In that case we have nothing special
+		 * to do.
+		 */
+		if ((destroyThis != NULL) 
+		  && (destroyThis->masterMenuPtr == destroyThis)) {
+		    destroyThis = NULL;
+		}
+	    }
+	    UnhookCascadeEntry(mePtr);
+	    if (menuRefPtr != NULL) {
+	        if (menuRefPtr->menuPtr == destroyThis) {
+	            menuRefPtr->menuPtr = NULL;
+	        }
+		if (destroyThis != NULL) {
+		    TkDestroyMenu(destroyThis);
+		}
+	    }
+	} else {
+	    UnhookCascadeEntry(mePtr);
+	}
     }
     if (mePtr->image != NULL) {
 	Tk_FreeImage(mePtr->image);
@@ -2198,6 +2261,11 @@ MenuCmdDeletedProc(clientData)
      */
 
     if (tkwin != NULL) {
+	/* 
+	 * Note: it may be desirable to NULL out the tkwin
+	 * field of menuPtr here:
+	 * menuPtr->tkwin = NULL;
+	 */
 	Tk_DestroyWindow(tkwin);
     }
 }
@@ -2935,6 +3003,13 @@ RecursivelyDeleteMenu(menuPtr)
     int i;
     TkMenuEntry *mePtr;
     
+    /* 
+     * It is not 100% clear that this preserve/release pair is
+     * required, but we have added them for safety in this
+     * very complex code.
+     */
+    Tcl_Preserve(menuPtr);
+    
     for (i = 0; i < menuPtr->numEntries; i++) {
     	mePtr = menuPtr->entries[i];
     	if ((mePtr->type == CASCADE_ENTRY)
@@ -2943,7 +3018,11 @@ RecursivelyDeleteMenu(menuPtr)
     	    RecursivelyDeleteMenu(mePtr->childMenuRefPtr->menuPtr);
     	}
     }
-    Tk_DestroyWindow(menuPtr->tkwin);
+    if (menuPtr->tkwin != NULL) {
+	Tk_DestroyWindow(menuPtr->tkwin);
+    }
+    
+    Tcl_Release(menuPtr);
 }
 
 /*
@@ -3095,18 +3174,14 @@ TkSetWindowMenuBar(interp, tkwin, oldMenuName, menuName)
 	     * that reference this menu.
  	     */
  
-            for (topLevelListPtr = menuRefPtr->topLevelListPtr,
-		    prevTopLevelPtr = NULL;
-		    (topLevelListPtr != NULL) 
-            	    && (topLevelListPtr->tkwin != tkwin);
-		    prevTopLevelPtr = topLevelListPtr,
-		    topLevelListPtr = topLevelListPtr->nextPtr) {
-
-		/*
-		 * Empty loop body.
-		 */
-		
-            }
+	    topLevelListPtr = menuRefPtr->topLevelListPtr;
+	    prevTopLevelPtr = NULL;
+	    
+            while ((topLevelListPtr != NULL) 
+		   && (topLevelListPtr->tkwin != tkwin)) {
+		prevTopLevelPtr = topLevelListPtr;
+		topLevelListPtr = topLevelListPtr->nextPtr;
+	    }
 
 	    /*
 	     * Now we have found the toplevel reference that matches the
@@ -3377,7 +3452,8 @@ TkFindMenuReferencesObj(interp, objPtr)
  *	is cleared. It cleans up the ref if it is now empty.
  *
  * Results:
- *	None.
+ *	Returns 1 if the references structure was freed, and 0 
+ *	otherwise.
  *
  * Side effects:
  *	If this is the last field to be cleared, the menu ref is
@@ -3386,7 +3462,7 @@ TkFindMenuReferencesObj(interp, objPtr)
  *----------------------------------------------------------------------
  */
 
-void
+int
 TkFreeMenuReferences(menuRefPtr)
     TkMenuReferences *menuRefPtr;		/* The menu reference to
 						 * free */
@@ -3396,7 +3472,9 @@ TkFreeMenuReferences(menuRefPtr)
     	    && (menuRefPtr->topLevelListPtr == NULL)) {
     	Tcl_DeleteHashEntry(menuRefPtr->hashEntryPtr);
     	ckfree((char *) menuRefPtr);
+	return 1;
     }
+    return 0;
 }
 
 /*
@@ -3453,28 +3531,28 @@ DeleteMenuCloneEntries(menuPtr, first, last)
     }
 }
 
-/*
- *----------------------------------------------------------------------
- *
- * TkMenuCleanup --
- *
- *	Resets menusInitialized to allow Tk to be finalized and reused
- *	without the DLL being unloaded.
- *
- * Results:
- *	None.
- *
- * Side effects:
- *	None.
- *
- *----------------------------------------------------------------------
- */
-
-static void
-TkMenuCleanup(ClientData unused)
-{
-    menusInitialized = 0;
-}
+/* 
+ *---------------------------------------------------------------------- 
+ * 
+ * TkMenuCleanup -- 
+ * 
+ *      Resets menusInitialized to allow Tk to be finalized and reused 
+ *      without the DLL being unloaded. 
+ * 
+ * Results: 
+ *      None. 
+ * 
+ * Side effects: 
+ *      None. 
+ * 
+ *---------------------------------------------------------------------- 
+ */ 
+
+static void 
+TkMenuCleanup(ClientData unused) 
+{ 
+    menusInitialized = 0; 
+} 
 
 /*
  *----------------------------------------------------------------------
@@ -3498,17 +3576,17 @@ TkMenuInit()
 {
     ThreadSpecificData *tsdPtr = (ThreadSpecificData *) 
 	    Tcl_GetThreadData(&dataKey, sizeof(ThreadSpecificData));
-
+    
     if (!menusInitialized) {
 	Tcl_MutexLock(&menuMutex);
 	if (!menusInitialized) {
 	    TkpMenuInit();
 	    menusInitialized = 1;
 	}
-	/*
-	 * Make sure we cleanup on finalize.
-	 */
-	Tcl_CreateExitHandler((Tcl_ExitProc *) TkMenuCleanup, NULL);
+	/* 
+	 * Make sure we cleanup on finalize. 
+	 */ 
+	Tcl_CreateExitHandler((Tcl_ExitProc *) TkMenuCleanup, NULL); 
 	Tcl_MutexUnlock(&menuMutex);
     }
     if (!tsdPtr->menusInitialized) {
diff --git a/generic/tkMenu.h b/generic/tkMenu.h
index 9553bc0..f007bce 100644
--- a/generic/tkMenu.h
+++ b/generic/tkMenu.h
@@ -8,7 +8,7 @@
  * See the file "license.terms" for information on usage and redistribution
  * of this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tkMenu.h,v 1.6 2001/10/12 13:30:31 tmh Exp $
+ * RCS: @(#) $Id: tkMenu.h,v 1.6.4.1 2003/07/15 13:59:06 vincentdarley Exp $
  */
 
 #ifndef _TKMENU
@@ -451,17 +451,21 @@ typedef struct TkMenuOptionTables {
  * RESIZE_PENDING:		Non-zero means a call to ComputeMenuGeometry
  *				has already been scheduled.
  * MENU_DELETION_PENDING	Non-zero means that we are currently destroying
- *				this menu. This is useful when we are in the 
- *				middle of cleaning this master menu's chain of 
- *				menus up when TkDestroyMenu was called again on
- *				this menu (via a destroy binding or somesuch).
+ *				this menu's internal structures. This is useful 
+ *				when we are in the middle of cleaning
+ *				this master menu's chain of menus up when
+ *				TkDestroyMenu was called again on this
+ *				menu (via a destroy binding or somesuch).
+ * MENU_WIN_DESTRUCTION_PENDING Non-zero means we are in the middle of 
+ *                              destroying this menu's Tk_Window.
  * MENU_PLATFORM_FLAG1...	Reserved for use by the platform-specific menu
  *				code.
  */
 
-#define REDRAW_PENDING		1
-#define RESIZE_PENDING		2
-#define MENU_DELETION_PENDING	4
+#define REDRAW_PENDING		        1
+#define RESIZE_PENDING		        2
+#define MENU_DELETION_PENDING	        4
+#define MENU_WIN_DESTRUCTION_PENDING	8
 #define MENU_PLATFORM_FLAG1	(1 << 30)
 #define MENU_PLATFORM_FLAG2	(1 << 29)
 #define MENU_PLATFORM_FLAG3	(1 << 28)
@@ -511,7 +515,7 @@ EXTERN TkMenuReferences *
 EXTERN TkMenuReferences *
 			TkFindMenuReferencesObj _ANSI_ARGS_((
 			    Tcl_Interp *interp, Tcl_Obj *namePtr));
-EXTERN void		TkFreeMenuReferences _ANSI_ARGS_((
+EXTERN int		TkFreeMenuReferences _ANSI_ARGS_((
 			    TkMenuReferences *menuRefPtr));
 EXTERN Tcl_HashTable *	TkGetMenuHashTable _ANSI_ARGS_((Tcl_Interp *interp));
 EXTERN int		TkGetMenuIndex _ANSI_ARGS_((Tcl_Interp *interp,
diff --git a/generic/tkMenuDraw.c b/generic/tkMenuDraw.c
index 42cdf43..ee1e89e 100644
--- a/generic/tkMenuDraw.c
+++ b/generic/tkMenuDraw.c
@@ -9,7 +9,7 @@
  * See the file "license.terms" for information on usage and redistribution
  * of this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tkMenuDraw.c,v 1.3 1999/04/16 01:51:19 stanton Exp $
+ * RCS: @(#) $Id: tkMenuDraw.c,v 1.3.20.1 2003/07/15 13:59:06 vincentdarley Exp $
  */
 
 #include "tkMenu.h"
@@ -769,15 +769,26 @@ TkMenuEventProc(clientData, eventPtr)
 	}
     } else if (eventPtr->type == DestroyNotify) {
 	if (menuPtr->tkwin != NULL) {
-	    TkDestroyMenu(menuPtr);
+	    if (!(menuPtr->menuFlags & MENU_DELETION_PENDING)) {
+		TkDestroyMenu(menuPtr);
+	    }
 	    menuPtr->tkwin = NULL;
+	}
+	if (menuPtr->menuFlags & MENU_WIN_DESTRUCTION_PENDING) {
+	    return;
+	}
+	menuPtr->menuFlags |= MENU_WIN_DESTRUCTION_PENDING;
+	if (menuPtr->widgetCmd != NULL) {
 	    Tcl_DeleteCommandFromToken(menuPtr->interp, menuPtr->widgetCmd);
+	    menuPtr->widgetCmd = NULL;
 	}
 	if (menuPtr->menuFlags & REDRAW_PENDING) {
 	    Tcl_CancelIdleCall(DisplayMenu, (ClientData) menuPtr);
+	    menuPtr->menuFlags &= ~REDRAW_PENDING;
 	}
 	if (menuPtr->menuFlags & RESIZE_PENDING) {
 	    Tcl_CancelIdleCall(ComputeMenuGeometry, (ClientData) menuPtr);
+	    menuPtr->menuFlags &= ~RESIZE_PENDING;
 	}
 	Tcl_EventuallyFree((ClientData) menuPtr, TCL_DYNAMIC);
     }
diff --git a/tests/menu.test b/tests/menu.test
index a9652bf..ed60a38 100644
--- a/tests/menu.test
+++ b/tests/menu.test
@@ -5,7 +5,7 @@
 # Copyright (c) 1998-1999 by Scriptics Corporation.
 # All rights reserved.
 #
-# RCS: @(#) $Id: menu.test,v 1.13 2002/07/14 05:48:46 dgp Exp $
+# RCS: @(#) $Id: menu.test,v 1.13.2.1 2003/07/15 13:59:06 vincentdarley Exp $
 
 package require tcltest 2.1
 namespace import -force tcltest::configure
@@ -2423,7 +2423,7 @@ test menu-32.7 {DeleteMenuCloneEntries - one entry} {
     .m1 add command -label Hello
     list [catch {.m1 delete Hello} msg] $msg [destroy .m1]
 } {0 {} {}}
-test menu-32.8 {Ensure all menu clone commands are deleted} {knownBug} {
+test menu-32.8 {Ensure all menu clone commands are deleted} {
     # SF bug #465324
     catch {destroy .menubar}
     catch {destroy .menubar.test}