Page MenuHomeWildfire Games

protect possible infinite loop when an order is issued on the same turn as garrisoning is effectively processed
ClosedPublic

Authored by mimo on Jun 27 2017, 7:58 PM.

Details

Reviewers
elexis
s0600204
bb
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19900: protect possible infinite loop when an order is issued on the same turn as…
Summary

see description in comment 7 of #4523: It may happen that an order arrived just at the same turn as the unit is garrisoned, and once garrisoned, units are not supposed to receive any order except Ungarrison if not turret. This may lead to infinite loop as is described in the ticket, with the given commands.txt file. The simplest is to just forget about such orders which are very unlikely

Test Plan

This 19895 replay reproduces the infinite loop and subsequent segfault:


(or adapt the patch to a21 and test on the commands.txt uploaded on #4523 a few days ago.)

A way to reliably reproduce the bug ingame is to assign a control-group to a unit, garrison it, then keep the controlgroup hotkey pressed (to select the unit for some frames) and right clicking an animal.

Event Timeline

mimo created this revision.Jun 27 2017, 7:58 PM
Vulcan added a subscriber: Vulcan.Jun 27 2017, 8:54 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
| 911| 911| 					this.FinishOrder();
| 912| 912| 					return;
| 913| 913| 				}
| 914|    |-				else
| 915|    |-				{
|    | 914|+				
| 916| 915| 					// Out of range; move there in formation
| 917| 916| 					if (this.MoveToGarrisonRange(msg.data.target))
| 918| 917| 					{
| 919| 918| 						this.SetNextState("GARRISON.APPROACHING");
| 920| 919| 						return;
| 921| 920| 					}
| 922|    |-				}
|    | 921|+				
| 923| 922| 			}
| 924| 923| 
| 925| 924| 			this.SetNextState("GARRISON.GARRISONING");
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|1958|1958| 					// TODO: we should probably only bother syncing projectile attacks, not melee
|1959|1959| 
|1960|1960| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|1961|    |-					this.resyncAnimation = (prepare != this.attackTimers.prepare) ? true : false;
|    |1961|+					this.resyncAnimation = (prepare != this.attackTimers.prepare);
|1962|1962| 
|1963|1963| 					this.FaceTowardsTarget(this.order.data.target);
|1964|1964| 
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2184|2184| 							this.PerformGather(nearby, false, false);
|2185|2185| 							return true;
|2186|2186| 						}
|2187|    |-						else
|2188|    |-						{
|    |2187|+						
|2189|2188| 							// It's probably better in this case, to avoid units getting stuck around a dropsite
|2190|2189| 							// in a "Target is far away, full, nearby are no good resources, return to dropsite" loop
|2191|2190| 							// to order it to GatherNear the resource position.
|2206|2205| 									return true;
|2207|2206| 								}
|2208|2207| 							}
|2209|    |-						}
|    |2208|+						
|2210|2209| 						return true;
|2211|2210| 					}
|2212|2211| 					return false;
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2196|2196| 								this.GatherNearPosition(pos.x, pos.z, oldType, oldTemplate);
|2197|2197| 								return true;
|2198|2198| 							}
|2199|    |-							else
|2200|    |-							{
|    |2199|+							
|2201|2200| 								// we're kind of stuck here. Return resource.
|2202|2201| 								var nearby = this.FindNearestDropsite(oldType.generic);
|2203|2202| 								if (nearby)
|2205|2204| 									this.PushOrderFront("ReturnResource", { "target": nearby, "force": false });
|2206|2205| 									return true;
|2207|2206| 								}
|2208|    |-							}
|    |2207|+							
|2209|2208| 						}
|2210|2209| 						return true;
|2211|2210| 					}
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2580|2580| 					this.StartTimer(prepare, this.healTimers.repeat);
|2581|2581| 
|2582|2582| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|2583|    |-					this.resyncAnimation = (prepare != this.healTimers.prepare) ? true : false;
|    |2583|+					this.resyncAnimation = (prepare != this.healTimers.prepare);
|2584|2584| 
|2585|2585| 					this.FaceTowardsTarget(this.order.data.target);
|2586|2586| 				},
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|3396|3396| 
|3397|3397| UnitAI.prototype.IsAnimal = function()
|3398|3398| {
|3399|    |-	return (this.template.NaturalBehaviour ? true : false);
|    |3399|+	return (!!this.template.NaturalBehaviour);
|3400|3400| };
|3401|3401| 
|3402|3402| UnitAI.prototype.IsDangerousAnimal = function()
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|3704|3704| 		// Otherwise we've successfully processed a new order
|3705|3705| 		return true;
|3706|3706| 	}
|3707|    |-	else
|3708|    |-	{
|    |3707|+	
|3709|3708| 		this.SetNextState("IDLE");
|3710|3709| 
|3711|3710| 		Engine.PostMessage(this.entity, MT_UnitAIOrderDataChanged, { "to": this.GetOrderData() });
|3726|3725| 		}
|3727|3726| 
|3728|3727| 		return false;
|3729|    |-	}
|    |3728|+	
|3730|3729| };
|3731|3730| 
|3732|3731| /**
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4756|4756| 	// If we are guarding/escorting, don't abandon as long as the guarded unit is in target range of the attacker
|4757|4757| 	if (this.isGuardOf)
|4758|4758| 	{
|4759|    |-		var cmpUnitAI =  Engine.QueryInterface(target, IID_UnitAI);
|    |4759|+		var cmpUnitAI = Engine.QueryInterface(target, IID_UnitAI);
|4760|4760| 		var cmpAttack = Engine.QueryInterface(target, IID_Attack);
|4761|4761| 		if (cmpUnitAI && cmpAttack &&
|4762|4762| 		    cmpAttack.GetAttackTypes().some(type => cmpUnitAI.CheckTargetAttackRange(this.isGuardOf, type)))
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4803|4803| 	// If we are guarding/escorting, chase at least as long as the guarded unit is in target range of the attacker
|4804|4804| 	if (this.isGuardOf)
|4805|4805| 	{
|4806|    |-		var cmpUnitAI =  Engine.QueryInterface(target, IID_UnitAI);
|    |4806|+		var cmpUnitAI = Engine.QueryInterface(target, IID_UnitAI);
|4807|4807| 		var cmpAttack = Engine.QueryInterface(target, IID_Attack);
|4808|4808| 		if (cmpUnitAI && cmpAttack &&
|4809|4809| 		    cmpAttack.GetAttackTypes().some(type => cmpUnitAI.CheckTargetAttackRange(this.isGuardOf, type)))
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4976|4976| 	{
|4977|4977| 		if (this.isGuardOf == target && this.order && this.order.type == "Guard")
|4978|4978| 			return;
|4979|    |-		else
|4980|    |-			this.RemoveGuard();
|    |4979|+		this.RemoveGuard();
|4981|4980| 	}
|4982|4981| 
|4983|4982| 	this.AddOrder("Guard", { "target": target, "force": false }, queued);
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'lastPos' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|5224|5224| 
|5225|5225| 	// Remember the position of our target, if any, in case it disappears
|5226|5226| 	// later and we want to head to its last known position
|5227|    |-	var lastPos = undefined;
|    |5227|+	var lastPos;
|5228|5228| 	var cmpPosition = Engine.QueryInterface(target, IID_Position);
|5229|5229| 	if (cmpPosition && cmpPosition.IsInWorld())
|5230|5230| 		lastPos = cmpPosition.GetPosition();

binaries/data/mods/public/simulation/components/UnitAI.js
|2482| »   »   »   »   »   »   let·nearby·=·this.FindNearestDropsite(resourceType.generic);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'nearby' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3887| »   var·isWorkType·=·type·=>·type·==·"Gather"·||·type·==·"Trade"·||·type·==·"Repair"·||·type·==·"ReturnResource";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4205| »   »   let·cmpOwnership·=·Engine.QueryInterface(ent,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4678| »   var·target·=·ents.find(target·=>·this.CanAttack(target,·forceResponse));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4693| »   var·target·=·ents.find(target·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4739| »   var·ent·=·ents.find(ent·=>·this.CanHeal(ent));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4762| »   »   ····cmpAttack.GetAttackTypes().some(type·=>·cmpUnitAI.CheckTargetAttackRange(this.isGuardOf,·type)))
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
| 363| »   »   ····&&·(this.lastShorelinePosition.z·==·cmpPosition.GetPosition().z))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|1951| »   »   »   »   »   »   var·cmpFormation·=·Engine.QueryInterface(this.formationController,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2177| »   »   »   »   »   »   »   »   ·&&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2178| »   »   »   »   »   »   »   »   ·||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2179| »   »   »   »   »   »   »   »   ·&&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2202| »   »   »   »   »   »   »   »   var·nearby·=·this.FindNearestDropsite(oldType.generic);
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2242| »   »   »   »   »   »   »   »   &&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2243| »   »   »   »   »   »   »   »   ||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2244| »   »   »   »   »   »   »   »   &&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2288| »   »   »   »   »   »   »   ||·(type.specific·==·resourceType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2289| »   »   »   »   »   »   »   &&·(type.specific·!=·"meat"·||·resourceTemplate·==·template))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2305| »   »   »   »   »   var·nearby·=·this.FindNearestDropsite(resourceType.generic);
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2477| »   »   »   »   »   var·cmpResourceGatherer·=·Engine.QueryInterface(this.entity,·IID_ResourceGatherer);
|    | [NORMAL] JSHintBear:
|    | 'cmpResourceGatherer' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2493| »   »   »   »   »   var·nearby·=·this.FindNearbyResource(function·(ent,·type,·template)·{
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2496| »   »   »   »   »   »   »   ||·(type.specific·==·resourceType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2497| »   »   »   »   »   »   »   &&·(type.specific·!=·"meat"·||·resourceTemplate·==·template))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2517| »   »   »   »   »   var·nearby·=·this.FindNearestDropsite(resourceType.generic);
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2701| »   »   »   »   »   var·cmpResourceGatherer·=·Engine.QueryInterface(this.entity,·IID_ResourceGatherer);
|    | [NORMAL] JSHintBear:
|    | 'cmpResourceGatherer' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2896| »   »   »   »   »   var·cmpResourceDropsite·=·Engine.QueryInterface(msg.data.newentity,·IID_ResourceDropsite);
|    | [NORMAL] JSHintBear:
|    | 'cmpResourceDropsite' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2991| »   »   »   »   »   »   var·target·=·this.alertGarrisoningTarget;
|    | [NORMAL] JSHintBear:
|    | 'target' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2995| »   »   »   »   »   if·(this.CanGarrison(target))
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|2998| »   »   »   »   »   »   if·(this.CheckGarrisonRange(target))
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3000| »   »   »   »   »   »   »   var·cmpGarrisonHolder·=·Engine.QueryInterface(target,·IID_GarrisonHolder);
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3022| »   »   »   »   »   »   »   »   var·cmpResourceDropsite·=·Engine.QueryInterface(target,·IID_ResourceDropsite);
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3023| »   »   »   »   »   »   »   »   if·(cmpResourceDropsite·&&·this.CanReturnResource(target,·true))
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3038| »   »   »   »   »   »   »   »   »   var·cmpHolderPosition·=·Engine.QueryInterface(target,·IID_Position);
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3039| »   »   »   »   »   »   »   »   »   var·cmpHolderUnitAI·=·Engine.QueryInterface(target,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3066| »   »   »   »   »   »   »   if·(this.MoveToTarget(target))
|    | [NORMAL] JSHintBear:
|    | 'target' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3363| »   return·this.alertRaiser·!=·undefined;
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3494| »   »   »   ||·this.orderQueue[0].type·==·"Pack"·||·this.orderQueue[0].type·==·"Unpack")))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|3558| »   »   if·(i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3849| »   »   var·order·=·{·"type":·type,·"data":·data·};
|    | [NORMAL] JSHintBear:
|    | 'order' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3918| »   for·(var·i·=·0;·i·<·this.orderQueue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3930| »   if·(this.workOrders.length·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/UnitAI.js
|4109| »   return·cmpHealth·&&·cmpHealth.GetHitpoints()·!=·0;
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/UnitAI.js
|4448| »   »   var·parabolicMaxRange·=·0;
|    | [NORMAL] JSHintBear:
|    | 'parabolicMaxRange' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|4452| »   var·guessedMaxRange·=·(range.max·+·parabolicMaxRange)/2;
|    | [NORMAL] JSHintBear:
|    | 'parabolicMaxRange' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4459| »   return·cmpUnitMotion.MoveToTargetRange(target,·range.min,·Math.min(range.max,·parabolicMaxRange));
|    | [NORMAL] JSHintBear:
|    | 'parabolicMaxRange' used out of scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4515| »   »   »   &&·cmpFormationUnitAI.order.data.target·==·target)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|4695| »   »   &&·this.CheckTargetDistanceFromHeldPosition(target,·IID_Attack,·this.GetBestAttackAgainst(target,·true))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|4696| »   »   &&·(this.GetStance().respondChaseBeyondVision·||·this.CheckTargetIsInVisionRange(target))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|5227| »   var·lastPos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'lastPos' to 'undefined'.

binaries/data/mods/public/simulation/components/UnitAI.js
|5566| »   »   »   »   »   »   &&·!MatchesClassList(cmpIdentity.GetClassesList(),·targetClasses.attack))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|5569| »   »   »   »   »   »   &&·MatchesClassList(cmpIdentity.GetClassesList(),·targetClasses.avoid))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|5582| »   var·targets·=·this.GetTargetsFromUnit();
|    | [NORMAL] JSHintBear:
|    | 'targets' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5583| »   for·(var·targ·of·targets)
|    | [NORMAL] JSHintBear:
|    | 'targ' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5589| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(targ,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5590| »   »   »   var·targetClasses·=·this.order.data.targetClasses;
|    | [NORMAL] JSHintBear:
|    | 'targetClasses' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5592| »   »   »   »   &&·!MatchesClassList(cmpIdentity.GetClassesList(),·targetClasses.attack))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|5595| »   »   »   »   &&·MatchesClassList(cmpIdentity.GetClassesList(),·targetClasses.avoid))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|5595| »   »   »   »   &&·MatchesClassList(cmpIdentity.GetClassesList(),·targetClasses.avoid))
|    | [MAJOR] JSHintBear:
|    | Too many errors. (91% scanned).
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/248/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1643/ for more details.

leper added a reviewer: Restricted Owners Package.Jun 29 2017, 8:56 PM
leper added a subscriber: leper.
leper added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
4951

This seems like it breaks with queud orders. Eg queue a lot of orders, one of them being a garrison order, then some more afterwards (assuming the building has no rally points set), so when you ungarrison the unit it will continue with those orders.

Ah, actually that seems broken since Ungarrison does a replace order instead of PushOrderFront, possibly in conjunction with finishing the current (garrison) order.

I'm mostly wondering whether this early return should just be done in case this isn't a queued order.

mimo added inline comments.Jun 29 2017, 10:00 PM
binaries/data/mods/public/simulation/components/UnitAI.js
4951

Yes, as Ungarrison does a replace order, it does not matter if we add queued orders as they will anyway be lost during the ungarrisoning. We may nonetheless only do the early return in the not queued case if we think we may change that behaviour one day in the future (the bug from the trac ticket will still be fixed because it is how i first wrote and tested the patch).

mimo updated this revision to Diff 2764.Jun 29 2017, 10:04 PM
mimo edited the summary of this revision. (Show Details)
mimo edited the test plan for this revision. (Show Details)

update patch

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
| 911| 911| 					this.FinishOrder();
| 912| 912| 					return;
| 913| 913| 				}
| 914|    |-				else
| 915|    |-				{
|    | 914|+				
| 916| 915| 					// Out of range; move there in formation
| 917| 916| 					if (this.MoveToGarrisonRange(msg.data.target))
| 918| 917| 					{
| 919| 918| 						this.SetNextState("GARRISON.APPROACHING");
| 920| 919| 						return;
| 921| 920| 					}
| 922|    |-				}
|    | 921|+				
| 923| 922| 			}
| 924| 923| 
| 925| 924| 			this.SetNextState("GARRISON.GARRISONING");
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|1958|1958| 					// TODO: we should probably only bother syncing projectile attacks, not melee
|1959|1959| 
|1960|1960| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|1961|    |-					this.resyncAnimation = (prepare != this.attackTimers.prepare) ? true : false;
|    |1961|+					this.resyncAnimation = (prepare != this.attackTimers.prepare);
|1962|1962| 
|1963|1963| 					this.FaceTowardsTarget(this.order.data.target);
|1964|1964| 
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2184|2184| 							this.PerformGather(nearby, false, false);
|2185|2185| 							return true;
|2186|2186| 						}
|2187|    |-						else
|2188|    |-						{
|    |2187|+						
|2189|2188| 							// It's probably better in this case, to avoid units getting stuck around a dropsite
|2190|2189| 							// in a "Target is far away, full, nearby are no good resources, return to dropsite" loop
|2191|2190| 							// to order it to GatherNear the resource position.
|2206|2205| 									return true;
|2207|2206| 								}
|2208|2207| 							}
|2209|    |-						}
|    |2208|+						
|2210|2209| 						return true;
|2211|2210| 					}
|2212|2211| 					return false;
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2196|2196| 								this.GatherNearPosition(pos.x, pos.z, oldType, oldTemplate);
|2197|2197| 								return true;
|2198|2198| 							}
|2199|    |-							else
|2200|    |-							{
|    |2199|+							
|2201|2200| 								// we're kind of stuck here. Return resource.
|2202|2201| 								var nearby = this.FindNearestDropsite(oldType.generic);
|2203|2202| 								if (nearby)
|2205|2204| 									this.PushOrderFront("ReturnResource", { "target": nearby, "force": false });
|2206|2205| 									return true;
|2207|2206| 								}
|2208|    |-							}
|    |2207|+							
|2209|2208| 						}
|2210|2209| 						return true;
|2211|2210| 					}
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2580|2580| 					this.StartTimer(prepare, this.healTimers.repeat);
|2581|2581| 
|2582|2582| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|2583|    |-					this.resyncAnimation = (prepare != this.healTimers.prepare) ? true : false;
|    |2583|+					this.resyncAnimation = (prepare != this.healTimers.prepare);
|2584|2584| 
|2585|2585| 					this.FaceTowardsTarget(this.order.data.target);
|2586|2586| 				},
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|3396|3396| 
|3397|3397| UnitAI.prototype.IsAnimal = function()
|3398|3398| {
|3399|    |-	return (this.template.NaturalBehaviour ? true : false);
|    |3399|+	return (!!this.template.NaturalBehaviour);
|3400|3400| };
|3401|3401| 
|3402|3402| UnitAI.prototype.IsDangerousAnimal = function()
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|3704|3704| 		// Otherwise we've successfully processed a new order
|3705|3705| 		return true;
|3706|3706| 	}
|3707|    |-	else
|3708|    |-	{
|    |3707|+	
|3709|3708| 		this.SetNextState("IDLE");
|3710|3709| 
|3711|3710| 		Engine.PostMessage(this.entity, MT_UnitAIOrderDataChanged, { "to": this.GetOrderData() });
|3726|3725| 		}
|3727|3726| 
|3728|3727| 		return false;
|3729|    |-	}
|    |3728|+	
|3730|3729| };
|3731|3730| 
|3732|3731| /**
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4756|4756| 	// If we are guarding/escorting, don't abandon as long as the guarded unit is in target range of the attacker
|4757|4757| 	if (this.isGuardOf)
|4758|4758| 	{
|4759|    |-		var cmpUnitAI =  Engine.QueryInterface(target, IID_UnitAI);
|    |4759|+		var cmpUnitAI = Engine.QueryInterface(target, IID_UnitAI);
|4760|4760| 		var cmpAttack = Engine.QueryInterface(target, IID_Attack);
|4761|4761| 		if (cmpUnitAI && cmpAttack &&
|4762|4762| 		    cmpAttack.GetAttackTypes().some(type => cmpUnitAI.CheckTargetAttackRange(this.isGuardOf, type)))
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4803|4803| 	// If we are guarding/escorting, chase at least as long as the guarded unit is in target range of the attacker
|4804|4804| 	if (this.isGuardOf)
|4805|4805| 	{
|4806|    |-		var cmpUnitAI =  Engine.QueryInterface(target, IID_UnitAI);
|    |4806|+		var cmpUnitAI = Engine.QueryInterface(target, IID_UnitAI);
|4807|4807| 		var cmpAttack = Engine.QueryInterface(target, IID_Attack);
|4808|4808| 		if (cmpUnitAI && cmpAttack &&
|4809|4809| 		    cmpAttack.GetAttackTypes().some(type => cmpUnitAI.CheckTargetAttackRange(this.isGuardOf, type)))
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|4977|4977| 	{
|4978|4978| 		if

http://jw:8080/job/phabricator_lint/269/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1669/ for more details.

elexis edited the test plan for this revision. (Show Details)Jul 10 2017, 4:37 AM
elexis edited the test plan for this revision. (Show Details)Jul 10 2017, 4:45 AM
elexis accepted this revision.Jul 10 2017, 5:13 AM
elexis added a subscriber: elexis.

s/only/
s/a weird state/an infinite loop

The turret check is not needed, is it? (or do we consider turrets being able to garrison?)

No opinion on the queue checking (if units are garrisoned they can be expected to lose their queue, though that might not be a clean solution).

Fixes the second currently known way to trigger the infinite loop, there may still be more unless someone proves otherwise, we can see in Alpha 23 if someone accidentally finds another.

This revision is now accepted and ready to land.Jul 10 2017, 5:13 AM
s0600204 accepted this revision.Jul 10 2017, 7:15 AM
s0600204 added subscribers: causative, s0600204.

(Started this several hours before elexis' above comment and modification of the test plan.)

I was initially (and suppose, still am) unable to reproduce the segfault problem reported in #4523 using svn/A22 with any (unmodified) replays posted on that ticket, nor using either of the "can reliably cause the segfault by..." instructions by @causative.

I was able to reproduce the segfault problem in A21 using causative's "script console" method, or by running a non-visual replay.

However, running A21 with the changes contained within this revision applied as well:

  • Replaying Franksy's commands.txt file, no segfault.
  • Replaying causative's "concise" file, segfaults.
  • Using causative's "script console" method, segfaults.

However, I reasoned I can probably ignore these last two, as these are caused by the back-to-work command calling AddOrders() rather than AddOrder() and that case was fixed by D685.

I was, however, a little curious why Franksy's replay no longer segfaults in A22.

So I traced the point at which the replay from Franksy stops segfaulting - r18953. A revision earlier, and a non-visual replay segfaults. On this revision (or, seemingly, any later one) and it doesn't. The replay itself is that of a random map generation, and r18953 contains small alterations to how some random maps are generated. Running a visual replay shows the difference - in r18952 (and prior), the citizen-cavalry targets a chicken. Thanks to the changes made in r18953, the entity number of the chicken is instead given to a tree. Thus the citizen-cavalry is not in the same place as before when the conflicting garrison/gather commands are sent.

Returning to A22, I amended causative's replay to use a gather command instead of a back-to-work command at the point of potential conflict, and sure enough - segfault.

And with the changes of this revision applied - no segfault. Yay!


@elexis wrote:

or do we consider turrets being able to garrison?

Going by the code of the IsTurret() function, turrets must be garrisoned or else they're not turrets...

(Replays can become super-OOS with every commit. Just a unit changing it's position by a fraction, an entity doing something a fraction of a second later or something costing more will all make a replay / game out-of-sync. I was wondering whether we should add hashing to singleplayer games for that reason too, so that people have a way to find out that their replay is OOS (#4202), maybe only every N turns)

bb accepted this revision.Jul 10 2017, 4:44 PM
bb added a subscriber: bb.

When investigating further in this issue, the proposed patch indeed fixes the issue, but it does remove the actual infinite loop. It only suspends it for garrisoned units, the actual loop happens when an unit is unable to reach a gather target that needs to be killed (f.e. being garrisoned). We will then pushOrderFront attack orders upon the unit, which will return a finishOrder() only. So this is exactly the same issue described in D64.
IMO both fixes should find its way into the code, since an garrisoned (nonturret) unit should receive any order AND there are more reason why a target can be unreachable, so both fixes are correct and necessary.

Was slightly wondering about a turret whose garrisonholder gets garrisoned, but then IsGarrsoned() should return true anyway.

elexis edited the test plan for this revision. (Show Details)Jul 10 2017, 4:52 PM
This revision was automatically updated to reflect the committed changes.