id,summary,reporter,owner,description,type,status,priority,milestone,component,resolution,keywords,cc,phab_field
4759,[PATCH] JS simulation test components should not be able to modify their template and entity data,elexis,,"'''Problem''':
Template data and the entity ID of the JS simulation components (EDIT: in tests!) should not be modifyable.

In most cases, such modifications come from assuming that a copy of the template data is modified, while in fact the underlying object is modified,
implying hidden bugs.

The affected simulation components then often have to add a `clone` call to not modify the original object.

C++ simulation components are not affected, because they read template data from the `ParamNode` directly.

'''Reproduce:'''
Open `Attack.js` and set `this.entity = -1` or `this.template.foo = ""bar""` for example.

'''Examples:'''
~~Two~~ examples are ~~r19623 and~~ r20015. These bugs would have been cought with a red error message instead of a hidden gameplay bug if the properties were frozen.

The same phenomenon of unintentionally modifying GUI and simulation data in JS had been observed and fixed in #3647 and #4257.

'''Responsible code''':
~~JS components are created by `ScriptComponent.cpp`.~~
~~The `template` and `entity` properties are set by `CComponentTypeScript::Init`.

~~r8865 actually attempted to freeze those two properties there, but actually didn't or doesn't anymore. (I've checked out that revision and r9000 but cannot compile it due to FCollada errors).

~~The template is converted from a `ParamNode` to a `JS::Value` in  `EngineScriptConversions.cpp` using `ScriptInterface::ToJSVal<CParamNode>`.
This function also calls deepfreezes.

~~After the `template` and `entity` properties are set, the `Init` function of the JS components is called by `CComponentTypeScript::Init` using `m_ScriptInterface.CallFunctionVoid`, which passes through the adventurous `NativeWrapperDecls.h` and ends up in `ScriptInterface::CallFunction_`, which just calls `JS_CallFunctionName`.

'''Explored solution:'''
~~By setting the `JSPROP_ENUMERATE` flag with the patch below, these two properties will become both visible and frozen in the simulation component (so the aim of the ticket would be achieved).

{{{
Index: source/simulation2/scripting/ScriptComponent.cpp
===================================================================
--- source/simulation2/scripting/ScriptComponent.cpp	(revision 20098)
+++ source/simulation2/scripting/ScriptComponent.cpp	(working copy)
@@ -43,8 +43,8 @@
 
 void CComponentTypeScript::Init(const CParamNode& paramNode, entity_id_t ent)
 {
-	m_ScriptInterface.SetProperty(m_Instance, ""entity"", (int)ent, true, false);
-	m_ScriptInterface.SetProperty(m_Instance, ""template"", paramNode, true, false);
+	m_ScriptInterface.SetProperty(m_Instance, ""entity"", (int)ent, true, true);
+	m_ScriptInterface.SetProperty(m_Instance, ""template"", paramNode, true, true);
 	m_ScriptInterface.CallFunctionVoid(m_Instance, ""Init"");
 }
 
}}}
~~However `for...in` loop and `Object.properties` don't list these properties currently, but after setting the flag, they would.~~
~~So every JS simulation component would have to take care to explicitly ignore these two. The C++ serialization code (including the `BinarySerializer.cpp`) will have to account for the two new properties encountered.

~~IF setting the `JSPROP_ENUMERATE` is not the right way to go, then the frozen `template` property must be unintentionally copied somewhere by the callstack, which is either a bug on our side or in SpiderMonkey.
",defect,new,Should Have,Alpha 23,Core engine,,,,Phab:D871