Hex editor shows wrong "File on Disk" when switching games

[kalimag]

Summary

When loading two different roms while the hex editor is open, the "File on Disk" in the hex edtior will show the contents of the previous rom, rather than the current rom. Additionally, if the previous rom was a MAME archive, an error will occur.

Repro

1. Open the hex editor and keep it open
2. Load any rom A (e.g a N64 rom with an easily identifiable header)
3. Load a different rom B
4. Switch to the "File on Disk" domain in the hex editor
5. "File on Disk" will display contents of rom A

Alternatively, load a MAME archive at step 2 to reproduce the exception

Output

If the first rom was a MAME archive:

HawkFile: Can't call GetStream() before you've successfully bound something!
   at BizHawk.Common.HawkFile.GetStream() in F:\Emulation\BizHawk\src\BizHawk.Common\HawkFile\HawkFile.cs:line 267
   at BizHawk.Client.EmuHawk.HexEditor.GetRomBytes() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\tools\HexEditor\HexEditor.cs:line 446
   at BizHawk.Client.EmuHawk.HexEditor.Restart() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\tools\HexEditor\HexEditor.cs:line 210
   at BizHawk.Client.EmuHawk.ToolManager.Restart(Config config, IEmulator emulator, IGameInfo game) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\tools\ToolManager.cs:line 521
   at BizHawk.Client.EmuHawk.MainForm.LoadRomInternal(String path, LoadRomArgs args, Boolean& failureIsFromAskSave) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 3934
   at BizHawk.Client.EmuHawk.MainForm.LoadRom(String path, LoadRomArgs args, Boolean& failureIsFromAskSave) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 3714
   at BizHawk.Client.EmuHawk.MainForm.LoadRom(String path, LoadRomArgs args) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 3710
   at BizHawk.Client.EmuHawk.MainForm.OpenRom() in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.cs:line 2427
   at BizHawk.Client.EmuHawk.MainForm.OpenRomMenuItem_Click(Object sender, EventArgs e) in F:\Emulation\BizHawk\src\BizHawk.Client.EmuHawk\MainForm.Events.cs:line 296

Host env.

• BizHawk 2.3 (earliest tested) up to 2.9 dev

Cause

The issue is that HexEditor.GetRomBytes() (called from HexEditor.Restart()) loads the rom from MainForm.CurrentlyOpenRomArgs, but MainForm.LoadRomInternal calls Tools.Restart() before updating CurrentlyOpenRomArgs:

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

Lines 425 to 433 in 1df6ce4

	private byte[] GetRomBytes()
	{
	var path = MainForm.CurrentlyOpenRomArgs.OpenAdvanced.SimplePath;
	if (string.IsNullOrEmpty(path))
	{
	return new byte[] { 0xFF };
	}
	using var file = new HawkFile(path);

BizHawk/src/BizHawk.Client.EmuHawk/MainForm.cs

Lines 3934 to 3950 in 15056f1

	Tools.Restart(Config, Emulator, Game);
	if (Config.Cheats.LoadFileByGame && Emulator.HasMemoryDomains())
	{
	CheatList.SetDefaultFileName(Tools.GenerateDefaultCheatFilename());
	if (CheatList.AttemptToLoadCheatFile(Emulator.AsMemoryDomains()))
	{
	AddOnScreenMessage("Cheats file loaded");
	}
	else if (CheatList.Any())
	{
	CheatList.Clear();
	}
	}
	CurrentlyOpenRom = oaOpenrom?.Path ?? openAdvancedArgs;
	CurrentlyOpenRomArgs = args;

[kalimag]

In general, I think calling Tools.Restart before updating CurrentlyOpenRom/CurrentlyOpenRomArgs is unexpected and undesirable.

Other places where those properties are read inside Restart:

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

Lines 227 to 232 in 15056f1

	// Don't reset scroll bar if restarting the same rom
	if (_lastRom != MainForm.CurrentlyOpenRom)
	{
	_lastRom = MainForm.CurrentlyOpenRom;
	ResetScrollBar();
	}

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

Lines 288 to 292 in 15056f1

	if (_lastRom != MainForm.CurrentlyOpenRom)
	{
	_lastRom = MainForm.CurrentlyOpenRom;
	SetupControlsAndProperties();
	}

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

Lines 56 to 90 in 15056f1

	public override void Restart()
	{
	FileSelectorPanel.Controls.Clear();
	AddButton_Click(null, null);
	AddButton_Click(null, null);
	if (!Game.IsNullInstance() && !MainForm.CurrentlyOpenRom.EndsWith(".xml"))
	{
	if (MainForm.CurrentlyOpenRom.Contains("|"))
	{
	var pieces = MainForm.CurrentlyOpenRom.Split('|');
	var directory = Path.GetDirectoryName(pieces[0]) ?? "";
	var filename = Path.ChangeExtension(pieces[1], ".xml");
	NameBox.Text = Path.Combine(directory, filename);
	}
	else
	{
	NameBox.Text = Path.ChangeExtension(MainForm.CurrentlyOpenRom, ".xml");
	}
	if (SystemDropDown.Items.Contains(Emulator.SystemId))
	{
	SystemDropDown.SelectedItem = Emulator.SystemId;
	}
	else if (Emulator is SMS sms && sms.IsGameGear)
	{
	SystemDropDown.SelectedItem = VSystemID.Raw.GGL;
	}
	FileSelectors.First().Path = MainForm.CurrentlyOpenRom;
	Recalculate();
	}
	}

All instances of tools reading those properties inside Restart assume that the properties contain the new rom that is being loaded, and are therefore currently bugged. They don't update their UIs when switching roms (the update will happen after a second restart), and MultiDiskBundler throws an NRE if no rom was loaded before.

Could also negatively affect external tools.

[YoshiRulz]

Context: these are ((this as ToolFormBase).MainForm as IMainFormForTools).* and so have found their way into ApiHawk. But whether they were ever meant to be isn't really relevant when the current behaviour also breaks first-party tools.