[[ Bug 21928 ]] Fix memory leak when using menu buttons

runrevmark · runrevmark · commit 0a4b9455c8d9 · 2019-03-26T14:15:56.000Z
This patch fixes a memory leak when using menu buttons caused by
failing to delete the internal stack created to enable them to
function. The leak came about when object deletion was being
abstracted, and the code was being refactored to use object handles.
The button's menu pointer was changed to an object handle and
the explicit deletion which was there previously removed. As
object handles are weak references, this meant that they were
never being deleted.

The leak has been fixed by ensuring that the dodel() method can
be run on menus-as-stacks (which required an extra check in the
MCStack::removereferences method) and then additionally adding
an explicit delete operator call on the button's menu-as-stack
in MCButton::freemenu().
diff --git a/docs/notes/bugfix-21928.md b/docs/notes/bugfix-21928.md
@@ -0,0 +1 @@
+# Fix memory leak when using menu buttons
diff --git a/engine/src/button.cpp b/engine/src/button.cpp
@@ -2860,11 +2860,23 @@ void MCButton::freemenu(Boolean force)
 		else
 		{
 			if (!MCStringIsEmpty(menustring) || force)
-			{
-				closemenu(False, True);
-				MCdispatcher->removepanel(menu);
+            {
+                closemenu(False, True);
+                
+                /* In this case the button owns the menu so after removing
+                 * any references to it that might exist in the environment
+                 * it must be explicitly deleted. */
+                MCStack *t_menu_ptr = menu.Get();
+				MCdispatcher->removepanel(t_menu_ptr);
 				MCstacks->deleteaccelerator(this, NULL);
-				menu->removeneed(this);
+				t_menu_ptr->removeneed(this);
+                t_menu_ptr->dodel();
+                
+                /* It is safe to explicitly delete the menu stack here (rather
+                 * than 'scheduledelete()') as menu stacks and their contents
+                 * can never be on C stack as they don't contain script. */
+                delete t_menu_ptr;
+                
 				menu = nil;
 			}
 		}
diff --git a/engine/src/stack.cpp b/engine/src/stack.cpp
@@ -1476,6 +1476,13 @@ void MCStack::removereferences()
     {
         MCdispatcher->remove_transient_stack(this);
     }
+    else if (!parent.IsValid() || parent->gettype() == CT_BUTTON)
+    {
+        /* If the parent is a button or non-existant then this is a
+         * menu-as-stack - registered with MCdispatcher as a panel. This code
+         * path is hit when performing dodel() on a menu-as-stack before it
+         * is explicitly deleted in MCButton::freemenu. */
+    }
     else if (parent->gettype() == CT_STACK)
     {
         remove(parent.GetAs<MCStack>()->substacks);

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+# Fix memory leak when using menu buttons`
Original file line number	Diff line number	Diff line change
`@@ -1476,6 +1476,13 @@ void MCStack::removereferences()`
`1476`	`1476`	`{`
`1477`	`1477`	`MCdispatcher->remove_transient_stack(this);`
`1478`	`1478`	`}`
	`1479`	`+ else if (!parent.IsValid() \|\| parent->gettype() == CT_BUTTON)`
	`1480`	`+ {`
	`1481`	`+ /* If the parent is a button or non-existant then this is a`
	`1482`	`+ * menu-as-stack - registered with MCdispatcher as a panel. This code`
	`1483`	`+ * path is hit when performing dodel() on a menu-as-stack before it`
	`1484`	`+ * is explicitly deleted in MCButton::freemenu. */`
	`1485`	`+ }`
`1479`	`1486`	`else if (parent->gettype() == CT_STACK)`
`1480`	`1487`	`{`
`1481`	`1488`	`remove(parent.GetAs<MCStack>()->substacks);`