Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3647 closed defect (fixed)

[PATCH] Some commands get modified by a simulation update

Reported by: elexis Owned by: leper
Priority: Should Have Milestone: Alpha 20
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by elexis)

Some actions in Commands.js modify the commands. This is usually not an issue, but can cause false positives OOS in serializationtest and might fail oddly somewhere down the line.

r17324 was a fix for one such case. There are possibly others.

A fix would be making sure we detect those problems (at least when running serializationtest), and fixing the cases where it happens.

Attachments (1)

clone_commands_for_serializationtest_v1.patch (6.3 KB ) - added by elexis 8 years ago.
Fixes the bug, but should be optimized performance-wise as this patch initializes the secondary context and component manager in non-serializationtestmode too.

Download all attachments as: .zip

Change History (15)

by elexis, 8 years ago

Fixes the bug, but should be optimized performance-wise as this patch initializes the secondary context and component manager in non-serializationtestmode too.

comment:1 by historic_bruno, 8 years ago

Why is the command modified in the first place? That seems undesirable.

comment:2 by wraitii, 8 years ago

I don't think your fix is the proper solution here (it would be handled better as part of a complete overhaul of the system imo, such as the RM fix).

The command change is indeed a bug in TryConstructWall where instead of locally modifying a boolean we modify the command "queued" parameter. That should be changed.

comment:3 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

Fixed alternatively by r17324.

comment:4 by historic_bruno, 8 years ago

It's an interesting thing to note however, perhaps somewhere in the docs, in case a similar bug pops up in the future.

I had a peek at TryConstructBuilding and it modifies cmd.angle for docks, so we may need to fix that as well: ps/trunk/binaries/data/mods/public/simulation/helpers/Commands.js?rev=17324#L890 Does that cause serialization test failure?

in reply to:  4 comment:5 by elexis, 8 years ago

Resolution: fixed
Status: closedreopened

Replying to historic_bruno:

we may need to fix that as well

An attack order also changes the command object: cmd.allowCapture = true;

Even if this and the dock doesn't OOS, it is just a matter of time until we will run into this issue again. We should fix the serializationtest as it's the only place where we can guarantee that the false-positive serializationtesterror won't repeat.

comment:6 by historic_bruno, 8 years ago

Alternatively, the serialization test is working correctly and detecting broken logic, which we would only be covering up by changing it :)

comment:7 by wraitii, 8 years ago

Description: modified (diff)
Keywords: patch removed
Priority: Must HaveShould Have
Summary: [PATCH] Serializationtest error when placing wallsSome commands get modified by a simulation update

I agree with historic_bruno. My suggestion to fix it would be custom-checking in the serialization test that the commands don't change, and if they change, crash with an obvious message and perhaps the changed command to make debugging easier.

We also should fix the broken logic in the places you mentioned.

comment:8 by mimo, 8 years ago

r17441 fixed these two cmd modifications

comment:9 by leper, 8 years ago

Owner: set to leper
Resolution: fixed
Status: reopenedclosed

In 17673:

Deep freeze simulation commands to prevent accidental updates. Fixes #3647.

comment:10 by elexis, 8 years ago

Keywords: patch added
Summary: Some commands get modified by a simulation update[PATCH] Some commands get modified by a simulation update

Two more places were identified where the command object was modified: 1) Non-visual replays: #3751. 2) The cheat "back to the future":

ERROR: JavaScript error: simulation/helpers/Cheat.js line 88
TypeError: "parameter" is read-only
  Cheat@simulation/helpers/Cheat.js:88:3
  g_Commands.cheat@simulation/helpers/Commands.js:66:3
  ProcessCommand@simulation/helpers/Commands.js:37:3
ERROR: Failed to call ProcessCommand() global script function

comment:11 by elexis, 8 years ago

In 17712:

Don't modify the command object. Refs #3647.

comment:12 by mimo, 8 years ago

In 17715:

fix wall buildings after r17673, refs #3647

comment:13 by elexis, 7 years ago

Description: modified (diff)

The issue of modifying a cached object occured in at least one place of the GUI and was fixed by a clone in r18788. To detect these things in the future similar to r17673 and r17711, we must add the freezing function to JS objects too: #4257.

comment:14 by elexis, 7 years ago

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

Note: See TracTickets for help on using tickets.