diff --git a/binaries/data/mods/_test.sim/simulation/components/test-serialize.js b/binaries/data/mods/_test.sim/simulation/components/test-serialize.js
index 01a635097c..be8cba461d 100644
--- a/binaries/data/mods/_test.sim/simulation/components/test-serialize.js
+++ b/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);
diff --git a/source/scriptinterface/AutoRooters.cpp b/source/scriptinterface/AutoRooters.cpp
new file mode 100644
index 0000000000..51ec4f3f9f
--- /dev/null
+++ b/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 .
+ */
+
+#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");
+ }
+}
diff --git a/source/scriptinterface/AutoRooters.h b/source/scriptinterface/AutoRooters.h
index 0e00a454b5..9dd2488524 100644
--- a/source/scriptinterface/AutoRooters.h
+++ b/source/scriptinterface/AutoRooters.h
@@ -15,8 +15,13 @@
* along with 0 A.D. If not, see .
*/
+#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 m_Objects;
+ // TODO: add vectors of other value types
+};
+
+#endif // INCLUDED_AUTOROOTERS
diff --git a/source/scriptinterface/ScriptInterface.cpp b/source/scriptinterface/ScriptInterface.cpp
index e6f63af694..8cc72fdcd2 100644
--- a/source/scriptinterface/ScriptInterface.cpp
+++ b/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(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)
{
diff --git a/source/scriptinterface/ScriptInterface.h b/source/scriptinterface/ScriptInterface.h
index 89506a87da..c11f9d969c 100644
--- a/source/scriptinterface/ScriptInterface.h
+++ b/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.
*/
diff --git a/source/simulation2/serialization/BinarySerializer.cpp b/source/simulation2/serialization/BinarySerializer.cpp
index f356b2ad88..fdcd998221 100644
--- a/source/simulation2/serialization/BinarySerializer.cpp
+++ b/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::iterator, bool> it = m_ScriptBackrefs.insert(std::make_pair(obj, (u32)m_ScriptBackrefs.size()+1));
+ std::pair 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::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();
-}
diff --git a/source/simulation2/serialization/BinarySerializer.h b/source/simulation2/serialization/BinarySerializer.h
index d5aa35bd94..2d2730f77e 100644
--- a/source/simulation2/serialization/BinarySerializer.h
+++ b/source/simulation2/serialization/BinarySerializer.h
@@ -20,6 +20,8 @@
#include "ISerializer.h"
+#include "scriptinterface/AutoRooters.h"
+
#include