Netplay: Fix possible Wii save restore race condition between Netplay and CPU threads on game shutdown by making the Wii Save Sync data part of the BootParameters.

[AdmiralCurtiss]

Scenario: Two people play Netplay with Wii save syncing enabled. This works in Dolphin by making a temporary Wii NAND, copying the host's save file into there, then running on that temp NAND for the game session. Then the host disconnects from the client so that this piece of code is reached:

dolphin/Source/Core/Core/NetPlayClient.cpp

Lines 1543 to 1549 in 6ca2897

	case ENET_EVENT_TYPE_DISCONNECT:
	m_dialog->OnConnectionLost();
	if (m_is_running.IsSet())
	StopGame();
	break;

and then in StopGame():

dolphin/Source/Core/Core/NetPlayClient.cpp

Lines 2251 to 2254 in 6ca2897

	// stop game
	m_dialog->StopGame();
	ClearWiiSyncData();

m_dialog->StopGame(); can return before the shutdown is actually complete -- I'm not entirely sure on the details here, but I believe what happens is that this emit Stop() here:

dolphin/Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp

Lines 691 to 698 in 6ca2897

	void NetPlayDialog::StopGame()
	{
	if (m_got_stop_request)
	return;
	m_got_stop_request = true;
	emit Stop();
	}

Is handled asynchronously by the GUI thread. Which then leads the Netplay thread to call ClearWiiSyncData(); while the CPU thread is still in the process of shutting down, which:

dolphin/Source/Core/Core/NetPlayClient.cpp

Lines 2557 to 2566 in 6ca2897

	void ClearWiiSyncData()
	{
	// We're just assuming it will always be here because it is
	const std::string path = File::GetUserPath(D_USER_IDX) + "Wii" GC_MEMCARD_NETPLAY DIR_SEP;
	if (File::Exists(path))
	File::DeleteDirRecursively(path);
	s_wii_sync_fs.reset();
	s_wii_sync_titles.clear();
	}

The important line here is the s_wii_sync_fs.reset();, which the CPU thread shutdown routine uses to detect whether it should copy back data to the main NAND or not -- if we're the host it is null, if we're the client it is not null, or that's the theory anyway.

dolphin/Source/Core/Core/WiiRoot.cpp

Lines 339 to 345 in 6ca2897

	void CleanUpWiiFileSystemContents()
	{
	if (!WiiRootIsTemporary() || !Config::Get(Config::SESSION_SAVE_DATA_WRITABLE) ||
	NetPlay::GetWiiSyncFS())
	{
	return;
	}

Anyway, long story short, I fixed this by just putting this data into the BootParameters so the CPU thread lifetime decides when it gets cleared, rather than some global in the Netplay files. This should circumvent this entire mess and have it work regardless of timing.

(Also note: The "Wii" GC_MEMCARD_NETPLAY directory being deleted is not, as you might assume, the temp NAND -- rather it is the staging directory used for collecting saves when they're sent to the client on netplay initialization. So that part doesn't break anything.)

[JMC47]

This is probably urgent to merge once it's ready, due to the nature of how this works.

[leoetlino]

Reviewed 9 of 9 files at r1, 5 of 5 files at r2, 5 of 5 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AdmiralCurtiss)

---

Source/Core/Core/Core.cpp, line 483 at r3 (raw file):

  BootSessionData boot_session_data = std::move(boot->boot_session_data);
  const std::optional<std::string> savestate_path = boot_session_data.GetSavestatePath();

Making a copy of the savestate path seems unnecessary now, so this can be turned into a reference

---

Source/Core/Core/Core.cpp, line 621 at r3 (raw file):

  // with the correct title context since save copying requires title directories to exist.
  Common::ScopeGuard wiifs_guard{
      [&boot_session_data]() { Core::CleanUpWiiFileSystemContents(boot_session_data); }};

no need for ()

---

Source/Core/Core/WiiRoot.cpp, line 342 at r3 (raw file):

(Also note: The "Wii" GC_MEMCARD_NETPLAY directory being deleted is not, as you might assume, the temp NAND -- rather it is the staging directory used for collecting saves when they're sent to the client on netplay initialization. So that part doesn't break anything.)

I'm a bit confused -- is this clearing the netplay temp NAND or the save staging area?

---

Source/Core/DolphinQt/NetPlay/NetPlayDialog.h, line 39 at r2 (raw file):

public:
  explicit NetPlayDialog(const GameListModel& game_list_model,
                         std::function<void(const std::string& path,

using StartGameCallback = ... to make this more readable (and avoid having to write the entire thing a second time for m_start_game_callback)?

[AdmiralCurtiss]

Ah oops, yes, staging area, not temp NAND, I was confused on this myself at first and it looks like the function name is a leftover from that. Probably not the right place for this either now that I think about it, that should be somewhere in the Netplay code...

[AdmiralCurtiss]

Alright, redid this a bit so the staging directory is deleted in a callback set from the Netplay code. I think this is a lot cleaner!

[leoetlino]

Reviewed 12 of 12 files at r5, 5 of 5 files at r6, 5 of 5 files at r7, 6 of 6 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AdmiralCurtiss)