Bug Patch: Memory Leak in iT2D Scripting (Fixes ITGB-205)

Phew! My game doesn't crash (much)! Yippee!

-----

Code Base: iT2D 1.5 (should work for others as well)

Bug:

Each run of a script may lead to a memory leak. Therefore, if you are running lots of scripts, you may gets tons of leaked memory. For example, my simple scene onUpdate was causing me approximately 30K/second of leaks. This would crash my game within a minute or two on the iPad.

(If you are not seeing a crash on the iOS simulator, remember that it is not limited to device-sized amounts of memory, but instead can use as much OSX memory as it wants. An iPad 1 has 128MB of memory and I believe that includes VRAM.)

The reason I mention that it "may" lead to memory leaks is that I have turned off PUAP_SCRIPT_CHANGE. This new feature was, I believe, to run your scripts faster in most cases. However in my case it had a TorqueScript stack corruption that lead to other problems. So I turned it off.

However, while fixing the bug, it seems to me there should be a very potential leak even for those with PUAP_SCRIPT_CHANGE still on, which it is by default. While I did not put in the time to test when this feature is on, if you see leaks, the patch should help, but read after the patch for more details!

Details:

When PUAP_SCRIPT_CHANGE it turned off, there is no attempt to delete the "tHashTable" that a "Dictionary" creates on each function call. To fix this, one must recognize that sometimes the hash table is shared between two or more Dictionaries, so it isn't enough just to call delete on the hash table. The patch below adds a variable to know when Dictionaries are shared and to delete only when appropriate.

Further, when a Dictionary deletes its tHashTable it must first delete all the Entries it put in the table. This does not appear to be done with PUAP_SCRIPT_CHANGE on either, so I'm surprised there aren't memory leaks for those with the changes on. Someone else will have to verify this.

Patch:

Files Affected:
Dictionary.cc
Dictionary.h

Included:
included in-line as a "unified diff" meaning it has context in it so that line-numbers aren't as important. Run this patch, using your favorite patch tool, from the engine/source/console directory.

Index: Dictionary.cc
===================================================================
--- Dictionary.cc	(revision 664)
+++ Dictionary.cc	(revision 689)
@@ -209,6 +209,7 @@
       ip( 0 )
 {
 	mHashTable = new tHashTable<StringTableEntry,Dictionary::Entry*>;
+	mHashTableRef = false;
 	recursiveDictionary = false;
 }
 
@@ -217,9 +218,9 @@
       scopeName( NULL ),
       scopeNamespace( NULL ),
       code( NULL ),
-      ip( 0 )
+      ip( 0 ),
+	  mHashTableRef(false)
 {
-	mHashTable = new tHashTable<StringTableEntry,Dictionary::Entry*>;
    setState(state,ref);
    recursiveDictionary = false;
 }
@@ -228,16 +229,19 @@
 {
    exprState = state;
 
+	if ( !mHashTableRef && mHashTable )
+		delete mHashTable;
+
    //copy hashTable from master
-   if (ref)
+   if (ref) {
       mHashTable = ref->mHashTable;
+	  mHashTableRef = true;
+   }
    else
    {
 	   //make a new one, but no memory leaks
-	   if( mHashTable ) {
-		   delete mHashTable;
-	   }
 	   mHashTable = new tHashTable<StringTableEntry,Dictionary::Entry*>;
+	   mHashTableRef = false;
    }
 }
 
@@ -245,16 +249,27 @@
 {
 #ifdef PUAP_SCRIPT_CHANGE
    delete mHashTable;
+#else
+	// if we are referencing another Dictionary's entries, we have nothing to do
+	if (mHashTableRef)
+		return;
+
+	// delete Entry objects stored in the hash table, then delete the hash table
+	for( entryIterator it = mHashTable->begin(); it != mHashTable->end(); it++ )
+		delete it->value;
+
+	delete mHashTable;
 #endif
 }
 
 void Dictionary::reset()
 {
    //make a new one, but no memory leaks
-   if( mHashTable ) {
+	if ( !mHashTableRef && mHashTable )
 	   delete mHashTable;
-   }
-   mHashTable = new tHashTable<StringTableEntry,Dictionary::Entry*>;
+
+    mHashTable = new tHashTable<StringTableEntry,Dictionary::Entry*>;
+	mHashTableRef = false;
 }
 
 
Index: Dictionary.h
===================================================================
--- Dictionary.h	(revision 664)
+++ Dictionary.h	(revision 689)
@@ -119,6 +119,7 @@
 public:
 	typedef tHashTable<StringTableEntry, Dictionary::Entry *>::Iterator entryIterator;
 	tHashTable<StringTableEntry, Dictionary::Entry *> *mHashTable;
+	bool mHashTableRef;
 
     StringTableEntry scopeName;
     Namespace *scopeNamespace;

Caveats!
If you have also turned off the PUAP_SCRIPT_CHANGE feature, this patch should work.

If you still have on the PUAP_SCRIPT_CHANGE feature, I'm surprised that it works at all. :P However, if you are also getting memory leaks with it on, I'm guessing it is the Dictionary Entries not being deleted. You may want to apply theh patch but also change the Dictionary destructor above. Try removing the ifdef statements and always run the code that deletes the Entries before deleting the mHashTable.

In other words, if you have PUAP_SCRIPT_CHANGE on you may want to change your code form this:

Dictionary::~Dictionary()
{
#ifdef PUAP_SCRIPT_CHANGE
   delete mHashTable;
#else
	// if we are referencing another Dictionary's entries, we have nothing to do
	if (mHashTableRef)
		return;

	// delete Entry objects stored in the hash table, then delete the hash table
	for( entryIterator it = mHashTable->begin(); it != mHashTable->end(); it++ )
		delete it->value;

	delete mHashTable;
#endif
}

to this:

Dictionary::~Dictionary()
{
	// if we are referencing another Dictionary's entries, we have nothing to do
	if (mHashTableRef)
		return;

	// delete Entry objects stored in the hash table, then delete the hash table
	for( entryIterator it = mHashTable->begin(); it != mHashTable->end(); it++ )
		delete it->value;

	delete mHashTable;
}