Replace JS_Enumerate with manual enumeration, to avoid its memory allocations.

Use LookupProperty to avoid having to check for getters.
Add a quicker method of rooting many script values.

This was SVN commit r7579.

binaries/data/mods/_test.sim/simulation/components/test-serialize.js

@@ -66,7 +66,7 @@ function TestScript1_getter() {}
 
 TestScript1_getter.prototype.Init = function() {
 	this.x = 100;
-	this.__defineGetter__('x', function () { return 200; });
+	this.__defineGetter__('x', function () { print("FAIL\n"); die(); return 200; });
 };
 
 Engine.RegisterComponentType(IID_Test1, "TestScript1_getter", TestScript1_getter);

source/scriptinterface/AutoRooters.cpp

@@ -0,0 +1,45 @@
+/* Copyright (C) 2010 Wildfire Games.
+ * This file is part of 0 A.D.
+ *
+ * 0 A.D. is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 0 A.D. is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with 0 A.D.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "precompiled.h"
+
+#include "AutoRooters.h"
+
+#include "scriptinterface/ScriptInterface.h"
+
+AutoGCRooter::AutoGCRooter(ScriptInterface& scriptInterface)
+	: m_ScriptInterface(scriptInterface)
+{
+	m_Previous = m_ScriptInterface.ReplaceAutoGCRooter(this);
+}
+
+AutoGCRooter::~AutoGCRooter()
+{
+	AutoGCRooter* r = m_ScriptInterface.ReplaceAutoGCRooter(m_Previous);
+	debug_assert(r == this); // must be correctly nested
+}
+
+void AutoGCRooter::Trace(JSTracer* trc)
+{
+	if (m_Previous)
+		m_Previous->Trace(trc);
+
+	for (size_t i = 0; i < m_Objects.size(); ++i)
+	{
+		JS_CALL_OBJECT_TRACER(trc, m_Objects[i], "AutoGCRooter object");
+	}
+}

source/scriptinterface/AutoRooters.h

@@ -15,8 +15,13 @@
  * along with 0 A.D.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef INCLUDED_AUTOROOTERS
+#define INCLUDED_AUTOROOTERS
+
 #include "js/jsapi.h"
 
+class ScriptInterface;
+
 // Exception-safety and GC-safety wrapper for JSIdArray
 // (TODO: it'd be nicer to use the existing js::AutoIdArray but currently that
 // requires a load of semi-private SpiderMonkey headers that cause a load of compile warnings)
@@ -41,3 +46,27 @@ public:
 	}
 };
 
+/**
+ * Helper for rooting large groups of script values.
+ * Construct this object, push values into it, and they will all be rooted until this
+ * object is destroyed.
+ * Many of these objects can be used at once, but their lifetimes must be correctly nested.
+ */
+class AutoGCRooter
+{
+public:
+	AutoGCRooter(ScriptInterface& scriptInterface);
+	~AutoGCRooter();
+
+	void Push(JSObject* obj) { m_Objects.push_back(obj); }
+
+	void Trace(JSTracer* trc);
+private:
+	ScriptInterface& m_ScriptInterface;
+	AutoGCRooter* m_Previous;
+
+	std::vector<JSObject*> m_Objects;
+	// TODO: add vectors of other value types
+};
+
+#endif // INCLUDED_AUTOROOTERS

source/scriptinterface/ScriptInterface.cpp

@@ -59,6 +59,8 @@ struct ScriptInterface_impl
 	JSContext* m_cx;
 	JSObject* m_glob; // global scope object
 	JSObject* m_nativeScope; // native function scope object
+
+	AutoGCRooter* m_rooter;
 };
 
 namespace
@@ -163,7 +165,16 @@ static void* jshook_function(JSContext* cx, JSStackFrame* fp, JSBool before, JSB
 }
 #endif
 
-ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContext* cx)
+void jshook_trace(JSTracer* trc, void* data)
+{
+	ScriptInterface_impl* m = static_cast<ScriptInterface_impl*>(data);
+
+	if (m->m_rooter)
+		m->m_rooter->Trace(trc);
+}
+
+ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContext* cx) :
+	m_rooter(NULL)
 {
 	JSBool ok;
 
@@ -201,6 +212,8 @@ ScriptInterface_impl::ScriptInterface_impl(const char* nativeScopeName, JSContex
 				| JSOPTION_VAROBJFIX // "recommended" (fixes variable scoping)
 		);
 
+		JS_SetExtraGCRoots(m_rt, jshook_trace, this);
+
 		// Threadsafe SpiderMonkey requires that we have a request before doing anything much
 		JS_BeginRequest(m_cx);
 
@@ -296,6 +309,14 @@ bool ScriptInterface::RemoveRoot(void* ptr)
 	return JS_RemoveRoot(m->m_cx, ptr) ? true : false;
 }
 
+AutoGCRooter* ScriptInterface::ReplaceAutoGCRooter(AutoGCRooter* rooter)
+{
+	debug_assert(m->m_rt); // this class must own the runtime, else the rooter won't work
+	AutoGCRooter* ret = m->m_rooter;
+	m->m_rooter = rooter;
+	return ret;
+}
+
 ScriptInterface::LocalRootScope::LocalRootScope(JSContext* cx) :
 	m_cx(cx)
 {

source/scriptinterface/ScriptInterface.h

@@ -29,6 +29,8 @@
 #include "ScriptTypes.h"
 #include "ScriptVal.h"
 
+class AutoGCRooter;
+
 namespace boost { class rand48; }
 
 // Set the maximum number of function arguments that can be handled
@@ -37,7 +39,6 @@ namespace boost { class rand48; }
 #define SCRIPT_INTERFACE_MAX_ARGS 6
 
 struct ScriptInterface_impl;
-class ScriptClass;
 class ScriptInterface
 {
 public:
@@ -181,6 +182,8 @@ public:
 	bool AddRoot(void* ptr, const char* name);
 	bool RemoveRoot(void* ptr);
 
+	AutoGCRooter* ReplaceAutoGCRooter(AutoGCRooter* rooter);
+
 	/**
 	 * Dump some memory heap debugging information to stderr.
 	 */

source/simulation2/serialization/BinarySerializer.cpp

@@ -29,17 +29,14 @@
 #include "scriptinterface/ScriptInterface.h"
 #include "scriptinterface/AutoRooters.h"
 
+#include "js/jsobj.h"
+
 CBinarySerializer::CBinarySerializer(ScriptInterface& scriptInterface) :
-	m_ScriptInterface(scriptInterface)
+	m_ScriptInterface(scriptInterface), m_ScriptBackrefsNext(1), m_Rooter(scriptInterface)
 {
 
 }
 
-CBinarySerializer::~CBinarySerializer()
-{
-	FreeScriptBackrefs();
-}
-
 /*
 
 The Put* implementations here are designed for subclasses
@@ -159,45 +156,60 @@ void CBinarySerializer::HandleScriptVal(jsval val)
 		}
 
 		// Find all properties (ordered by insertion time)
-		JSIdArray* ida = JS_Enumerate(cx, obj);
-		if (!ida)
-		{
-			LOGERROR(L"JS_Enumerate failed");
-			throw PSERROR_Serialize_ScriptError();
-		}
-		// For safety, root all the property IDs
-		// (This should be unnecessary if we're certain that properties could never
-		// get deleted during deserialization)
-		IdArrayWrapper idaWrapper(cx, ida);
 
-		NumberU32_Unbounded("num props", (uint32_t)ida->length);
+		// JS_Enumerate is a bit slow (lots of memory allocation), so do the enumeration manually
+		// (based on the code from JS_Enumerate, using the probably less stable JSObject API):
 
-		for (jsint i = 0; i < ida->length; ++i)
+		// (Note that we don't do any rooting, because we assume nothing is going to trigger GC.
+		// I'm not absolute certain that's necessarily a valid assumption.)
+
+		jsval iter_state, num_properties;
+		if (!obj->enumerate(cx, JSENUMERATE_INIT, &iter_state, &num_properties))
+			throw PSERROR_Serialize_ScriptError("enumerate INIT failed");
+		debug_assert(JSVAL_TO_INT(num_properties) >= 0);
+		size_t n = (size_t)JSVAL_TO_INT(num_properties);
+
+		// Note: num_properties might be 0 if the object doesn't know in advance how many to enumerate;
+		// we can't distinguish that from really having 0 properties without performing the actual iteration,
+		// so just assume the object always returns the correct count
+
+		NumberU32_Unbounded("num props", (uint32_t)n);
+
+		jsid id;
+
+		for (size_t i = 0; i < n; ++i)
 		{
+			if (!obj->enumerate(cx, JSENUMERATE_NEXT, &iter_state, &id))
+				throw PSERROR_Serialize_ScriptError("enumerate NEXT failed");
+
+			if (JSVAL_IS_NULL(iter_state))
+				throw PSERROR_Serialize_ScriptError("enumerate NEXT gave unexpected null");
+
 			jsval idval, propval;
 
-			// Forbid getters, because they will be weird and not serialise properly
-			JSPropertyDescriptor descr;
-			if (!JS_GetPropertyDescriptorById(cx, obj, ida->vector[i], JSRESOLVE_QUALIFIED, &descr))
-				throw PSERROR_Serialize_ScriptError("JS_GetPropertyDescriptorById failed");
-			if (descr.attrs & JSPROP_GETTER)
-				throw PSERROR_Serialize_ScriptError("Cannot serialize property getters");
-
 			// Get the property name as a string
-			if (!JS_IdToValue(cx, ida->vector[i], &idval))
+			if (!JS_IdToValue(cx, id, &idval))
 				throw PSERROR_Serialize_ScriptError("JS_IdToValue failed");
 			JSString* idstr = JS_ValueToString(cx, idval);
 			if (!idstr)
 				throw PSERROR_Serialize_ScriptError("JS_ValueToString failed");
-			CScriptValRooted idstrRoot(cx, STRING_TO_JSVAL(idstr));
 
 			ScriptString("prop name", idstr);
 
-			if (!JS_GetPropertyById(cx, obj, ida->vector[i], &propval))
-				throw PSERROR_Serialize_ScriptError("JS_GetPropertyById failed");
+			// Use LookupProperty instead of GetProperty to avoid the danger of getters
+			// (they might delete values and trigger GC)
+			if (!JS_LookupPropertyById(cx, obj, id, &propval))
+				throw PSERROR_Serialize_ScriptError("JS_LookupPropertyById failed");
 
 			HandleScriptVal(propval);
 		}
+
+		// Check we really reached the end of the iteration
+		if (!obj->enumerate(cx, JSENUMERATE_NEXT, &iter_state, &id))
+			throw PSERROR_Serialize_ScriptError("enumerate NEXT failed");
+		if (!JSVAL_IS_NULL(iter_state))
+			throw PSERROR_Serialize_ScriptError("enumerate NEXT didn't give unexpected null");
+
 		break;
 	}
 	case JSTYPE_FUNCTION:
@@ -273,7 +285,7 @@ u32 CBinarySerializer::GetScriptBackrefTag(JSObject* obj)
 	// The tags are stored in a map. Maybe it'd be more efficient to store it inline in the object
 	// somehow? but this works okay for now
 
-	std::pair<std::map<JSObject*, u32>::iterator, bool> it = m_ScriptBackrefs.insert(std::make_pair(obj, (u32)m_ScriptBackrefs.size()+1));
+	std::pair<backrefs_t::iterator, bool> it = m_ScriptBackrefs.insert(std::make_pair(obj, m_ScriptBackrefsNext));
 
 	// If it was already there, return the tag
 	if (!it.second)
@@ -281,19 +293,8 @@ u32 CBinarySerializer::GetScriptBackrefTag(JSObject* obj)
 
 	// If it was newly inserted, we need to make sure it gets rooted
 	// for the duration that it's in m_ScriptBackrefs
-	if (!JS_AddRoot(m_ScriptInterface.GetContext(), (void*)&it.first->first))
-		throw PSERROR_Serialize_ScriptError("JS_AddRoot failed");
+	m_Rooter.Push(it.first->first);
+	m_ScriptBackrefsNext++;
 	// Return a non-tag number so callers know they need to serialize the object
 	return 0;
 }
-
-void CBinarySerializer::FreeScriptBackrefs()
-{
-	std::map<JSObject*, u32>::iterator it = m_ScriptBackrefs.begin();
-	for (; it != m_ScriptBackrefs.end(); ++it)
-	{
-		if (!JS_RemoveRoot(m_ScriptInterface.GetContext(), (void*)&it->first))
-			throw PSERROR_Serialize_ScriptError("JS_RemoveRoot failed");
-	}
-	m_ScriptBackrefs.clear();
-}

source/simulation2/serialization/BinarySerializer.h

@@ -20,6 +20,8 @@
 
 #include "ISerializer.h"
 
+#include "scriptinterface/AutoRooters.h"
+
 #include <map>
 
 /**
@@ -31,7 +33,6 @@ class CBinarySerializer : public ISerializer
 	NONCOPYABLE(CBinarySerializer);
 public:
 	CBinarySerializer(ScriptInterface& scriptInterface);
-	virtual ~CBinarySerializer();
 
 protected:
 	virtual void PutNumber(const char* name, uint8_t value);
@@ -48,10 +49,16 @@ private:
 	// PutScriptVal implementation details:
 	void ScriptString(const char* name, JSString* string);
 	void HandleScriptVal(jsval val);
-	u32 GetScriptBackrefTag(JSObject* obj);
-	void FreeScriptBackrefs();
-	std::map<JSObject*, u32> m_ScriptBackrefs;
+
 	ScriptInterface& m_ScriptInterface;
+
+	typedef std::map<JSObject*, u32> backrefs_t;
+
+	backrefs_t m_ScriptBackrefs;
+	u32 m_ScriptBackrefsNext;
+	u32 GetScriptBackrefTag(JSObject* obj);
+
+	AutoGCRooter m_Rooter;
 };
 
 #endif // INCLUDED_BINARYSERIALIZER

source/simulation2/tests/test_ComponentManager.h

@@ -707,7 +707,8 @@ public:
 		man.AddComponent(ent1, man.LookupCID("TestScript1_getter"), noParam);
 
 		std::stringstream stateStream;
-		TS_ASSERT_THROWS_PSERROR(man.SerializeState(stateStream), PSERROR_Serialize_ScriptError, "Cannot serialize property getters");
+		TS_ASSERT(man.SerializeState(stateStream));
+		// (The script will die if the getter is executed)
 	}
 
 	void test_script_serialization_template()