Crash when rebooting emulator core

[SuuperW]

The following Lua script crashes EmuHawk when rebooting the core:

event.onexit(function()
	memory.read_u8(0)
end)

while true do
	emu.frameadvance()
end

This happens because the script is not stopped until LuaConsole.Restart, at which point the old core instance has been disposed. (Of course, even if the exit handler read from the new core, that would not be correct either.)

Repro

1. Open a ROM with a core that supports memory API
2. Start the above Lua script
3. Reload the ROM

Output

No error message.

Host env.

BizHawk 2.11 and current master

[YoshiRulz]

X–Y problem.
Implementors of IToolForm.Restart are expected to use the updated values that have just been provided...

BizHawk/src/BizHawk.Client.EmuHawk/tools/ToolManager.cs

Lines 604 to 608 in c8af415

	if (ServiceInjector.UpdateServices(_emulator.ServiceProvider, tool)
	&& (tool is not IExternalToolForm || ApiInjector.UpdateApis(GetOrInitApiProvider, tool)))
	{
	if (tool.IsActive) tool.Restart();
	}

...which includes [RequiredService] IEmulator Emulator. A separate Stop method would be redundant.
Some Lua callbacks, including onexit, only support a subset of the API. When a script attempts to call an unsupported function like peeking, we should terminate the callback (or the script, but for an onexit handler there's little difference) and print an error to the Lua Console.

[SuuperW]

Some Lua callbacks, including onexit, only support a subset of the API.

Source? Just because they happen to not work while the core is rebooting, doesn't mean they are not intended to work when a script is stopping. Memory reads and writes do work when a script is stopping, so long as the core hasn't been destroyed.

And saying they should not be used is unacceptable. Scripts that are used as cheats must be able to do RAM pokes that disable the cheat (restore original data or code) when they exit.

Implementors of IToolForm.Restart are expected to use the updated values that have just been provided ... A separate Stop method would be redundant.

I think you do not understand the issue. Reading or poking with the newly created emulator core would be incorrect. Shutdown logic (the onexit handler) should not run on a just-created core, but on the core it was intended to run on. This is impossible to do if that core has been destroyed, thus the need for stopping the script prior to core disposal. Such shutdown logic may not be necessary in the case of a reboot, but as-is the user cannot handle that.

Adding a parameter to the onexit callback to indicate if the core has rebooted could solve the particular situation I have encountered, but I think running the exit handler at the proper time is a nicer, cleaner solution. Plus, solving this by adding that parameter would force script authors to consider this edge case, which isn't necessary.

[CasualPokePlayer]

The main callbacks which would have limited API support would be the memory and input callbacks, which are inherently in place as those are inside FrameAdvance(), and thus trying to do things like savestates or rebooting the core would be disastrous and thus not allowed (and code is already in place to prevent users from using such APIs). event.onexit probably doesn't have much reason to have limited API support, granted I imagine most API wouldn't be particularly useful in an onexit context (since the script is effectively dead after that point) but that's not really relevant to whether it can be used or not.

[YoshiRulz]

What CPP said. Apart from looking for call-sites of IsInInputOrMemoryCallback, this stuff is implicit and under-documented because of course it is.

[...] Memory reads and writes do work when a script is stopping, so long as the core hasn't been destroyed.

And saying they should not be used is unacceptable. Scripts that are used as cheats must be able to do RAM pokes that disable the cheat (restore original data or code) when they exit.

As in, depending on GC timing? Scripts definitely shouldn't be relying on that, though if we could ensure it was consistent (WeakReference?) then I suppose it would be best to support pokes in onexit (not onconsoleclose) for use cases like yours. Has the call order changed over the years?

Implementors of IToolForm.Restart are expected to use the updated values that have just been provided ... A separate Stop method would be redundant.

I think you do not understand the issue. Reading or poking with the newly created emulator core would be incorrect. Shutdown logic (the onexit handler) should not run on a just-created core, but on the core it was intended to run on. This is impossible to do if that core has been destroyed, thus the need for stopping the script prior to core disposal. Such shutdown logic may not be necessary in the case of a reboot, but as-is the user cannot handle that.

This only applies to scripts which interact with the core from within an onexit handler, assuming we want to support that, and not to ext. tools. For example, poking on exit could look something like:

_domain = _memoryDomains?.First();
Closing += (_, _) => _domain?.PokeByte(0x1234, 0);

Adding a parameter to the onexit callback to indicate if the core has rebooted could solve the particular situation I have encountered, but I think running the exit handler at the proper time is a nicer, cleaner solution. Plus, solving this by adding that parameter would force script authors to consider this edge case, which isn't necessary.

Conclusion from #3800 was to not add more callback spaghetti for script authors to trip over; scripts' lifetimes are tied to the core's.

[SuuperW]

What CPP said. Apart from looking for call-sites of IsInInputOrMemoryCallback, this stuff is implicit and under-documented because of course it is.

I do not know what you mean by saying "What CPP said". His post supports my position. IsInInputOrMemoryCallback is not used for the onexit event because of course it isn't. That is only for things that "are inside FrameAdvance()", which onexit is not. There are no intentional restrictions on API use for the onexit handler.
Even if IsInInputOrMemoryCallback were used, that would not address this issue. Memory reads and pokes are allowed in input and memory callbacks.

As in, depending on GC timing?

As in, if you stop a script normally.

(not onconsoleclose)

There is no reason to not allow this. Just because we don't know a solid use case doesn't mean we should introduce artificial restrictions. The onexit handler should run prior to any console closing logic that would interfere with it, just as it should for core reboots. (I have not looked at this event, and do not know if there are any actual issues there.)

Has the call order changed over the years?

I don't think so, this has probably been a problem for as long as onexit has existed. I just tested 2.9 to be sure, and the problem exists there.

assuming we want to support that

WE DO

ext. tools. For example, poking on exit could look something like:

That would be poking when the tool closes, which is not the same thing.
The issue arises because Lua scripts require an onexit handler, which must be called any time the script stops. And that includes core reboots even though core reboots aren't the normal way of stopping scripts. Thus, Lua Console needs some way of calling the onexit handlers at the proper time. And I would prefer a general solution that allows all tools to run logic prior to core disposal over one that makes an exception for the Lua Console.

scripts' lifetimes are tied to the core's.

From that discussion, no they are not. client.reboot_core() will start a new core, but no Lua scripts will be stopped on such a reboot. This is different from regular core reboots, and I think this is an issue. Although I am not sure what a good solution is, since preserving the script that called reboot_core seems pretty valuable. Anyway, that's another issue. More related to this is "not add more callback spaghetti", which I agree for this issue. There's no need to add anything to the Lua API to resolve this issue, as my PR shows.