Changes between Initial Version and Version 2 of Ticket #4759


Ignore:
Timestamp:
Sep 5, 2017, 1:11:32 AM (7 years ago)
Author:
elexis
Comment:

You are right, I'm glad I can reproduce that ingame! The original error that led to the fix in r19623 was a "read-only" error as well.

The issue exists only within the JS simulation components tests (I thought they were created equally many hours ago).

r20015 made me look at the entire deepfreeze thing (#4257) and it appears like two clones could be unneeded there.

​Phab:D871 removes them and freezes the two properties.

Thanks for finding my fallacy, case closed basically!

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #4759

    • Property Patch → Phab:D871
    • Property Milestone Backlog → Alpha 23
    • Property Summary JS simulation components should not be able to modify their template and entity data → [PATCH] JS simulation test components should not be able to modify their template and entity data

  • Ticket #4759 – Description

    initial v2  
    11'''Problem''':
    2 Template data and the entity ID of the JS simulation components should not be modifyable.
     2Template data and the entity ID of the JS simulation components (EDIT: in tests!) should not be modifyable.
    33
    44In most cases, such modifications come from assuming that a copy of the template data is modified, while in fact the underlying object is modified,
    … …  
    1313
    1414'''Examples:'''
    15 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.
     15~~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.
    1616
    1717The same phenomenon of unintentionally modifying GUI and simulation data in JS had been observed and fixed in #3647 and #4257.
    1818
    1919'''Responsible code''':
    20 JS components are created by `ScriptComponent.cpp`.
    21 The `template` and `entity` properties are set by `CComponentTypeScript::Init`.
     20~~JS components are created by `ScriptComponent.cpp`.~~
     21~~The `template` and `entity` properties are set by `CComponentTypeScript::Init`.
    2222
    23 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).
     23~~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).
    2424
    25 The template is converted from a `ParamNode` to a `JS::Value` in  `EngineScriptConversions.cpp` using `ScriptInterface::ToJSVal<CParamNode>`.
     25~~The template is converted from a `ParamNode` to a `JS::Value` in  `EngineScriptConversions.cpp` using `ScriptInterface::ToJSVal<CParamNode>`.
    2626This function also calls deepfreezes.
    2727
    28 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`.
     28~~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`.
    2929
    3030'''Explored solution:'''
    31 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).
     31~~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).
    3232
    3333{{{
    … …  
    4848 
    4949}}}
    50 However `for...in` loop and `Object.properties` don't list these properties currently, but after setting the flag, they would.
    51 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.
     50~~However `for...in` loop and `Object.properties` don't list these properties currently, but after setting the flag, they would.~~
     51~~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.
    5252
    53 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.
     53~~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.