Deepfreeze Aura, Technology and Resource Templates, Simulation states…
AuditedrP20100
Actions

Description

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

Details

Auditors

mimo

Committed

elexis

Sep 3 2017, 6:37 PM

Differential Revision

D829: Deep freeze templates, techs, auras, resources data and entity states

Parents

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

Branches

Unknown

Tags

Unknown

Build Status

Buildable 2990
Build 5165: Trigger Windows Autobuild
Build 5164: Post-Commit Build	Jenkins

Event Timeline

elexis committed rP20100: Deepfreeze Aura, Technology and Resource Templates, Simulation states….Sep 3 2017, 6:37 PM

Harbormaster completed building B2990: rP20100: Deepfreeze Aura, Technology and Resource Templates, Simulation states….Sep 3 2017, 7:24 PM

In game setup, if you first select scenarios, then random, changing the team of any player gives

ERROR: JavaScript error: gui/gamesetup/gamesetup.js line 666
TypeError: "Team" is read-only

g_PlayerDropdowns.playerTeam.select@gui/gamesetup/gamesetup.js:666:4
initDropdown/dropdown.onSelectionChange@gui/gamesetup/gamesetup.js:1100:3

While if you leave the gamesetup and then reenters with random, everything works.

This commit now has outstanding concerns.Sep 4 2017, 6:36 PM

If I see this correctly, then we have found a bug in the code before that commit with this commit.
L666 actually changed the player defaults (in a completely non-obvious way)!
I'm positively surprised - I thought this was a useless freeze.
Don't really want to find out when exactly in the history of the gamesetup that issue was introduced, nor how this would have been noticeable.

(While at it removing an unneeded assignment in the first hunk)

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 20106)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -979,11 +979,11 @@ function initDefaults()
 	{
 		g_DefaultPlayerData[i].Civ = "random";
 		g_DefaultPlayerData[i].Team = -1;
 	}
 
-	g_DefaultPlayerData = deepfreeze(g_DefaultPlayerData);
+	deepfreeze(g_DefaultPlayerData);
 }
 
 /**
  * Sets default values for all g_GameAttribute settings which don't have a value set.
  */
@@ -1541,11 +1541,11 @@ function sanitizePlayerData(playerData)
 	playerData.forEach((pData, index) => {
 
 		// Use defaults if the map doesn't specify a value
 		for (let prop in g_DefaultPlayerData[index])
 			if (!(prop in pData))
-				pData[prop] = g_DefaultPlayerData[index][prop];
+				pData[prop] = clone(g_DefaultPlayerData[index][prop]);
 
 		// Replace colors with the best matching color of PlayerDefaults
 		if (g_GameAttributes.mapType != "scenario")
 		{
 			let colorDistances = g_PlayerColorPickerList.map(color => colorDistance(color, pData.Color));

In rP20100#23070, @elexis wrote:

(While at it removing an unneeded assignment in the first hunk)

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 20106)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -979,11 +979,11 @@ function initDefaults()
 	{
 		g_DefaultPlayerData[i].Civ = "random";
 		g_DefaultPlayerData[i].Team = -1;
 	}
 
-	g_DefaultPlayerData = deepfreeze(g_DefaultPlayerData);
+	deepfreeze(g_DefaultPlayerData);
 }
 
 /**
  * Sets default values for all g_GameAttribute settings which don't have a value set.
  */
@@ -1541,11 +1541,11 @@ function sanitizePlayerData(playerData)
 	playerData.forEach((pData, index) => {
 
 		// Use defaults if the map doesn't specify a value
 		for (let prop in g_DefaultPlayerData[index])
 			if (!(prop in pData))
-				pData[prop] = g_DefaultPlayerData[index][prop];
+				pData[prop] = clone(g_DefaultPlayerData[index][prop]);
 
 		// Replace colors with the best matching color of PlayerDefaults
 		if (g_GameAttributes.mapType != "scenario")
 		{
 			let colorDistances = g_PlayerColorPickerList.map(color => colorDistance(color, pData.Color));

I've just tested this patch, but still have the same problem.

Correct, there are two more clones needed.
This patch should be complete, because the only other reference to g_DefaultPlayerData is in g_PlayerMiscControls.playerName and that isn't modified anywhere (string only used to display the caption of the playername label)..

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 20106)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -427,11 +427,11 @@ var g_Dropdowns = {
 			g_GameAttributes.mapPath = g_MapPath[g_GameAttributes.mapType];
 			delete g_GameAttributes.map;
 
 			if (g_GameAttributes.mapType != "scenario")
 				g_GameAttributes.settings = {
-					"PlayerData": g_DefaultPlayerData.slice(0, 4)
+					"PlayerData": clone(g_DefaultPlayerData.slice(0, 4))
 				};
 
 			reloadMapFilterList();
 		},
 		"autocomplete": 0,
@@ -491,11 +491,11 @@ var g_Dropdowns = {
 		"select": (idx) => {
 			let num = idx + 1;
 			let pData = g_GameAttributes.settings.PlayerData;
 			g_GameAttributes.settings.PlayerData =
 				num > pData.length ?
-					pData.concat(g_DefaultPlayerData.slice(pData.length, num)) :
+					pData.concat(clone(g_DefaultPlayerData.slice(pData.length, num))) :
 					pData.slice(0, num);
 			unassignInvalidPlayers(num);
 			sanitizePlayerData(g_GameAttributes.settings.PlayerData);
 		},
 	},
@@ -979,11 +979,11 @@ function initDefaults()
 	{
 		g_DefaultPlayerData[i].Civ = "random";
 		g_DefaultPlayerData[i].Team = -1;
 	}
 
-	g_DefaultPlayerData = deepfreeze(g_DefaultPlayerData);
+	deepfreeze(g_DefaultPlayerData);
 }
 
 /**
  * Sets default values for all g_GameAttribute settings which don't have a value set.
  */
@@ -1541,11 +1541,11 @@ function sanitizePlayerData(playerData)
 	playerData.forEach((pData, index) => {
 
 		// Use defaults if the map doesn't specify a value
 		for (let prop in g_DefaultPlayerData[index])
 			if (!(prop in pData))
-				pData[prop] = g_DefaultPlayerData[index][prop];
+				pData[prop] = clone(g_DefaultPlayerData[index][prop]);
 
 		// Replace colors with the best matching color of PlayerDefaults
 		if (g_GameAttributes.mapType != "scenario")
 		{
 			let colorDistances = g_PlayerColorPickerList.map(color => colorDistance(color, pData.Color));

I've tested this in alpha 19 and 22 and it indeed revealed a hidden bug!
It's not drastic, but the code isn't doing what was intended.
If we change the colors and teams and swap between scenario and random maps, the modified defaults instead of the defined defaults are applied.

Thaks, that fixes the problem.

All concerns with this commit have now been addressed.Sep 4 2017, 8:26 PM

elexis mentioned this in rP20109: Fix a nearly hidden gamesetup bug that exists since at least alpha 19, revealed….Sep 4 2017, 9:03 PM

elexis mentioned this in D1004: Read-only Gate selection panel issue.Nov 2 2017, 12:38 AM

elexis mentioned this in rP20412: Cleanup gate locking GUI code of rP15375..Nov 5 2017, 5:59 PM