onShipHit event can cause crashes

[Brianetta]

Code like this (first line is line 23):

local onShipHit = function (ship, attacker)
    if attacker:isa('Ship') and attacker:IsPlayer() then
        PlayerDamagedShips[ship]=true
    end
end

EventQueue.onShipHit:Connect(onShipHit)

can cause errors like this:

Error: data/modules/StatsTracking/Statstracking.lua:24: attempt to index local 'attacker' (a nil value)
stack traceback:
    data/modules/StatsTracking/Statstracking.lua:24: in function <data/modules/StatsTracking/Statstracking.lua:23>
Error: data/modules/StatsTracking/Statstracking.lua:24: attempt to index local 'attacker' (a nil value)
stack traceback:
    data/modules/StatsTracking/Statstracking.lua:24: in function <data/modules/StatsTracking/Statstracking.lua:23>

I've seen this happen when I have fired a missile at a ship, and when an AI ship has been shooting with a laser at another AI ship.

[robn]

I know this one well. An example scenario where this occurs is when two ships are firing. The first one is destroyed, but some of his lasers or missiles are still floating and hit the other one. onShipHit then fires with a null attacker.

The way I want to fix this in the long term is to keep dead ships hanging around in some kind of "dead" state until there's nothing else referencing them. Once all the lasers, missiles and whatever else that have the ship listed as their owner are gone, the ship can be cleaned up. This is easier to do now we have the refcounting smart pointer but is still some effort.

[Brianetta]

In these instances, no attacking ships were destroyed. The test cases (for "assist" kills) involved undefended, unarmed, static targets, attacked by specially spawned adders with pulse lasers. On one occasion, the only attacker was me, and I certainly hadn't been damaged or destroyed.

[impaktor]

I'm currently tinkering with getting crime into Lua, at least some parts of it, and I think this bug needs to be resolved if it isn't already.

Once this is resolved, StatsTracking can track assisted kills as well.

Anyone feeling up for the challenge?

[impaktor]

Grievance

This bug continues to halt code using Lua, since anything referencing an attacker upon a ship being destroyed will trigger this bug.

How to reproduce

• Set city detail to max, to get as many buildings as possible.

• Save script below in data/modules/trigger.lua.

  local Event = import("Event")
  local Comms = import("Comms")
  
  local onShipHit = function(ship, attacker)
      if ship:IsPlayer() then
          ship:SetHullPercent(100)
          Comms.ImportantMessage("I am immortal", ship.label)
          return
      end
  
      -- calling attacker here is fine, since no one is dead yet
      if not attacker:IsPlayer() then return end
  end
  
  local onShipDestroyed = function(ship, attacker)
      -- If attacker is the ship that got destroyed then calling it will
      -- cause crash (sometimes/usually?); only when dead attacker still
      -- has lead in the air?
      if attacker:exists() and attacker:IsPlayer() then -- Crash! / BUG
          Comms.ImportantMessage("Now I am become death!", ship.label)
      end
  end
  
  local onShipFiring = function(ship)
      if not ship:IsPlayer() then return end
      Comms.ImportantMessage("boomsticks are not allowed!", ship.label)
  end
  
  
  Event.Register("onShipHit", onShipHit)
  Event.Register("onShipDestroyed", onShipDestroyed)
  Event.Register("onShipFiring", onShipFiring)
  

• Fly low in the city, between the buildings and annoy the police. They will crash into the buildings, and usually trigger this bug.

Error message

Yesterday @johnbartholomew asked for some error messages/bt, since it's in lua there's not much of backtrace, but here are two different error messages:

error: [string "[T] @modules/trigger.lua"]:19: attempt to index local 'attacker' (a nil value)
stack traceback:
    [string "[T] @modules/trigger.lua"]:19: in function 'cb'
    [string "[T] @libs/Event.lua"]:34: in function '?'
    [string "[T] @libs/Event.lua"]:185: in function <[string "[T] @libs/Event.lua"]:180>

or:

TradeShips: Removed 0 ships before serialization
HB-8378 left Epsilon Eridani for GJ 1005
cleanTSTable:total:30,active:21,removed:0
error: [string "[T] @modules/trigger.lua"]:19: attempt to call method 'IsPlayer' (a nil value)
stack traceback:
    [string "[T] @modules/trigger.lua"]:19: in function 'cb'
    [string "[T] @libs/Event.lua"]:34: in function '?'
    [string "[T] @libs/Event.lua"]:185: in function <[string "[T] @libs/Event.lua"]:180>
[Thread 0x7fffef3ea700 (LWP 6566) exited]
[Thread 0x7fffee3e8700 (LWP 6568) exited]
[Thread 0x7fffeebe9700 (LWP 6567) exited]
[Thread 0x7ffff7fbc740 (LWP 6564) exited]
[Inferior 1 (process 6564) exited with code 01]
(gdb) bt
No stack.

[walterar]

if attacker and attacker:exists() and attacker:IsPlayer() then -- NOT Crash! / NO BUG ;)

[walterar]

The problem is not onShipHit event.
The problem is that "xx: exists()", "xx:IsPlayer()", etc, do not return nil when xx is nil.

[fluffyfreak]

So the fix is that TradeShips.lua needs to have

if attacker and not attacker:IsPlayer() then return end -- XX

and it will all work fine then?

[walterar]

right

[impaktor]

I'm not convinved. Feels like a hacky workaround of the actual problem described by @robn.

[johnbartholomew]

I agree this is a workaround, but since it is likely to take quite a lot of effort/time to implement a better fix, I think we should do this anyway.

There may be other scripts that need this fix too, but I guess we can wait for people to report errors.

[fluffyfreak]

I don't really care :) it's been open for more than 2 years without a fix, and there are always problems associated with keeping around "dead" objects so you can clean them up later - like references that they hold to other things and the references that they then hold to other things.

This just stops the crash happening which is sometimes the best solution. It's simple, the bug goes away, and it doesn't introduce large amounts of complexity with the unknown future bugs that come with it.

and @johnbartholomew beat me to the post! :P