Opening/closing tools from Lua can crash BizHawk

[kalimag]

Summary

If a tool is directly or indirectly opened or closed from Lua, in many cases a collection changed exception occurs in ToolManager and BizHawk crashes.

There are a few different ways this can happen, but all come down to ToolManager._tools being modified within one of the various foreach (var tool in _tools) loops.

Presumably the same problem exists with external tools that cause other tools to be opened/closed from Update... and Restart.

Repro

Various ways to trigger the issue, there may be more:

-- Open a rom then run this script
emu.frameadvance() -- necessary so the next line is executed from the update event
client.openhexeditor() -- or any other tool

This example also works from the Lua Console:

-- Open a rom then run this script
event.onframestart(function() client.openhexeditor() end)

Some others:

-- Open a rom, open the hex editor, then run this script
emu.frameadvance()
client.closerom()
--or
client.openrom(".\\nonexistent")
--or
client.openrom(".\\multiple-roms.zip") -- then cancel rom selection
--or anything else that causes an open rom that tools are dependent on to unload

-- Activate this script, then open a rom, then restart the core
client.openhexeditor()
while true do
    emu.frameadvance();
end

Output

System.InvalidOperationException
  Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource) in f:\dd\ndp\clr\src\BCL\system\throwhelper.cs:line 99
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare() in f:\dd\ndp\clr\src\BCL\system\collections\generic\list.cs:line 1178
   at BizHawk.Client.EmuHawk.ToolManager.UpdateToolsAfter() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\tools\ToolManager.cs:line 762
   at BizHawk.Client.EmuHawk.MainForm.UpdateToolsAfter() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 1767
   at BizHawk.Client.EmuHawk.MainForm.StepRunLoop_Core(Boolean force) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 3055
   at BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 868
   at BizHawk.Client.EmuHawk.Program.SubMain(String[] args) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\Program.cs:line 345
   at BizHawk.Client.EmuHawk.Program.Main(String[] args) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\Program.cs:line 89

Stack trace may differ depending on what event caused the tool open/close.

Host env.

• BizHawk 2.5.0 up to current dev
• BizHawk 2.4.2 (partially?) unaffected (probably 22ee71b0cf901cc5bfbac64390860f2c46acec5c)

[kalimag]

I think the easiest solution would be to turn ToolManager._tools from a List<T> into an ImmutableArray<T>. I can't think of any mutable collection type that has the same APIs and can safely be modified during enumeration, and making defensive copies for every enumeration would be wasteful. Modifying the _tools collection is not common so I imagine the perf should be insignificant.

I can open a PR for this if you agree.

[YoshiRulz]

No, it needs to be mutable. Calling ToArray is correct. Oh, I see you meant a copy-on-write. That might also work, but your worries about the overhead would be misplaced, since it's a very small array of references (nuints).

[kalimag]

Yeah, that's what I mean. It's a choice between making a defensive copy every time the field is enumerated (multiple times every frame) or copy on write when a tool is opened/closed.

I realize either way it's not a measurable perf hit on its own, but the latter seems much cleaner to me.