From cfeffdcf4208686d64d509f559272d64bbd5da70 Mon Sep 17 00:00:00 2001 From: Techjar Date: Thu, 12 Jul 2018 20:37:12 -0400 Subject: [PATCH] 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. --- Source/Core/Core/NetPlayClient.cpp | 11 ++++++--- Source/Core/DolphinQt/MainWindow.cpp | 4 ++-- Source/Core/DolphinQt/NetPlay/MD5Dialog.cpp | 17 ++++++++++---- .../Core/DolphinQt/NetPlay/NetPlayDialog.cpp | 23 +++++++++++-------- .../DolphinQt/NetPlay/PadMappingDialog.cpp | 4 ++-- Source/Core/DolphinQt/Settings.cpp | 8 +++---- Source/Core/DolphinQt/Settings.h | 10 ++++---- 7 files changed, 48 insertions(+), 29 deletions(-) diff --git a/Source/Core/Core/NetPlayClient.cpp b/Source/Core/Core/NetPlayClient.cpp index d29b931ae0..00f2915951 100644 --- a/Source/Core/Core/NetPlayClient.cpp +++ b/Source/Core/Core/NetPlayClient.cpp @@ -70,6 +70,10 @@ NetPlayClient::~NetPlayClient() 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_thread.join(); } @@ -1636,12 +1640,14 @@ void NetPlayClient::ComputeMD5(const std::string& file_identifier) return; } + if (m_MD5_thread.joinable()) + m_MD5_thread.join(); m_MD5_thread = std::thread([this, file]() { std::string sum = MD5::MD5Sum(file, [&](int progress) { sf::Packet packet; packet << static_cast(NP_MSG_MD5_PROGRESS); packet << progress; - Send(packet); + SendAsync(std::move(packet)); return m_should_compute_MD5; }); @@ -1649,9 +1655,8 @@ void NetPlayClient::ComputeMD5(const std::string& file_identifier) sf::Packet packet; packet << static_cast(NP_MSG_MD5_RESULT); packet << sum; - Send(packet); + SendAsync(std::move(packet)); }); - m_MD5_thread.detach(); } const PadMappingArray& NetPlayClient::GetPadMapping() const diff --git a/Source/Core/DolphinQt/MainWindow.cpp b/Source/Core/DolphinQt/MainWindow.cpp index 30fdaef58f..f6f7ce1f9e 100644 --- a/Source/Core/DolphinQt/MainWindow.cpp +++ b/Source/Core/DolphinQt/MainWindow.cpp @@ -668,7 +668,7 @@ bool MainWindow::RequestStop() const Core::State state = Core::GetState(); // Only pause the game, if NetPlay is not running - bool pause = Settings::Instance().GetNetPlayClient() == nullptr; + bool pause = !Settings::Instance().GetNetPlayClient(); if (pause) Core::SetState(Core::State::Paused); @@ -1071,7 +1071,7 @@ bool MainWindow::NetPlayJoin() std::string host_ip; u16 host_port; - if (Settings::Instance().GetNetPlayServer() != nullptr) + if (Settings::Instance().GetNetPlayServer()) { host_ip = "127.0.0.1"; host_port = Settings::Instance().GetNetPlayServer()->GetPort(); diff --git a/Source/Core/DolphinQt/NetPlay/MD5Dialog.cpp b/Source/Core/DolphinQt/NetPlay/MD5Dialog.cpp index b86b1e782e..ec8db64fc1 100644 --- a/Source/Core/DolphinQt/NetPlay/MD5Dialog.cpp +++ b/Source/Core/DolphinQt/NetPlay/MD5Dialog.cpp @@ -21,7 +21,11 @@ static QString GetPlayerNameFromPID(int pid) { 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) { @@ -82,7 +86,11 @@ void MD5Dialog::show(const QString& title) m_results.clear(); 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_status_labels[player->pid] = new QLabel; @@ -118,7 +126,8 @@ void MD5Dialog::SetResult(int pid, const std::string& 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<>()) == m_results.end()) @@ -134,7 +143,7 @@ void MD5Dialog::SetResult(int pid, const std::string& result) void MD5Dialog::reject() { - auto* server = Settings::Instance().GetNetPlayServer(); + auto server = Settings::Instance().GetNetPlayServer(); if (server) server->AbortMD5(); diff --git a/Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp b/Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp index ce296acef8..5e3d757eeb 100644 --- a/Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp +++ b/Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp @@ -240,8 +240,9 @@ void NetPlayDialog::ConnectWidgets() if (value == m_buffer_size) return; - if (Settings::Instance().GetNetPlayServer() != nullptr) - Settings::Instance().GetNetPlayServer()->AdjustPadBufferSize(value); + auto server = Settings::Instance().GetNetPlayServer(); + if (server) + server->AdjustPadBufferSize(value); }); 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() { - auto* client = Settings::Instance().GetNetPlayClient(); + auto client = Settings::Instance().GetNetPlayClient(); + auto server = Settings::Instance().GetNetPlayServer(); + if (!client) + return; // Update Player List const auto players = client->GetPlayers(); @@ -466,11 +470,10 @@ void NetPlayDialog::UpdateGUI() break; } } - else if (Settings::Instance().GetNetPlayServer()) + else if (server) { - m_hostcode_label->setText( - QString::fromStdString(Settings::Instance().GetNetPlayServer()->GetInterfaceHost( - m_room_box->currentData().toString().toStdString()))); + m_hostcode_label->setText(QString::fromStdString( + server->GetInterfaceHost(m_room_box->currentData().toString().toStdString()))); m_hostcode_action_button->setText(tr("Copy")); m_hostcode_action_button->setEnabled(true); } @@ -555,7 +558,7 @@ void NetPlayDialog::GameStatusChanged(bool running) void NetPlayDialog::SetOptionsEnabled(bool enabled) { - if (Settings::Instance().GetNetPlayServer() != nullptr) + if (Settings::Instance().GetNetPlayServer()) { m_start_button->setEnabled(enabled); m_game_button->setEnabled(enabled); @@ -574,7 +577,9 @@ void NetPlayDialog::OnMsgStartGame() DisplayMessage(tr("Started game"), "green"); QueueOnObject(this, [this] { - Settings::Instance().GetNetPlayClient()->StartGame(FindGame(m_current_game)); + auto client = Settings::Instance().GetNetPlayClient(); + if (client) + client->StartGame(FindGame(m_current_game)); }); } diff --git a/Source/Core/DolphinQt/NetPlay/PadMappingDialog.cpp b/Source/Core/DolphinQt/NetPlay/PadMappingDialog.cpp index fbfe64e538..d1b7c97ee4 100644 --- a/Source/Core/DolphinQt/NetPlay/PadMappingDialog.cpp +++ b/Source/Core/DolphinQt/NetPlay/PadMappingDialog.cpp @@ -59,8 +59,8 @@ void PadMappingDialog::ConnectWidgets() int PadMappingDialog::exec() { - auto* client = Settings::Instance().GetNetPlayClient(); - auto* server = Settings::Instance().GetNetPlayServer(); + auto client = Settings::Instance().GetNetPlayClient(); + auto server = Settings::Instance().GetNetPlayServer(); // Load Settings m_players = client->GetPlayers(); m_pad_mapping = server->GetPadMapping(); diff --git a/Source/Core/DolphinQt/Settings.cpp b/Source/Core/DolphinQt/Settings.cpp index c1dce6b29d..56183daa86 100644 --- a/Source/Core/DolphinQt/Settings.cpp +++ b/Source/Core/DolphinQt/Settings.cpp @@ -276,9 +276,9 @@ GameListModel* Settings::GetGameListModel() const return model; } -NetPlay::NetPlayClient* Settings::GetNetPlayClient() +std::shared_ptr Settings::GetNetPlayClient() { - return m_client.get(); + return m_client; } void Settings::ResetNetPlayClient(NetPlay::NetPlayClient* client) @@ -286,9 +286,9 @@ void Settings::ResetNetPlayClient(NetPlay::NetPlayClient* client) m_client.reset(client); } -NetPlay::NetPlayServer* Settings::GetNetPlayServer() +std::shared_ptr Settings::GetNetPlayServer() { - return m_server.get(); + return m_server; } void Settings::ResetNetPlayServer(NetPlay::NetPlayServer* server) diff --git a/Source/Core/DolphinQt/Settings.h b/Source/Core/DolphinQt/Settings.h index 68303530fa..377ff11586 100644 --- a/Source/Core/DolphinQt/Settings.h +++ b/Source/Core/DolphinQt/Settings.h @@ -25,7 +25,7 @@ namespace NetPlay { class NetPlayClient; class NetPlayServer; -} +} // namespace NetPlay class GameListModel; class InputConfig; @@ -98,9 +98,9 @@ public: void DecreaseVolume(int volume); // NetPlay - NetPlay::NetPlayClient* GetNetPlayClient(); + std::shared_ptr GetNetPlayClient(); void ResetNetPlayClient(NetPlay::NetPlayClient* client = nullptr); - NetPlay::NetPlayServer* GetNetPlayServer(); + std::shared_ptr GetNetPlayServer(); void ResetNetPlayServer(NetPlay::NetPlayServer* server = nullptr); // Cheats @@ -169,8 +169,8 @@ signals: private: bool m_batch = false; bool m_controller_state_needed = false; - std::unique_ptr m_client; - std::unique_ptr m_server; + std::shared_ptr m_client; + std::shared_ptr m_server; Settings(); };