Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#4257 closed defect (fixed)

DeepFreeze JS variables that are data sources

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 23
Component: UI & Simulation Keywords:
Cc: Patch: Phab:D829

Description (last modified by elexis)

The functions GetSimState, GetEntityState, GetExtendedEntityState, GetTemplateData, GetTemplateDataWithoutLocalization, GetTechnologyData cache their results in global variables and return these objects each.

However, returning an object means that the reference is returned, not a copy. This isn't an issue if that object was newly created (or cloned). But if the function returns an object which is accessible outside of the scope of the function (f.e. a global cache variable), modifying the returned object modifies the cache, which is not intentional and adulterates the original data.

This is often not considered and has caused a number of bugs already, f.e. r18788 r17324 r17673 r17441 r17712 r17715.

The mentioned functions should use Object.freeze to ensure that the returned object can't be modified.

There are likely many other places in gui/ where global variables are used to cache things, that should be frozen.

Change History (12)

comment:1 by elexis, 4 years ago

Another example are the cached JSON aura and tech templates in the DataTemplateManager. If those were modified somewhere that were pretty bad.

comment:2 by elexis, 4 years ago

Notice Object.freeze is not sufficient as it doesn't work recursively.

comment:3 by javiergodas, 4 years ago

Owner: set to javiergodas
Status: newassigned

comment:4 by elexis, 4 years ago

Resources.js should also freeze the resource objects, once it is included (#3934).

comment:5 by elexis, 3 years ago

Owner: javiergodas removed
Status: assignednew

comment:6 by elexis, 3 years ago

Description: modified (diff)
Milestone: BacklogAlpha 23

Exemplary JS code for recursive freezing (deep-freeze) is described at:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

It should be added to globalscripts/utility.js as it is needed in both simulation and GUI.

comment:7 by elexis, 3 years ago

Summary: Freeze cached session objectsDeepFreeze JS variables that are data sources

comment:8 by elexis, 3 years ago

Keywords: simple removed
Patch: Phab:D829

comment:9 by elexis, 3 years ago

In 20099:

Add deepfreeze function to recursively mark JS Objects as read-only.

Refs #4257, D829
Proofread By: leper

comment:10 by elexis, 3 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 20100:

Deepfreeze Aura, Technology and Resource Templates, Simulation states, GameAttributes and few other JS objects.
This reveals unintentional modifications to these objects which would most often imply hidden bugs.

Differential Revision: https://code.wildfiregames.com/D829
Fixes #4257
Refs #3647

comment:11 by elexis, 3 years ago

In r20265: Deepfreeze the map settings object (created from g_GameAttributes.settings by the gamesetup extending the map JSON data) to prevent random map scripts from unintentionally modifying that, refs #4257.

comment:12 by elexis, 3 years ago

In 20412:

Cleanup gate locking GUI code of rP15375.

Differential Revision: https://code.wildfiregames.com/D1004
Patch By: temple

Remove any unneeded complexity to compute the button states.
The entity state of selected gates was changed, so fix the read-only errors following rP20100, refs #4257.

Note: See TracTickets for help on using tickets.