Revert dad2857538 / Keep track of serialized objects using a GCHashTable

This reverts dad2857538. That diff had two issues:
- It modifies the JS objects, which means subsequent serialization in
quicksave are 'dirty'.
- It doesn't work with non-extensible objects. That's rather annoying,
and has already caused problems.
It also revert f0faab7a42, which was necessary because of the second
issue.

Fixes #5908

Differential Revision: https://code.wildfiregames.com/D3336
This was SVN commit r24563.

binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/PerPlayer/Dropdowns/PlayerColor.js

@@ -48,7 +48,7 @@ PlayerSettingControls.PlayerColor = class extends GameSettingControlDropdown
 					smallestDistance = distance;
 				}
 			}
-			this.values.push(clone(closestColor));
+			this.values.push(closestColor);
 		}
 
 		this.dropdown.list = this.values.map(color => coloredText(this.ColorIcon, rgbToGuiColor(color)));

source/simulation2/serialization/BinarySerializer.cpp

@@ -59,7 +59,12 @@ CBinarySerializerScriptImpl::CBinarySerializerScriptImpl(const ScriptInterface&
 {
 	ScriptRequest rq(m_ScriptInterface);
 
-	m_ScriptBackrefSymbol.init(rq.cx, JS::NewSymbol(rq.cx, nullptr));
+	JS_AddExtraGCRootsTracer(m_ScriptInterface.GetGeneralJSContext(), Trace, this);
+}
+
+CBinarySerializerScriptImpl::~CBinarySerializerScriptImpl()
+{
+	JS_RemoveExtraGCRootsTracer(m_ScriptInterface.GetGeneralJSContext(), Trace, this);
 }
 
 void CBinarySerializerScriptImpl::HandleScriptVal(JS::HandleValue val)
@@ -89,11 +94,11 @@ void CBinarySerializerScriptImpl::HandleScriptVal(JS::HandleValue val)
 		JS::RootedObject obj(rq.cx, &val.toObject());
 
 		// If we've already serialized this object, just output a reference to it
-		i32 tag = GetScriptBackrefTag(obj);
-		if (tag != -1)
+		u32 tag = GetScriptBackrefTag(obj);
+		if (tag != 0)
 		{
 			m_Serializer.NumberU8_Unbounded("type", SCRIPT_TYPE_BACKREF);
-			m_Serializer.NumberI32("tag", tag, 0, JSVAL_INT_MAX);
+			m_Serializer.NumberU32("tag", tag, 0, JSVAL_INT_MAX);
 			break;
 		}
 
@@ -437,7 +442,13 @@ void CBinarySerializerScriptImpl::ScriptString(const char* name, JS::HandleStrin
 	}
 }
 
-i32 CBinarySerializerScriptImpl::GetScriptBackrefTag(JS::HandleObject obj)
+void CBinarySerializerScriptImpl::Trace(JSTracer *trc, void *data)
+{
+	CBinarySerializerScriptImpl* serializer = static_cast<CBinarySerializerScriptImpl*>(data);
+	serializer->m_ScriptBackrefTags.trace(trc);
+}
+
+u32 CBinarySerializerScriptImpl::GetScriptBackrefTag(JS::HandleObject obj)
 {
 	// To support non-tree structures (e.g. "var x = []; var y = [x, x];"), we need a way
 	// to indicate multiple references to one object(/array). So every time we serialize a
@@ -449,33 +460,13 @@ i32 CBinarySerializerScriptImpl::GetScriptBackrefTag(JS::HandleObject obj)
 
 	ScriptRequest rq(m_ScriptInterface);
 
-	JS::RootedValue symbolValue(rq.cx, JS::SymbolValue(m_ScriptBackrefSymbol));
-	JS::RootedId symbolId(rq.cx);
-	ENSURE(JS_ValueToId(rq.cx, symbolValue, &symbolId));
-
-	JS::RootedValue tagValue(rq.cx);
-
-	// If it was already there, return the tag
-	bool tagFound;
-	ENSURE(JS_HasPropertyById(rq.cx, obj, symbolId, &tagFound));
-	if (tagFound)
+	ObjectTagMap::Ptr ptr = m_ScriptBackrefTags.lookup(JS::Heap<JSObject*>(obj.get()));
+	if (!ptr.found())
 	{
-		ENSURE(JS_GetPropertyById(rq.cx, obj, symbolId, &tagValue));
-		ENSURE(tagValue.isInt32());
-		return tagValue.toInt32();
+		ENSURE(m_ScriptBackrefTags.put(JS::Heap<JSObject*>(obj.get()), ++m_ScriptBackrefsNext));
+		// Return 0 to mean "you have to serialize this object";
+		return 0;
 	}
-
-	tagValue = JS::Int32Value(m_ScriptBackrefsNext);
-	// TODO: this fails if the object cannot be written to.
-	// This means we could end up in an infinite loop...
-	if (!JS_DefinePropertyById(rq.cx, obj, symbolId, tagValue, JSPROP_READONLY))
-	{
-		// For now just warn, this should be user-fixable and may not actually error out.
-		JS::RootedValue objVal(rq.cx, JS::ObjectValue(*obj.get()));
-		LOGWARNING("Serialization symbol cannot be written on object %s", m_ScriptInterface.ToString(&objVal));
-	}
-
-	++m_ScriptBackrefsNext;
-	// Return a non-tag number so callers know they need to serialize the object
-	return -1;
+	else
+		return ptr->value();
 }

source/simulation2/serialization/BinarySerializer.h

@@ -22,6 +22,9 @@
 
 #include "lib/byte_order.h"
 
+#include "js/AllocPolicy.h"
+#include "js/GCHashTable.h"
+
 #include <iostream>
 #include <map>
 #include <streambuf>
@@ -83,16 +86,20 @@ class CBinarySerializerScriptImpl
 {
 public:
 	CBinarySerializerScriptImpl(const ScriptInterface& scriptInterface, ISerializer& serializer);
+	~CBinarySerializerScriptImpl();
 
 	void ScriptString(const char* name, JS::HandleString string);
 	void HandleScriptVal(JS::HandleValue val);
 private:
+	static void Trace(JSTracer* trc, void* data);
+
 	const ScriptInterface& m_ScriptInterface;
 	ISerializer& m_Serializer;
 
-	JS::PersistentRootedSymbol m_ScriptBackrefSymbol;
-	i32 m_ScriptBackrefsNext;
-	i32 GetScriptBackrefTag(JS::HandleObject obj);
+	using ObjectTagMap = JS::GCHashMap<JS::Heap<JSObject*>, u32, js::PointerHasher<JSObject*>, js::SystemAllocPolicy>;
+	ObjectTagMap m_ScriptBackrefTags;
+	u32 m_ScriptBackrefsNext;
+	u32 GetScriptBackrefTag(JS::HandleObject obj);
 };
 
 /**

source/simulation2/serialization/StdDeserializer.cpp

@@ -31,6 +31,8 @@ CStdDeserializer::CStdDeserializer(const ScriptInterface& scriptInterface, std::
 	m_ScriptInterface(scriptInterface), m_Stream(stream)
 {
 	JS_AddExtraGCRootsTracer(m_ScriptInterface.GetGeneralJSContext(), CStdDeserializer::Trace, this);
+	// Insert a dummy object in front, as valid tags start at 1.
+	m_ScriptBackrefs.emplace_back(nullptr);
 }
 
 CStdDeserializer::~CStdDeserializer()

source/simulation2/tests/test_Serializer.h

@@ -719,13 +719,13 @@ public:
 		);
 
 		helper_script_roundtrip("Nested maps using backrefs",
-			"var a = new Map(); var b = new Map(); a.set(1, b); a.set(2, b); a",
+			"var a = new Map(); var b = new Map(); a.set(1, b); a.set(2, b); a.set(3, b); a",
 		/* expected: */
 			"({})",
 		/* expected stream: */
-			25,
+			35,
 			"\x0f" // SCRIPT_TYPE_MAP
-			"\x02\0\0\0" // size
+			"\x03\0\0\0" // size
 
 			"\x05" // SCRIPT_TYPE_INT
 			"\x01\0\0\0" // 1
@@ -735,7 +735,12 @@ public:
 			"\x05" // SCRIPT_TYPE_INT
 			"\x02\0\0\0" // 2
 			"\x08" // SCRIPT_TYPE_BACKREF
-			"\x01\0\0\0" // ref. to object #1, i.e. "b", with #0 being "a"
+			"\x02\0\0\0" // ref. to object #2, i.e. "b", with #1 being "a"
+
+			"\x05" // SCRIPT_TYPE_INT
+			"\x03\0\0\0" // 3
+			"\x08" // SCRIPT_TYPE_BACKREF
+			"\x02\0\0\0" // ref. to object #2, i.e. "b", with #1 being "a"
 		);
 	}
 
@@ -757,7 +762,7 @@ public:
 			"\x01\0\0\0" // num props
 				"\x01\x03\0\0\0" "bar" // "bar"
 				"\x08" // SCRIPT_TYPE_BACKREF
-				"\x01\0\0\0" // ref to object #1, i.e. "b", with #0 being "a"
+				"\x02\0\0\0" // ref to object #2, i.e. "b", with #1 being "a"
 		);
 	}