Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash with an OXP (Equipment by ship class) #204

Closed
perror opened this Issue Aug 15, 2016 · 38 comments

Comments

Projects
None yet
3 participants
@perror
Copy link

perror commented Aug 15, 2016

I get a segfault each time I have the following OXP active in my list:

oolite.oxp.redspear.equipment_by_ship_class.oxz

Seems that the Javascript engine does not like it.
If you cannot reproduce the bug, tell me what information you need.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 15, 2016

Seems to run fine here under Windows. Please provide:

  • Latest.log following a crash, after having restarted the game with Shift down.
  • If on Linux, distro and version and whether you are running the binary from oolite.org or a binary provided by your distro.
  • A specific and reproducible way of generating the crash.

Also, please check if it crashes when only that OXP is loaded and nothing else.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 15, 2016

Crashed here on applying changes when installing. Here's a backtrace:

#0  0x00007fcfc3d58b89 in objc_msg_lookup (receiver=0x6138b40, op=0xf8eef0 <_OBJC_SELECTOR_TABLE+5552>)
    at /build/gcc-6-4nfnFV/gcc-6-6.1.1/src/libobjc/sendmsg.c:442
#1  0x00000000004f750d in -[ShipEntity hasOneEquipmentItem:includeMissiles:whileLoading:] (self=0x27dae10, 
    _cmd=0xf8eec0 <_OBJC_SELECTOR_TABLE+5504>, itemKey=0x935d710, includeMissiles=1 '\001', loading=0 '\000')
    at src/Core/Entities/ShipEntity.m:3047
#2  0x00000000004f72e7 in -[ShipEntity hasOneEquipmentItem:includeWeapons:whileLoading:] (self=0x27dae10, 
    _cmd=0xf8ef10 <_OBJC_SELECTOR_TABLE+5584>, itemKey=0x935d710, includeWeapons=1 '\001', loading=0 '\000')
    at src/Core/Entities/ShipEntity.m:3009
#3  0x00000000004f7da8 in -[ShipEntity hasEquipmentItem:includeWeapons:whileLoading:] (self=0x27dae10, 
    _cmd=0xf8ef20 <_OBJC_SELECTOR_TABLE+5600>, equipmentKeys=0x61390b0, includeWeapons=1 '\001', loading=0 '\000')
    at src/Core/Entities/ShipEntity.m:3106
#4  0x00000000004f9622 in -[ShipEntity equipmentValidToAdd:whileLoading:inContext:] (self=0x27dae10, _cmd=0xf8f050 <_OBJC_SELECTOR_TABLE+5904>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, loading=0 '\000', context=0xf83fa0 <_OBJC_INSTANCE_148>) at src/Core/Entities/ShipEntity.m:3415
#5  0x00000000004f9233 in -[ShipEntity equipmentValidToAdd:inContext:] (self=0x27dae10, _cmd=0xf8efb0 <_OBJC_SELECTOR_TABLE+5744>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, context=0xf83fa0 <_OBJC_INSTANCE_148>) at src/Core/Entities/ShipEntity.m:3382
#6  0x00000000004f8999 in -[ShipEntity canAddEquipment:inContext:] (self=0x27dae10, _cmd=0xf4fc90 <_OBJC_SELECTOR_TABLE+12848>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, context=0xf83fa0 <_OBJC_INSTANCE_148>) at src/Core/Entities/ShipEntity.m:3226
#7  0x000000000049125c in -[PlayerEntity canAddEquipment:inContext:] (self=0x27dae10, _cmd=0xf8f240 <_OBJC_SELECTOR_TABLE+6400>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, context=0xf83fa0 <_OBJC_INSTANCE_148>) at src/Core/Entities/PlayerEntity.m:11059
#8  0x00000000004f9f29 in -[ShipEntity addEquipmentItem:withValidation:inContext:] (self=0x27dae10, _cmd=0xf4ef20 <_OBJC_SELECTOR_TABLE+9408>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, validateAddition=1 '\001', context=0xf83fa0 <_OBJC_INSTANCE_148>)
    at src/Core/Entities/ShipEntity.m:3533
#9  0x00000000004914ce in -[PlayerEntity addEquipmentItem:withValidation:inContext:] (self=0x27dae10, _cmd=0xf4ef20 <_OBJC_SELECTOR_TABLE+9408>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, validateAddition=1 '\001', context=0xf83fa0 <_OBJC_INSTANCE_148>)
    at src/Core/Entities/PlayerEntity.m:11094
#10 0x000000000049136c in -[PlayerEntity addEquipmentItem:inContext:] (self=0x27dae10, _cmd=0xf8db40 <_OBJC_SELECTOR_TABLE+512>, 
    equipmentKey=0xf83fe0 <_OBJC_INSTANCE_150>, context=0xf83fa0 <_OBJC_INSTANCE_148>) at src/Core/Entities/PlayerEntity.m:11070
#11 0x00000000004e7b2d in -[ShipEntity setUpFromDictionary:] (self=0x27dae10, _cmd=0xf4da00 <_OBJC_SELECTOR_TABLE+4000>, shipDict=0x71d8f50)
    at src/Core/Entities/ShipEntity.m:331
#12 0x000000000045fc56 in -[PlayerEntity setUpShipFromDictionary:] (self=0x27dae10, _cmd=0x10ee370 <_OBJC_SELECTOR_TABLE+10480>, 
    shipDict=0x71d8f50) at src/Core/Entities/PlayerEntity.m:2190
#13 0x0000000000745cd2 in -[Universe setUpInitialUniverse] (self=0x2662820, _cmd=0x10ebdb0 <_OBJC_SELECTOR_TABLE+816>)
    at src/Core/Universe.m:9905
#14 0x0000000000745708 in -[Universe reinitAndShowDemo:] (self=0x2662820, _cmd=0x10151c0 <_OBJC_SELECTOR_TABLE+2272>, showDemo=1 '\001')
    at src/Core/Universe.m:9847
#15 0x00000000005fd7ce in -[OOOXZManager isRestarting] (self=0x211cb30, _cmd=0xf5e850 <_OBJC_SELECTOR_TABLE+6288>)
    at src/Core/OOOXZManager.m:1299
#16 0x00000000004bfb2f in -[PlayerEntity(OOControlsPrivate) pollDemoControls:] (self=0x27dae10, _cmd=0xf5d1c0 <_OBJC_SELECTOR_TABLE+512>, 
    delta_t=0.016650974750518799) at src/Core/Entities/PlayerEntityControls.m:4369
#17 0x00000000004ab080 in -[PlayerEntity(Controls) pollControls:] (self=0x27dae10, _cmd=0xf4dbe0 <_OBJC_SELECTOR_TABLE+4480>, 
    delta_t=0.016650974750518799) at src/Core/Entities/PlayerEntityControls.m:389
#18 0x0000000000462194 in -[PlayerEntity update:] (self=0x27dae10, _cmd=0x10edab0 <_OBJC_SELECTOR_TABLE+8240>, delta_t=0.016650974750518799)
    at src/Core/Entities/PlayerEntity.m:2535
#19 0x0000000000736b29 in -[Universe update:] (self=0x2662820, _cmd=0x10bc970 <_OBJC_SELECTOR_TABLE+784>, inDeltaT=0.016650974750518799)
    at src/Core/Universe.m:6625
#20 0x00000000006e52f4 in -[GameController doPerformGameTick] (self=0x20b5790, _cmd=0x10bc960 <_OBJC_SELECTOR_TABLE+768>)
    at src/Core/GameController.m:350
#21 0x00000000006e51d2 in -[GameController performGameTick:] (self=0x20b5790, _cmd=0x10bc9d0 <_OBJC_SELECTOR_TABLE+880>, sender=0x66a9c90)
    at src/Core/GameController.m:328
#22 0x00007fcfc5bbaca6 in -[NSTimer fire] (self=0x66a9c90, _cmd=<optimized out>) at NSTimer.m:260
#23 0x00007fcfc5b87ee0 in -[NSRunLoop limitDateForMode:] (self=0x20e14f0, _cmd=0x7fcfc6039c90 <_OBJC_SELECTOR_TABLE+1232>, 
    mode=0x7fcfc603aa70 <_OBJC_INSTANCE_2>) at NSRunLoop.m:1031
#24 0x00007fcfc5b87764 in -[NSRunLoop runMode:beforeDate:] (self=0x20e14f0, _cmd=<optimized out>, mode=0x7fcfc603aa70 <_OBJC_INSTANCE_2>, 
    date=0x20ea580) at NSRunLoop.m:1274
#25 0x00007fcfc5b86fc5 in -[NSRunLoop runUntilDate:] (self=0x20e14f0, _cmd=0x7fcfc6039cc0 <_OBJC_SELECTOR_TABLE+1280>, date=0x20ea580)
    at NSRunLoop.m:1323
#26 0x00000000006e4f09 in -[GameController applicationDidFinishLaunching:] (self=0x20b5790, _cmd=0x10c1980 <_OBJC_SELECTOR_TABLE+96>, 
    notification=0x0) at src/Core/GameController.m:279
#27 0x00000000006ed6cb in main (argc=1, argv=0x7ffc3b7f04a8) at src/SDL/main.m:113

Last bit of the log:

18:18:29.812 [shipData.load.begin]: Loading ship data.
18:18:29.829 [plist.parse.failed]: Failed to parse /home/kevin/GNUstep/Library/ApplicationSupport/Oolite/ManagedAddOns/oolite.oxp.redspear.equipment_by_ship_class.oxz/Config/shipyard-overrides.plist as a property list.
Parse failed at line 254 (char 5290) - reached end of string
18:18:30.129 [script.javascript.init]: JavaScript reset successful.
18:18:30.200 [texture.load.png.warning]: ----- A PNG loading warning occurred for /home/kevin/git/oolite/oolite.app/Resources/Textures/trumblekit.png: iCCP: profile 'ICC Profile': 'RGB ': RGB color space not permitted on grayscale PNG.

I'm wondering if it's another problem with the latest GNUstep version.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 15, 2016

If it runs with the oolite.org binary, then we can assume it is a GNUstep issue. Which version of the oxz are you running?

Looking at my log, I am also getting this message:

20:13:26.058 [shipData.load.begin]: Loading ship data.
20:13:26.158 [plist.parse.failed]: Failed to parse C:/OoliteProject/oolite.app/GNUstep/Library/ApplicationSupport/Oolite/ManagedAddOns/oolite.oxp.redspear.equipment_by_ship_class.oxz/Config/shipyard-overrides.plist as a property list.
Parse failed at line 254 (char 5290) - reached end of string

However, the application seems to survive for some reason.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 15, 2016

Inspecting the variables in gdb. At the crash:

(gdb) print *(missile_list[0])
$8 = {{isa = 0x0}, _techLevel = 65, _price = 111650776, _name = 0x0, _identifier = 0x0, _description = 0x0, _isAvailableToAll = 0,
  _requiresEmptyPylon = 0, _requiresMountedPylon = 0, _requiresClean = 0, _requiresNotClean = 0, _portableBetweenShips = 0,
  _requiresFreePassengerBerth = 0, _requiresFullFuel = 0, _requiresNonFullFuel = 0, _isMissileOrMine = 0, _isVisible = 0,
  _isAvailableToPlayer = 0, _isAvailableToNPCs = 0, _fastAffinityA = 0, _fastAffinityB = 0, _canCarryMultiple = 0, _installTime = 0,
  _repairTime = 0, _damageProbability = 2.70450604e-43, _requiredCargoSpace = 0, _requiresEquipment = 0x0, _requiresAnyEquipment = 0x400000000,
  _incompatibleEquipment = 0x10d0da0 <_OBJC_Class_OOEquipmentType>, _conditions = 0x8, _provides = 0x30d4, _scriptInfo = 0x6a6f5d0,
  _weaponInfo = 0x6a6f670, _script = 0x6a6ec60, _condition_script = 0x1c01, _jsSelf = 0x0}
(gdb) print [missile_list[0] identifier]
Can't find selector "identifier"

At a breakpoint with the OXP removed:

(gdb) print *(missile_list[0])
[Thread 0x7fffaf7fe700 (LWP 8615) exited]
[Thread 0x7fffaeffd700 (LWP 8616) exited]
$5 = {{isa = 0x10d0da0 <_OBJC_Class_OOEquipmentType>}, _techLevel = 1, _price = 300, _name = 0x83e6360, _identifier = 0x83e63a0, 
  _description = 0x83e63e0, _isAvailableToAll = 1, _requiresEmptyPylon = 1, _requiresMountedPylon = 0, _requiresClean = 0, 
  _requiresNotClean = 0, _portableBetweenShips = 0, _requiresFreePassengerBerth = 0, _requiresFullFuel = 0, _requiresNonFullFuel = 0, 
  _isMissileOrMine = 1, _isVisible = 1, _isAvailableToPlayer = 1, _isAvailableToNPCs = 1, _fastAffinityA = 0, _fastAffinityB = 0, 
  _canCarryMultiple = 0, _installTime = 0, _repairTime = 0, _damageProbability = 0, _requiredCargoSpace = 0, _requiresEquipment = 0x0, 
  _requiresAnyEquipment = 0x0, _incompatibleEquipment = 0x0, _conditions = 0x0, _provides = 0x5427c50, _scriptInfo = 0x0, 
  _weaponInfo = 0x5428f30, _script = 0x0, _condition_script = 0x83e6630, _jsSelf = 0x0}
(gdb) print [missile_list[i] identifier]
$6 = (struct NSString *) 0x83e63a0
@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 15, 2016

Kevin, can you please give it a run with the oolite.org binaries?

For what is worth, the shipyard-overrides plist in the oxz has a missing closing brace in the ferdelance-player entry.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 15, 2016

That segfaulted too.

@perror

This comment has been minimized.

Copy link
Author

perror commented Aug 15, 2016

Nothing to add on kanthoney backtrace, I have pretty much the same effect.
And, yes, this is on a Linux distribution (last version 1.84).

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 16, 2016

Update: The Windows build also segfaults, but does it transparently. Running the executable via gdb shows a segfault, with what appears to be a trashed stack. The fact that we are not seeing a crash in Windows when running outside of gdb seems to be just a matter of luck.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 16, 2016

Suggested fix, please check and confirm (patch is against 1.84 release source):

diff --git a/src/Core/Entities/ShipEntity.m b/src/Core/Entities/ShipEntity.m
index df0606b..24c2d5f 100644
--- a/src/Core/Entities/ShipEntity.m
+++ b/src/Core/Entities/ShipEntity.m
@@ -3038,16 +3038,18 @@ ShipEntity* doOctreesCollide(ShipEntity* prime, ShipEntity* other)
        if ([_equipment containsObject:damaged])  return YES;
    }

-   if (includeMissiles && missiles > 0)
+   NSArray *currentMissilesList = [self missilesList];
+   unsigned currentMissilesCount = [currentMissilesList count];
+   if (includeMissiles && currentMissilesCount > 0)
    {
        unsigned i;
        if ([itemKey isEqualToString:@"thargon"]) itemKey = @"EQ_THARGON";
-       for (i = 0; i < missiles; i++)
+       for (i = 0; i < currentMissilesCount; i++)
        {
-           if ([[missile_list[i] identifier] isEqualTo:itemKey])  return YES;
+           if ([[[currentMissilesList objectAtIndex:i] identifier] isEqualTo:itemKey])  return YES;
        }
    }
-   
+
    return NO;
 }

Note that this seems to work for me, but it does not seem to get to the root of the problem. Basically, what I have done in this patch is substitute the missiles variable with the count of the missilesList array. For some reason (initialization issues maybe?) these two do not always agree and that is when the missile_list array goes haywire. I have seen many cases where missiles was reporting a value of 3, the missilesList count was 0 and all members of the missile_list array were nil. The root fix would probably be to find where missiles goes wrong and correct that.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 16, 2016

If I set up breakpoints in OOEquipmentType.m at lines 69 and 87, and run the command p *(gOOPlayer.missile_list[0]) at each to get the state of the missiles, then at line 69 I get:

$193 = {{isa = 0xdeadface}, _techLevel = 1, _price = 300, _name = 0x0, _identifier = 0x0, _description = 0x0, _isAvailableToAll = 1, 
  _requiresEmptyPylon = 1, _requiresMountedPylon = 0, _requiresClean = 0, _requiresNotClean = 0, _portableBetweenShips = 0, 
  _requiresFreePassengerBerth = 0, _requiresFullFuel = 0, _requiresNonFullFuel = 0, _isMissileOrMine = 1, _isVisible = 1, _isAvailableToPlayer = 1, 
  _isAvailableToNPCs = 1, _fastAffinityA = 0, _fastAffinityB = 0, _canCarryMultiple = 0, _installTime = 0, _repairTime = 0, _damageProbability = 0, 
  _requiredCargoSpace = 0, _requiresEquipment = 0x0, _requiresAnyEquipment = 0x0, _incompatibleEquipment = 0x0, _conditions = 0x0, _provides = 0x0, 
  _scriptInfo = 0x0, _weaponInfo = 0x0, _script = 0x0, _condition_script = 0x0, _jsSelf = 0x0}

which is all nice and regular, but at line 87 I get

$194 = {{isa = 0x646441646567616e}, _techLevel = 7596569194795331151, _price = 8299694825854100852, _name = 0x65726f2e61726170, 
  _identifier = 0x737365636f72705f, _description = 0x532f7a786f2e726f, _isAvailableToAll = 1, _requiresEmptyPylon = 1, _requiresMountedPylon = 0, 
  _requiresClean = 0, _requiresNotClean = 0, _portableBetweenShips = 1, _requiresFreePassengerBerth = 1, _requiresFullFuel = 0, _requiresNonFullFuel = 0, 
  _isMissileOrMine = 1, _isVisible = 0, _isAvailableToPlayer = 0, _isAvailableToNPCs = 1, _fastAffinityA = 1, _fastAffinityB = 1, _canCarryMultiple = 0, 
  _installTime = 177, _repairTime = 109001664, _damageProbability = 1.28476776e-24, _requiredCargoSpace = 32767, _requiresEquipment = 0x63c1eed, 
  _requiresAnyEquipment = 0x6f682f0000000000, _incompatibleEquipment = 0x6e6976656b2f656d, _conditions = 0x70657473554e472f, 
  _provides = 0x7972617262694c2f, _scriptInfo = 0x6163696c7070412f, _weaponInfo = 0x707075536e6f6974, _script = 0x696c6f4f2f74726f, 
  _condition_script = 0x67616e614d2f6574, _jsSelf = 0x736e4f6464416465}

which is trash. So the problem looks like it's occurring somewhere in the code

for (itemEnum = [equipmentData objectEnumerator]; (itemInfo = [itemEnum nextObject]); )
{
        item = [[[OOEquipmentType alloc] initWithInfo:itemInfo] autorelease];
        if (item != nil)
        {
                [equipmentTypes addObject:item];
                [equipmentTypesByIdentifier setObject:item forKey:[item identifier]];
        }
        NSString* condition_script = [item conditionScript];
        if (condition_script != nil)
        {   
                if (![conditionScripts containsObject:condition_script])
                {
                        [conditionScripts addObject:condition_script];
                }   
        }
}
@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Aug 17, 2016

I think that there is a problem also on the first state: $193 = {{isa = 0xdeadface} indicates that the object has been already deallocated and any subsequent messages to the object will result in segfault. It seems that the crash (or a generally invalid operation) has already happened at that point, but the program keeps going simply because no critical memory address has been accessed yet.

@perror

This comment has been minimized.

Copy link
Author

perror commented Aug 17, 2016

Maybe try to set up a gdb watchpoint on the missile struct ($193.isa), to see when it is touched in the program. If you try to guess the exact location, it will be endless.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 17, 2016

The corruption happens a couple of lines above, with the code

DESTROY(sEquipmentTypesByIdentifier)

Running p gOOPlayer.missiles after this shows the missile count is still 3.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 17, 2016

I don't think that's the problem, though, as the ship setup takes place after all that anyway. I'm slightly suspicious that missiles is set on line 321 of ShipEntity.m without initializing missile_list. The crash starts a few lines later at line 331, which eventually calls hasOneEquipmentItem which accesses the uninitialized missile_list.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 17, 2016

Running the game with the OXP uninstalled, I notice that the player ship is set up twice - once with a basic configuration to get things started, and then again with the real configuration. On the first pass through, the missile list is uninitialized. If we tried adding equipment in this state the game would crash, but normally that doesn't happen on the first pass. By the second pass through the missile list has been initialised and we can add equipment safely.

I would guess that the OXP adds some equipment to the basic Cobra, which the game tries to add in the first pass, causing the crash.

kanthoney added a commit that referenced this issue Aug 19, 2016

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Aug 19, 2016

There's an attempted fix in branch issue_204. I've moved the equipment adding part after the missile initializing part. This fixes the crash, but ship setup is a bit convoluted so there may be side effects.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 4, 2017

Here is another patch attempting to fix it. The problem apparently is that the missile_list is not yet initialized when addEquipment calls arrive. The philosophy of this fix is exactly the same as the one by Kevin, but I move the missile initialization code up rather than the upgrades setup code down. Doing that brings the missile init into the same part where all missile related initializations happen. There is a comment in the original code saying that missile init must come after scanClass setup, but I saw no scanClass related problems in my tests and I am not sure whether that comment was old and valid when it was written but not so anymore. Anyway, here it is for consideration:

diff --git a/src/Core/Entities/ShipEntity.m b/src/Core/Entities/ShipEntity.m
index b9d131f..6c70832 100644
--- a/src/Core/Entities/ShipEntity.m
+++ b/src/Core/Entities/ShipEntity.m
@@ -324,6 +324,27 @@ static ShipEntity *doOctreesCollide(ShipEntity *prime, ShipEntity *other);
 	if (missiles > max_missiles) missiles = max_missiles;
 	missile_load_time = fmax(0.0, [shipDict oo_doubleForKey:@"missile_load_time" defaultValue:0.0]); // no negative load times
 	missile_launch_time = [UNIVERSE getTime] + missile_load_time;
+	// Populate the missiles here. Must be done now because upcoming addEquipmentItem calls need the missile_list initialized
+	_missileRole = [shipDict oo_stringForKey:@"missile_role"];
+	unsigned	i, j;
+	for (i = 0, j = 0; i < missiles; i++)
+	{
+		missile_list[i] = [self selectMissile];
+		// could loop forever (if missile_role is badly defined, selectMissile might return nil in some cases) . Try 3 times, and if no luck, skip
+		if (missile_list[i] == nil && j < 3)
+		{
+			j++;
+			i--;
+		}
+		else
+		{
+			j = 0;
+			if (missile_list[i] == nil)
+			{
+				missiles--;
+			}
+		}
+	}
 	
 	// upgrades:
 	equipment_weight = 0; 
@@ -662,29 +683,6 @@ static ShipEntity *doOctreesCollide(ShipEntity *prime, ShipEntity *other);
 	[self setScannerDisplayColorHostile1:nil];
 	[self setScannerDisplayColorHostile2:nil];
 
-
-	// Populate the missiles here. Must come after scanClass.
-	_missileRole = [shipDict oo_stringForKey:@"missile_role"];
-	unsigned	i, j;
-	for (i = 0, j = 0; i < missiles; i++)
-	{
-		missile_list[i] = [self selectMissile];
-		// could loop forever (if missile_role is badly defined, selectMissile might return nil in some cases) . Try 3 times, and if no luck, skip
-		if (missile_list[i] == nil && j < 3)
-		{
-			j++;
-			i--;
-		}
-		else
-		{
-			j = 0;
-			if (missile_list[i] == nil)
-			{
-				missiles--;
-			}
-		}
-	}
-
 	// accuracy. Must come after scanClass, because we are using scanClass to determine if this is a missile.
 
 // missiles: range 0 to +10

Edit to add: I think now I know why there was the comment about initializing after scanClass. It looks like with this patch the Thargoids don't throw thargons anymore. I'll try to test some more to confirm when I can, but for now best to hold with this patch.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 4, 2017

The scanClass comment was introduced in 8731d87, which suggests the reason for putting the missile init after scanClass had something to do with thargoids. That might be why you've not seen any problems. Or, as you say, there might not be a problem any more.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 4, 2017

Have you moved the code from setUpShipFromDictionary to setUpFromDictionary? The problem there is that setUpFromDictionary is for items common to players and NPC's, whereas setUpShipFromDictionary is for NPC's only (PlayerEntity has its own version). So you'd give the player some NPC missiles and then (maybe?) overwrite them with the player's missiles later on.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 4, 2017

Looking at my fix, that moved code in the opposite direction, meaning the player ship wouldn't call some stuff it previously had been calling.

kanthoney added a commit that referenced this issue Mar 4, 2017

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 4, 2017

Branch issue_204_v2 moves the missile list initiation into a separate method setUpMissiles, which is subclassed for the player and called just after the missile parameters have been set.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 4, 2017

The crash has stopped. I think this is the fix we wanted. Just one detail though: Thargoids no longer launch thargons and this is related to missile setups being executed now before scanClass setup (that's why there was that comment there, it seems). Moving the setupMissiles and _missileRole lines where they used to be inside setUpShipFromDictionary solves the problem, still avoids the crash and does not seem to be causing any other issues that I can see.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 4, 2017

... and if we do as I say above, then we don't call missiles setup for the player anymore...
[sigh] need rest...

kanthoney added a commit that referenced this issue Mar 4, 2017

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 4, 2017

Another completely different fix in issue_204_v3. This one sets missiles to zero, while storing the number of missiles the ship will have (needed later in the method) in tmp_missiles. Just before missile setup in setUpShipFromDictionary (for both player and ship) the dictionary is consulted for the number of missiles.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 5, 2017

Kevin, I think I've got it. It's a refcount problem and your pointing me to the PlayerEntity missile setup handling was very helpful. Here is what fixes it for me:
On the master branch, PlayerEntity.m, -setUpShipFromDictionary method, around line 2247.
Change

missile_list[i] = [OOEquipmentType equipmentTypeWithIdentifier:@"EQ_MISSILE"];

to

missile_list[i] = [[OOEquipmentType equipmentTypeWithIdentifier:@"EQ_MISSILE"] retain];
@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 5, 2017

If there are no objections or something that I may have missed, I would go ahead and commit the above one-liner fix later today.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 5, 2017

Looking through the code, nothing else that calls [OOEquipmentType equipmentTypeWithIdentifier] uses retain, so I'm not sure why it's needed here. I don't think retain is needed because the equipment types are stored in a global dictionary called sEquipmentTypesByIdentifier so never get released.

I can confirm the fix works on Linux, but I don't know why.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 5, 2017

I've deleted the issue_204 branch as that's definitely not right.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 5, 2017

I think I see why the one-liner stops the crash. As per a comment above, there are two passes in creating the player ship, and I think sEquipmentTypesByIdentifier is destroyed and recreated between them, invalidating the missiles. The retain prevents the missiles from being invalidated, and therefore the crash, at the expense of a small memory leak.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 5, 2017

OK, maybe we cqn leave this open a little longer for further analysis then. We know we have a relatively good workaround, so if it comes to having to release and no better solution has been found until then, we can always commit that.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 9, 2017

We don't have enough potential fixes for this, so I've added branch issue_204_v4. This one simply fills the missile_list with nil values and then checks for nil values in hasOneEquipmentItem where the crash takes place.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 10, 2017

I think the v4 version is the best one so far. Although I would really like to understand where exactly that missile_list gets invalidated and reinited, if it is indeed the case as mentioned earlier, this last fix seems to be doing a logical initialization at an apparently proper point so I would be inclined to say let's go with it, maybe with a TODO comment that a potential invalidation and reinit of the array may be happening and should be investigated.

kanthoney added a commit that referenced this issue Mar 10, 2017

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 10, 2017

TODO comment added.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 11, 2017

Some further investigation later, I think this is what happens with this bug:

  1. Universe loading starts, we reach the -initWithGameView method. This calls -setUpSettings, which in turn calls +loadEquipment from the OOEquipmentType class. This is the first time where the static dicts sEquipmentTypesByIdentifier and friends get set up. Backtrace at that time:
#0  +[OOEquipmentType loadEquipment] (self=0x7a6b00, _cmd=0x7aeed8)
    at src/Core/OOEquipmentType.m:60
#1  0x006a15fa in -[Universe setUpSettings] (self=0x435f618, _cmd=0x7adc30)
    at src/Core/Universe.m:9728
#2  0x006ab91f in -[Universe initWithGameView:] (self=0x435f618, 
    _cmd=0x79b918, inGameView=0x36cfea8) at src/Core/Universe.m:364
#3  0x0065e723 in -[GameController applicationDidFinishLaunching:] (
    self=0x4365ee8, _cmd=0x79fe08, notification=0x0)
    at src/Core/GameController.m:252
#4  0x006648ae in SDL_main (argc=argc@entry=1, argv=argv@entry=0x2d89248)
    at src/SDL/main.m:113
#5  0x006b3f2d in console_main (argc=argc@entry=1, argv=argv@entry=0x2d89248)
    at ./src/main/win32/SDL_win32_main.c:246
#6  0x006b40ed in WinMain@16 (hInst=0x400000, hPrev=0x0, 
    szCmdLine=0x2a43b2 "", sw=10) at ./src/main/win32/SDL_win32_main.c:382
#7  0x006bcf2b in main ()
  1. When loading a player or starting a new game, the -setUpSettings method is called again, with the result of +loadEquipment being called again, as seen in the second bactrace:
#0  +[OOEquipmentType loadEquipment] (self=0x7a6b00, _cmd=0x7aeed8)
    at src/Core/OOEquipmentType.m:60
#1  0x006a15fa in -[Universe setUpSettings] (self=0x435f618, _cmd=0x7adc30)
    at src/Core/Universe.m:9728
#2  0x00695407 in -[Universe reinitAndShowDemo:] (self=0x435f618, 
    _cmd=0x7adce0, showDemo=0 '\000') at src/Core/Universe.m:9833
#3  0x00496a4d in -[PlayerEntity(LoadSave) loadPlayerFromFile:asNew:] (
    self=0x415d418, _cmd=0x6e8318, fileToOpen=0x4392c00, asNew=1 '\001')
    at src/Core/Entities/PlayerEntityLoadSave.m:646
#4  0x00495202 in -[PlayerEntity(LoadSave) startScenario] (self=0x415d418, 
    _cmd=0x6df4f0) at src/Core/Entities/PlayerEntityLoadSave.m:345
#5  0x00472da3 in -[PlayerEntity(OOControlsPrivate) pollDemoControls:] (
    self=0x415d418, _cmd=0x6de9b0, delta_t=0.016999959945678711)
    at src/Core/Entities/PlayerEntityControls.m:4352
#6  0x0046fb83 in -[PlayerEntity(Controls) pollControls:] (self=0x415d418, 
    _cmd=0x6cd5f8, delta_t=0.016999959945678711)
    at src/Core/Entities/PlayerEntityControls.m:389
#7  0x0043e216 in -[PlayerEntity update:] (self=0x415d418, _cmd=0x7aeaf8, 
    delta_t=0.016999959945678711) at src/Core/Entities/PlayerEntity.m:2554
#8  0x006a7988 in -[Universe update:] (self=0x435f618, _cmd=0x79b990, 
    inDeltaT=0.016999959945678711) at src/Core/Universe.m:6629
#9  0x0065eb00 in -[GameController doPerformGameTick] (self=0x4365ee8, 
    _cmd=0x79b988) at src/Core/GameController.m:350
	(...)
  1. At this point, although sEquipmentTypesByIdentifier and friends exist, they get destroyed and re-initialized. The pointers of those dictionary contents now point to addresses which are not the same as the ones held for the identifiers stored in missile_list[], as those identifiers no longer exist in memory. As soon as an access of the missile_list[] aray is attempted, we are hitting invalid addresses right away, hence crash. Additionally, given that the sEquipmentTypesByIdentifier dicts are basically holding information that is loaded from equipment.plist, which never changes, it does not make sense to try and load them or re-init them again once that has been done for the very first time.

As a result, I think that the base fix (hey look, another one of them!) to the bug could be to just ensure that the sEquipmentTypesByIdentifier and sEquipmentTypes static dictinaries are initialized just once. To do that, we can just check for one of them, as they are both inited inside the same method and almost simultaneously by adding the line
if (sEquipmentTypesByIdentifier != nil) return;
at the start of the +loadEquipment method. This fixes the crash for me.

Edit to add: Not sure what happens if a different equipment.plist has to be loaded though, which I guess could happen in the case of switching scenarios. Maybe we do need to be able to execute the +loadEquipment method on player reload after all...

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 11, 2017

The fly in the ointment with that (you knew there was one!) is that the list gets reinitialised if you add any OXZ's to include any new equipment.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 11, 2017

Yeah, not a good fix after all I guess. We need to keep the ability to reload equipment on player reload. The more I think about it, the more I get convinced that v4 should be the approved fix.

@kanthoney

This comment has been minimized.

Copy link
Contributor

kanthoney commented Mar 11, 2017

Yes, definitely need to initialize the missile_list somehow. Short of completely refactoring the ship setup code, I think v4 is the best fix. I've taken the plunge and merged it into master.

@AnotherCommander

This comment has been minimized.

Copy link
Member

AnotherCommander commented Mar 11, 2017

Good. And just to add a couple of closing comments to the earlier description of what is happening, the missile_list array for the player does get set in Player setUpShipFromDictionary, but by the time this is attempted, we have already referenced its (invalid at that time) elements from within ShipEntity setUpFromDictionary when checking whether equipment can be added or not.

I would say that, unless anyone would want to add or comment further, we can close this in a while.

kanthoney added a commit that referenced this issue Mar 14, 2017

AnotherCommander added a commit that referenced this issue Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.