id,summary,reporter,owner,description,type,status,priority,milestone,component,resolution,keywords,cc,phab_field
4759,JS simulation 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 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,Backlog,Core engine,,,,