Fix more segfaults on NetPlay quit

Basically everything here was race conditions in Qt callbacks, so I changed the client/server instances to std::shared_ptr and added null checks. It checks that the object exists in the callback, and the shared_ptr ensures it doesn't get destroyed until we're done with it.

MD5 check would also cause a segfault if you quit without cancelling it first, which was pretty silly.
This commit is contained in:
Techjar 2018-07-12 20:37:12 -04:00
parent a21d536f99
commit cfeffdcf42
7 changed files with 48 additions and 29 deletions

View File

@ -70,6 +70,10 @@ NetPlayClient::~NetPlayClient()
if (m_is_connected) if (m_is_connected)
{ {
m_should_compute_MD5 = false;
m_dialog->AbortMD5();
if (m_MD5_thread.joinable())
m_MD5_thread.join();
m_do_loop.Clear(); m_do_loop.Clear();
m_thread.join(); m_thread.join();
} }
@ -1636,12 +1640,14 @@ void NetPlayClient::ComputeMD5(const std::string& file_identifier)
return; return;
} }
if (m_MD5_thread.joinable())
m_MD5_thread.join();
m_MD5_thread = std::thread([this, file]() { m_MD5_thread = std::thread([this, file]() {
std::string sum = MD5::MD5Sum(file, [&](int progress) { std::string sum = MD5::MD5Sum(file, [&](int progress) {
sf::Packet packet; sf::Packet packet;
packet << static_cast<MessageId>(NP_MSG_MD5_PROGRESS); packet << static_cast<MessageId>(NP_MSG_MD5_PROGRESS);
packet << progress; packet << progress;
Send(packet); SendAsync(std::move(packet));
return m_should_compute_MD5; return m_should_compute_MD5;
}); });
@ -1649,9 +1655,8 @@ void NetPlayClient::ComputeMD5(const std::string& file_identifier)
sf::Packet packet; sf::Packet packet;
packet << static_cast<MessageId>(NP_MSG_MD5_RESULT); packet << static_cast<MessageId>(NP_MSG_MD5_RESULT);
packet << sum; packet << sum;
Send(packet); SendAsync(std::move(packet));
}); });
m_MD5_thread.detach();
} }
const PadMappingArray& NetPlayClient::GetPadMapping() const const PadMappingArray& NetPlayClient::GetPadMapping() const

View File

@ -668,7 +668,7 @@ bool MainWindow::RequestStop()
const Core::State state = Core::GetState(); const Core::State state = Core::GetState();
// Only pause the game, if NetPlay is not running // Only pause the game, if NetPlay is not running
bool pause = Settings::Instance().GetNetPlayClient() == nullptr; bool pause = !Settings::Instance().GetNetPlayClient();
if (pause) if (pause)
Core::SetState(Core::State::Paused); Core::SetState(Core::State::Paused);
@ -1071,7 +1071,7 @@ bool MainWindow::NetPlayJoin()
std::string host_ip; std::string host_ip;
u16 host_port; u16 host_port;
if (Settings::Instance().GetNetPlayServer() != nullptr) if (Settings::Instance().GetNetPlayServer())
{ {
host_ip = "127.0.0.1"; host_ip = "127.0.0.1";
host_port = Settings::Instance().GetNetPlayServer()->GetPort(); host_port = Settings::Instance().GetNetPlayServer()->GetPort();

View File

@ -21,7 +21,11 @@
static QString GetPlayerNameFromPID(int pid) static QString GetPlayerNameFromPID(int pid)
{ {
QString player_name = QObject::tr("Invalid Player ID"); QString player_name = QObject::tr("Invalid Player ID");
for (const auto* player : Settings::Instance().GetNetPlayClient()->GetPlayers()) auto client = Settings::Instance().GetNetPlayClient();
if (!client)
return player_name;
for (const auto* player : client->GetPlayers())
{ {
if (player->pid == pid) if (player->pid == pid)
{ {
@ -82,7 +86,11 @@ void MD5Dialog::show(const QString& title)
m_results.clear(); m_results.clear();
m_check_label->setText(QString::fromStdString("")); m_check_label->setText(QString::fromStdString(""));
for (const auto* player : Settings::Instance().GetNetPlayClient()->GetPlayers()) auto client = Settings::Instance().GetNetPlayClient();
if (!client)
return;
for (const auto* player : client->GetPlayers())
{ {
m_progress_bars[player->pid] = new QProgressBar; m_progress_bars[player->pid] = new QProgressBar;
m_status_labels[player->pid] = new QLabel; m_status_labels[player->pid] = new QLabel;
@ -118,7 +126,8 @@ void MD5Dialog::SetResult(int pid, const std::string& result)
m_results.push_back(result); m_results.push_back(result);
if (m_results.size() >= Settings::Instance().GetNetPlayClient()->GetPlayers().size()) auto client = Settings::Instance().GetNetPlayClient();
if (client && m_results.size() >= client->GetPlayers().size())
{ {
if (std::adjacent_find(m_results.begin(), m_results.end(), std::not_equal_to<>()) == if (std::adjacent_find(m_results.begin(), m_results.end(), std::not_equal_to<>()) ==
m_results.end()) m_results.end())
@ -134,7 +143,7 @@ void MD5Dialog::SetResult(int pid, const std::string& result)
void MD5Dialog::reject() void MD5Dialog::reject()
{ {
auto* server = Settings::Instance().GetNetPlayServer(); auto server = Settings::Instance().GetNetPlayServer();
if (server) if (server)
server->AbortMD5(); server->AbortMD5();

View File

@ -240,8 +240,9 @@ void NetPlayDialog::ConnectWidgets()
if (value == m_buffer_size) if (value == m_buffer_size)
return; return;
if (Settings::Instance().GetNetPlayServer() != nullptr) auto server = Settings::Instance().GetNetPlayServer();
Settings::Instance().GetNetPlayServer()->AdjustPadBufferSize(value); if (server)
server->AdjustPadBufferSize(value);
}); });
connect(m_start_button, &QPushButton::clicked, this, &NetPlayDialog::OnStart); connect(m_start_button, &QPushButton::clicked, this, &NetPlayDialog::OnStart);
@ -371,7 +372,10 @@ void NetPlayDialog::show(std::string nickname, bool use_traversal)
void NetPlayDialog::UpdateGUI() void NetPlayDialog::UpdateGUI()
{ {
auto* client = Settings::Instance().GetNetPlayClient(); auto client = Settings::Instance().GetNetPlayClient();
auto server = Settings::Instance().GetNetPlayServer();
if (!client)
return;
// Update Player List // Update Player List
const auto players = client->GetPlayers(); const auto players = client->GetPlayers();
@ -466,11 +470,10 @@ void NetPlayDialog::UpdateGUI()
break; break;
} }
} }
else if (Settings::Instance().GetNetPlayServer()) else if (server)
{ {
m_hostcode_label->setText( m_hostcode_label->setText(QString::fromStdString(
QString::fromStdString(Settings::Instance().GetNetPlayServer()->GetInterfaceHost( server->GetInterfaceHost(m_room_box->currentData().toString().toStdString())));
m_room_box->currentData().toString().toStdString())));
m_hostcode_action_button->setText(tr("Copy")); m_hostcode_action_button->setText(tr("Copy"));
m_hostcode_action_button->setEnabled(true); m_hostcode_action_button->setEnabled(true);
} }
@ -555,7 +558,7 @@ void NetPlayDialog::GameStatusChanged(bool running)
void NetPlayDialog::SetOptionsEnabled(bool enabled) void NetPlayDialog::SetOptionsEnabled(bool enabled)
{ {
if (Settings::Instance().GetNetPlayServer() != nullptr) if (Settings::Instance().GetNetPlayServer())
{ {
m_start_button->setEnabled(enabled); m_start_button->setEnabled(enabled);
m_game_button->setEnabled(enabled); m_game_button->setEnabled(enabled);
@ -574,7 +577,9 @@ void NetPlayDialog::OnMsgStartGame()
DisplayMessage(tr("Started game"), "green"); DisplayMessage(tr("Started game"), "green");
QueueOnObject(this, [this] { QueueOnObject(this, [this] {
Settings::Instance().GetNetPlayClient()->StartGame(FindGame(m_current_game)); auto client = Settings::Instance().GetNetPlayClient();
if (client)
client->StartGame(FindGame(m_current_game));
}); });
} }

View File

@ -59,8 +59,8 @@ void PadMappingDialog::ConnectWidgets()
int PadMappingDialog::exec() int PadMappingDialog::exec()
{ {
auto* client = Settings::Instance().GetNetPlayClient(); auto client = Settings::Instance().GetNetPlayClient();
auto* server = Settings::Instance().GetNetPlayServer(); auto server = Settings::Instance().GetNetPlayServer();
// Load Settings // Load Settings
m_players = client->GetPlayers(); m_players = client->GetPlayers();
m_pad_mapping = server->GetPadMapping(); m_pad_mapping = server->GetPadMapping();

View File

@ -276,9 +276,9 @@ GameListModel* Settings::GetGameListModel() const
return model; return model;
} }
NetPlay::NetPlayClient* Settings::GetNetPlayClient() std::shared_ptr<NetPlay::NetPlayClient> Settings::GetNetPlayClient()
{ {
return m_client.get(); return m_client;
} }
void Settings::ResetNetPlayClient(NetPlay::NetPlayClient* client) void Settings::ResetNetPlayClient(NetPlay::NetPlayClient* client)
@ -286,9 +286,9 @@ void Settings::ResetNetPlayClient(NetPlay::NetPlayClient* client)
m_client.reset(client); m_client.reset(client);
} }
NetPlay::NetPlayServer* Settings::GetNetPlayServer() std::shared_ptr<NetPlay::NetPlayServer> Settings::GetNetPlayServer()
{ {
return m_server.get(); return m_server;
} }
void Settings::ResetNetPlayServer(NetPlay::NetPlayServer* server) void Settings::ResetNetPlayServer(NetPlay::NetPlayServer* server)

View File

@ -25,7 +25,7 @@ namespace NetPlay
{ {
class NetPlayClient; class NetPlayClient;
class NetPlayServer; class NetPlayServer;
} } // namespace NetPlay
class GameListModel; class GameListModel;
class InputConfig; class InputConfig;
@ -98,9 +98,9 @@ public:
void DecreaseVolume(int volume); void DecreaseVolume(int volume);
// NetPlay // NetPlay
NetPlay::NetPlayClient* GetNetPlayClient(); std::shared_ptr<NetPlay::NetPlayClient> GetNetPlayClient();
void ResetNetPlayClient(NetPlay::NetPlayClient* client = nullptr); void ResetNetPlayClient(NetPlay::NetPlayClient* client = nullptr);
NetPlay::NetPlayServer* GetNetPlayServer(); std::shared_ptr<NetPlay::NetPlayServer> GetNetPlayServer();
void ResetNetPlayServer(NetPlay::NetPlayServer* server = nullptr); void ResetNetPlayServer(NetPlay::NetPlayServer* server = nullptr);
// Cheats // Cheats
@ -169,8 +169,8 @@ signals:
private: private:
bool m_batch = false; bool m_batch = false;
bool m_controller_state_needed = false; bool m_controller_state_needed = false;
std::unique_ptr<NetPlay::NetPlayClient> m_client; std::shared_ptr<NetPlay::NetPlayClient> m_client;
std::unique_ptr<NetPlay::NetPlayServer> m_server; std::shared_ptr<NetPlay::NetPlayServer> m_server;
Settings(); Settings();
}; };