Merge pull request livecode#6926 from runrevmark/bugfix-21787

livecodepanos · web-flow · commit f1287f01d165 · 2019-03-27T23:44:46.000+02:00
[[ Bug 21787 ]] Fix memory leak when implicitly deleting substacks
diff --git a/docs/notes/bugfix-21787.md b/docs/notes/bugfix-21787.md
@@ -0,0 +1 @@
+# Fix memory leak which occurs when substacks are deleted implicitly
diff --git a/engine/src/dispatch.cpp b/engine/src/dispatch.cpp
@@ -367,14 +367,32 @@ void MCDispatch::removestack(MCStack *sptr)
 
 void MCDispatch::destroystack(MCStack *sptr, Boolean needremove)
 {
+    /* Make sure no messages are sent when destroying the given stack as this
+     * destruction method is only ever used for stacks which are not-yet-alive
+     * (e.g. failed to deserialize) or in restricted contexts (e.g. licensing
+     * dialog on startup). */
+    Boolean oldstate = MClockmessages;
+    MClockmessages = True;
+    
 	if (needremove)
     {
         MCStack *t_substacks = sptr -> getsubstacks();
-        while (t_substacks)
+        while(t_substacks != nullptr)
         {
+            /* The MCStack::dodel() method removes the stack from its mainstack 
+             * so we must explicitly delete it explicitly. Note that there is
+             * no need to scheduledelete() in this case as destroystack() is
+             * only called when it is known that no script is running from the
+             * stack. */
             t_substacks -> dodel();
+            delete t_substacks;
+            
+            /* Refetch the substacks list - the substack we just processed will
+             * have been removed from it. */
             t_substacks = sptr -> getsubstacks();
         }
+        
+        /* Release any references to the mainstack */
         sptr -> dodel();
     }
 	if (sptr == MCstaticdefaultstackptr)
@@ -383,9 +401,13 @@ void MCDispatch::destroystack(MCStack *sptr, Boolean needremove)
 		MCdefaultstackptr = MCstaticdefaultstackptr;
 	if (MCacptr && MCacptr->getmessagestack() == sptr)
 		MCacptr->setmessagestack(NULL);
-	Boolean oldstate = MClockmessages;
-	MClockmessages = True;
+    
+    /* Delete the stack explicitly. Note that there is no need to use
+     * scheduledelete here as destroystack() is only called when it is known
+     * that no script is running from the stack. */
 	delete sptr;
+    
+    /* Restore the previous message lock state. */
 	MClockmessages = oldstate;
 }
 
diff --git a/engine/src/stack.cpp b/engine/src/stack.cpp
@@ -1419,8 +1419,17 @@ Boolean MCStack::del(bool p_check_flag)
 
     while (substacks)
     {
+        /* When a substack is deleted it removes itself from its mainstack,
+         * however it isn't actually destroyed - it must be explicitly
+         * scheduled for deletion. */
+        MCStack *t_substack = substacks;
         if (!substacks -> del(false))
             return False;
+        
+        /* Schedule the substack for deletion - unlike a main stack we don't
+         * need to check for it being in the MCtodestroy list as only mainstacks
+         * can ever be in that list. */
+        t_substack->scheduledelete();
     }
     
     return dodel();

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+# Fix memory leak which occurs when substacks are deleted implicitly`