From adcb9755ff9a36f8da45e4535d882992a93d5724 Mon Sep 17 00:00:00 2001 From: Itms Date: Sun, 17 Nov 2024 16:25:17 +0100 Subject: [PATCH] Refactor freezing of JS objects - Shallow-freezing of objects is never used in our codebase, so remove that code path. - Deep-freeze is bugged in recent versions of SpiderMonkey (see bug https://bugzilla.mozilla.org/show_bug.cgi?id=1930258). Until a fix and/or a better API is provided, reimplement this feature by recusively freezing object properties. - Add tests for the deepfreeze function. --- source/graphics/MapGenerator.cpp | 2 +- source/lobby/XmppClient.cpp | 4 +-- source/ps/Replay.cpp | 2 +- source/ps/scripting/JSInterface_Mod.cpp | 2 +- source/scriptinterface/Object.h | 30 +++++++++++++++---- source/scriptinterface/ScriptInterface.cpp | 2 +- .../tests/test_ScriptInterface.h | 18 +++++++++++ source/simulation2/Simulation2.cpp | 4 +-- .../scripting/EngineScriptConversions.cpp | 2 +- source/simulation2/system/TurnManager.cpp | 4 +-- 10 files changed, 53 insertions(+), 17 deletions(-) diff --git a/source/graphics/MapGenerator.cpp b/source/graphics/MapGenerator.cpp index 68e4ff9c1d..b17c2e70ba 100644 --- a/source/graphics/MapGenerator.cpp +++ b/source/graphics/MapGenerator.cpp @@ -388,7 +388,7 @@ Script::StructuredClone RunMapGenerationScript(const StopToken stopToken, std::a } // Prevent unintentional modifications to the settings object by random map scripts - if (!Script::FreezeObject(rq, settingsVal, true)) + if (!Script::DeepFreezeObject(rq, settingsVal)) { LOGERROR("RunMapGenerationScript: Failed to deepfreeze settings"); return nullptr; diff --git a/source/lobby/XmppClient.cpp b/source/lobby/XmppClient.cpp index 7783e8c7f0..f53c0ac310 100644 --- a/source/lobby/XmppClient.cpp +++ b/source/lobby/XmppClient.cpp @@ -704,7 +704,7 @@ void XmppClient::CreateGUIMessage( JS::RootedObject messageObj(rq.cx, message.toObjectOrNull()); SetGUIMessageProperty(rq, messageObj, args...); - Script::FreezeObject(rq, message, true); + Script::DeepFreezeObject(rq, message); m_GuiMessageQueue.push_back(JS::Heap(message)); } @@ -755,7 +755,7 @@ JS::Value XmppClient::GuiPollNewMessages(const ScriptInterface& guiInterface) if (true) { Script::SetProperty(rq, historicMessage, "historic", true); - Script::FreezeObject(rq, historicMessage, true); + Script::DeepFreezeObject(rq, historicMessage); m_HistoricGuiMessages.push_back(JS::Heap(historicMessage)); } else diff --git a/source/ps/Replay.cpp b/source/ps/Replay.cpp index 9d19eb2d45..1cc9c26fe1 100644 --- a/source/ps/Replay.cpp +++ b/source/ps/Replay.cpp @@ -279,7 +279,7 @@ void CReplayPlayer::Replay(const bool serializationtest, const int rejointesttur ScriptRequest rq(g_Game->GetSimulation2()->GetScriptInterface()); JS::RootedValue data(rq.cx); Script::ParseJSON(rq, line, &data); - Script::FreezeObject(rq, data, true); + Script::DeepFreezeObject(rq, data); commands.emplace_back(SimulationCommand(player, rq.cx, data)); } else if (type == "hash" || type == "hash-quick") diff --git a/source/ps/scripting/JSInterface_Mod.cpp b/source/ps/scripting/JSInterface_Mod.cpp index 0ab43744ad..47cf8a4c8e 100644 --- a/source/ps/scripting/JSInterface_Mod.cpp +++ b/source/ps/scripting/JSInterface_Mod.cpp @@ -124,7 +124,7 @@ JS::Value GetEngineInfo(const ScriptInterface& scriptInterface) "engine_version", engine_version, "mods", mods); - Script::FreezeObject(rq, metainfo, true); + Script::DeepFreezeObject(rq, metainfo); return metainfo; } diff --git a/source/scriptinterface/Object.h b/source/scriptinterface/Object.h index 774db88993..2470623c36 100644 --- a/source/scriptinterface/Object.h +++ b/source/scriptinterface/Object.h @@ -150,17 +150,35 @@ inline bool GetObjectClassName(const ScriptRequest& rq, JS::HandleValue val, T& return GetObjectClassName(rq, obj, name); } -inline bool FreezeObject(const ScriptRequest& rq, JS::HandleValue objVal, bool deep) +inline bool DeepFreezeObject(const ScriptRequest& rq, JS::HandleValue objVal) { if (!objVal.isObject()) + { + LOGERROR("DeepFreezeObject expected object type!"); + return false; + } + + // Get all properties and recursively freeze those that are objects + JS::RootedObject obj(rq.cx, &objVal.toObject()); + JS::RootedIdVector props(rq.cx); + if (!js::GetPropertyKeys(rq.cx, obj, JSITER_HIDDEN, &props)) return false; - JS::RootedObject obj(rq.cx, &objVal.toObject()); + for (size_t i = 0; i < props.length(); ++i) + { + JS::RootedId id(rq.cx, props[i]); + JS::RootedValue val(rq.cx); + if (!JS_IdToValue(rq.cx, id, &val)) + return false; - if (deep) - return JS_DeepFreezeObject(rq.cx, obj); - else - return JS_FreezeObject(rq.cx, obj); + if (!val.isObject()) + continue; + + if (!DeepFreezeObject(rq, val)) + return false; + } + + return JS_FreezeObject(rq.cx, obj); } /** diff --git a/source/scriptinterface/ScriptInterface.cpp b/source/scriptinterface/ScriptInterface.cpp index 0df8477046..6b382ddabd 100644 --- a/source/scriptinterface/ScriptInterface.cpp +++ b/source/scriptinterface/ScriptInterface.cpp @@ -214,7 +214,7 @@ JS::Value deepfreeze(const ScriptInterface& scriptInterface, JS::HandleValue val return JS::UndefinedValue(); } - Script::FreezeObject(rq, val, true); + Script::DeepFreezeObject(rq, val); return val; } diff --git a/source/scriptinterface/tests/test_ScriptInterface.h b/source/scriptinterface/tests/test_ScriptInterface.h index 065913b6d7..8ac1159038 100644 --- a/source/scriptinterface/tests/test_ScriptInterface.h +++ b/source/scriptinterface/tests/test_ScriptInterface.h @@ -134,6 +134,24 @@ public: } } + void test_deepfreeze() + { + ScriptInterface script("Test", "Test", g_ScriptContext); + TestLogger logger; + + TS_ASSERT(!script.LoadScript(L"testFreezeObject.js", "var obj1 = { foo: \"a\"}; deepfreeze(obj1); obj1.bar = \"b\";")); + TS_ASSERT_STR_CONTAINS(logger.GetOutput(), "Object is not extensible"); + + TS_ASSERT(!script.LoadScript(L"testDeepFreezeObject.js", "var obj2 = { foo: \"a\"}; deepfreeze(obj2); obj2.foo = \"b\";")); + TS_ASSERT_STR_CONTAINS(logger.GetOutput(), "\"foo\" is read-only"); + + TS_ASSERT(!script.LoadScript(L"testFreezeArray.js", "var array = [0]; deepfreeze(array); array.push(1);")); + TS_ASSERT_STR_CONTAINS(logger.GetOutput(), "can\'t define array index property past the end of an array with non-writable length"); + + TS_ASSERT(!script.LoadScript(L"testFreezeFunction.js", "var fn = a => a+1; deepfreeze(fn); fn.name = \"foo\";")); + TS_ASSERT_STR_CONTAINS(logger.GetOutput(), "\"name\" is read-only"); + } + /** * This test is mainly to make sure that all required template overloads get instantiated at least once so that compiler errors * in these functions are revealed instantly (but it also tests the basic functionality of these functions). diff --git a/source/simulation2/Simulation2.cpp b/source/simulation2/Simulation2.cpp index 7a92a6b769..9c43445d3f 100644 --- a/source/simulation2/Simulation2.cpp +++ b/source/simulation2/Simulation2.cpp @@ -171,7 +171,7 @@ public: for (const SimulationCommand& command : commands) { JS::RootedValue tmpCommand(rqNew.cx, Script::CloneValueFromOtherCompartment(newScript, oldScript, command.data)); - Script::FreezeObject(rqNew, tmpCommand, true); + Script::DeepFreezeObject(rqNew, tmpCommand); SimulationCommand cmd(command.player, rqNew.cx, tmpCommand); newCommands.emplace_back(std::move(cmd)); } @@ -849,7 +849,7 @@ void CSimulation2::LoadMapSettings() // Initialize here instead of in Update() ScriptFunction::CallVoid(rq, global, "LoadMapSettings", m->m_MapSettings); - Script::FreezeObject(rq, m->m_InitAttributes, true); + Script::DeepFreezeObject(rq, m->m_InitAttributes); GetScriptInterface().SetGlobal("InitAttributes", m->m_InitAttributes, true, true, true); if (!m->m_StartupScript.empty()) diff --git a/source/simulation2/scripting/EngineScriptConversions.cpp b/source/simulation2/scripting/EngineScriptConversions.cpp index 0f6d1bcb6c..6cc31a3470 100644 --- a/source/simulation2/scripting/EngineScriptConversions.cpp +++ b/source/simulation2/scripting/EngineScriptConversions.cpp @@ -73,7 +73,7 @@ template<> void Script::ToJSVal(const ScriptRequest& rq, JS::Mutabl // Prevent modifications to the object, so that it's safe to share between // components and to reconstruct on deserialization if (ret.isObject()) - Script::FreezeObject(rq, ret, true); + Script::DeepFreezeObject(rq, ret); } template<> void Script::ToJSVal(const ScriptRequest& rq, JS::MutableHandleValue ret, const CParamNode* const& val) diff --git a/source/simulation2/system/TurnManager.cpp b/source/simulation2/system/TurnManager.cpp index 7e2da4fbd8..68112b0255 100644 --- a/source/simulation2/system/TurnManager.cpp +++ b/source/simulation2/system/TurnManager.cpp @@ -223,7 +223,7 @@ void CTurnManager::AddCommand(int client, int player, JS::HandleValue data, u32 ScriptRequest rq(m_Simulation2.GetScriptInterface()); - Script::FreezeObject(rq, data, true); + Script::DeepFreezeObject(rq, data); size_t command_in_turns = turn - (m_CurrentTurn+1); if (m_QueuedCommands.size() <= command_in_turns) @@ -295,7 +295,7 @@ void CTurnManager::QuickSave(JS::HandleValue GUIMetadata) m_QuickSaveMetadata.set(Script::DeepCopy(rq, GUIMetadata)); // Freeze state to ensure that consectuvie loads don't modify the state - Script::FreezeObject(rq, m_QuickSaveMetadata, true); + Script::DeepFreezeObject(rq, m_QuickSaveMetadata); LOGMESSAGERENDER("Quicksaved game"); }