From 254776dd6c41379e380273500fd5c4ecb7d8b418 Mon Sep 17 00:00:00 2001 From: YuSanka Date: Fri, 31 Jan 2025 23:38:33 +0100 Subject: [PATCH 1/3] Fixed some of detected memory leaks --- src/slic3r/GUI/Plater.cpp | 8 ++++---- src/slic3r/GUI/Tab.cpp | 4 ++-- src/slic3r/GUI/TopBarMenus.cpp | 2 +- src/slic3r/GUI/UserAccountCommunication.cpp | 6 ++++++ src/slic3r/GUI/wxExtensions.cpp | 4 ++++ src/slic3r/Utils/PresetUpdaterWrapper.cpp | 5 +++++ 6 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/slic3r/GUI/Plater.cpp b/src/slic3r/GUI/Plater.cpp index 62d0bf3608..5184d4d708 100644 --- a/src/slic3r/GUI/Plater.cpp +++ b/src/slic3r/GUI/Plater.cpp @@ -694,11 +694,11 @@ void Plater::priv::init() hsizer->Add(sidebar, 0, wxEXPAND | wxLEFT | wxRIGHT, 0); q->SetSizer(hsizer); - menus.init(q); - - // Events: - if (wxGetApp().is_editor()) { + menus.init(q); + + // Events: + // Preset change event this->q->Bind(EVT_OBJ_LIST_OBJECT_SELECT, [this](wxEvent&) { priv::selection_changed(); }); this->q->Bind(EVT_SCHEDULE_BACKGROUND_PROCESS, [this](SimpleEvent&) { this->schedule_background_process(); }); diff --git a/src/slic3r/GUI/Tab.cpp b/src/slic3r/GUI/Tab.cpp index 7b3787b0a5..2b1d1be57d 100644 --- a/src/slic3r/GUI/Tab.cpp +++ b/src/slic3r/GUI/Tab.cpp @@ -5507,9 +5507,9 @@ void TabSLAMaterial::build_tilt_group(Slic3r::GUI::PageShp page) // TRN: 'Profile' in this context denotes a group of parameters used to configure // layer separation procedure for SLA printers. auto optgroup = page->new_optgroup(L("Profile settings")); - optgroup->on_change = [this, optgroup](const t_config_option_key& key, boost::any value) + optgroup->on_change = [this](const t_config_option_key& key, boost::any value) { - if (key.find_first_of("use_tilt") == 0) + if (key.find("use_tilt") == 0) toggle_tilt_options(key == "use_tilt#0"); update_dirty(); diff --git a/src/slic3r/GUI/TopBarMenus.cpp b/src/slic3r/GUI/TopBarMenus.cpp index 89324b3a85..87d4a95012 100644 --- a/src/slic3r/GUI/TopBarMenus.cpp +++ b/src/slic3r/GUI/TopBarMenus.cpp @@ -106,7 +106,7 @@ void TopBarMenus::UpdateAccountMenu() void TopBarMenus::RemoveHideLoginItem() { if (m_hide_login_item) - account.Remove(m_hide_login_item); + account.Destroy(m_hide_login_item); } void TopBarMenus::Popup(TopBarItemsCtrl* popup_ctrl, wxMenu* menu, wxPoint pos) diff --git a/src/slic3r/GUI/UserAccountCommunication.cpp b/src/slic3r/GUI/UserAccountCommunication.cpp index c7eb09efc0..ff386ff1a0 100644 --- a/src/slic3r/GUI/UserAccountCommunication.cpp +++ b/src/slic3r/GUI/UserAccountCommunication.cpp @@ -225,7 +225,13 @@ UserAccountCommunication::UserAccountCommunication(wxEvtHandler* evt_handler, Ap UserAccountCommunication::~UserAccountCommunication() { m_token_timer->Stop(); + delete m_token_timer; + m_token_timer = nullptr; + m_polling_timer->Stop(); + delete m_polling_timer; + m_polling_timer = nullptr; + if (m_thread.joinable()) { // Stop the worker thread, if running. { diff --git a/src/slic3r/GUI/wxExtensions.cpp b/src/slic3r/GUI/wxExtensions.cpp index a4bfe74f38..aa29db9ae2 100644 --- a/src/slic3r/GUI/wxExtensions.cpp +++ b/src/slic3r/GUI/wxExtensions.cpp @@ -117,6 +117,10 @@ wxMenuItem* append_menu_item(wxMenu* menu, int id, const wxString& string, const entry->SetMenuItem(item); accelerator_entries_cache().push_back(entry); } + else { + delete entry; + entry = nullptr; + } } #endif diff --git a/src/slic3r/Utils/PresetUpdaterWrapper.cpp b/src/slic3r/Utils/PresetUpdaterWrapper.cpp index 51123dd268..ca98f15f38 100644 --- a/src/slic3r/Utils/PresetUpdaterWrapper.cpp +++ b/src/slic3r/Utils/PresetUpdaterWrapper.cpp @@ -27,6 +27,11 @@ PresetUpdaterWrapper::~PresetUpdaterWrapper() if (m_ui_status) m_ui_status->set_canceled(true); m_worker_thread.join(); + + if (m_ui_status) { + delete m_ui_status; + m_ui_status = nullptr; + } } } From f1261f8e0167733a735fd730e6cd1c5dd797b099 Mon Sep 17 00:00:00 2001 From: David Kocik Date: Wed, 5 Feb 2025 13:08:30 +0100 Subject: [PATCH 2/3] PresetUpdaterWrapper as resetable unique pointer. --- src/slic3r/Utils/PresetUpdaterWrapper.cpp | 119 ++++++++++++---------- src/slic3r/Utils/PresetUpdaterWrapper.hpp | 6 +- 2 files changed, 69 insertions(+), 56 deletions(-) diff --git a/src/slic3r/Utils/PresetUpdaterWrapper.cpp b/src/slic3r/Utils/PresetUpdaterWrapper.cpp index ca98f15f38..645e2be1cf 100644 --- a/src/slic3r/Utils/PresetUpdaterWrapper.cpp +++ b/src/slic3r/Utils/PresetUpdaterWrapper.cpp @@ -19,6 +19,7 @@ wxDEFINE_EVENT(EVT_CONFIG_UPDATER_SYNC_DONE, wxCommandEvent); PresetUpdaterWrapper::PresetUpdaterWrapper() : m_preset_updater(std::make_unique()) , m_preset_archive_database(std::make_unique()) + , m_ui_status(std::make_unique()) { } PresetUpdaterWrapper::~PresetUpdaterWrapper() @@ -27,11 +28,6 @@ PresetUpdaterWrapper::~PresetUpdaterWrapper() if (m_ui_status) m_ui_status->set_canceled(true); m_worker_thread.join(); - - if (m_ui_status) { - delete m_ui_status; - m_ui_status = nullptr; - } } } @@ -40,27 +36,28 @@ bool PresetUpdaterWrapper::wizard_sync(const PresetBundle* preset_bundle, const assert(!m_modal_thread.joinable()); // Cancel sync before starting wizard to prevent two downloads at same time. cancel_worker_thread(); - PresetUpdaterUIStatus* ui_status = new PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_5_TRIES); - Slic3r::ScopeGuard sg([ui_status]() { delete ui_status; }); - GUI::ProgressUpdaterDialog* dialog = new GUI::ProgressUpdaterDialog(ui_status, parent, headline); - ui_status->set_handler(dialog); + + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_5_TRIES); + + GUI::ProgressUpdaterDialog* dialog = new GUI::ProgressUpdaterDialog(m_ui_status.get(), parent, headline); + m_ui_status->set_handler(dialog); VendorMap vendors_copy = preset_bundle->vendors; - auto worker_body = [ui_status, this, vendors_copy, full_sync]() + auto worker_body = [this, vendors_copy, full_sync]() { - if (!m_preset_archive_database->sync_blocking(ui_status)) { - ui_status->end(); + if (!m_preset_archive_database->sync_blocking(m_ui_status.get())) { + m_ui_status->end(); return; } - if (ui_status->get_canceled()) { ui_status->end(); return; } + if (m_ui_status->get_canceled()) { m_ui_status->end(); return; } if (full_sync) { // Since there might be new repos, we need to sync preset updater const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - m_preset_updater->sync_blocking(vendors_copy, repos, ui_status); - if (ui_status->get_canceled()) { ui_status->end(); return; } + m_preset_updater->sync_blocking(vendors_copy, repos, m_ui_status.get()); + if (m_ui_status->get_canceled()) { m_ui_status->end(); return; } m_preset_updater->update_index_db(); } - ui_status->end(); + m_ui_status->end(); }; m_modal_thread = std::thread(worker_body); // We need to call ShowModal here instead of prompting it from event callback. @@ -70,28 +67,28 @@ bool PresetUpdaterWrapper::wizard_sync(const PresetBundle* preset_bundle, const m_modal_thread.join(); parent->RemoveChild(dialog); dialog->Destroy(); - ui_status->set_handler(nullptr); + m_ui_status->set_handler(nullptr); // Only now it is possible to work with ui_status, that was previously used in worker thread. - if (std::string s = ui_status->get_error(); !s.empty()) { - std::string err_text = GUI::format(_u8L("Failed to download %1%"), ui_status->get_target()); + if (std::string s = m_ui_status->get_error(); !s.empty()) { + std::string err_text = GUI::format(_u8L("Failed to download %1%"), m_ui_status->get_target()); GUI::ErrorDialog err_msg(nullptr, err_text + "\n\n" + s, false); err_msg.ShowModal(); return false; } // Should m_preset_updater->config_update run even if there is cancel? - if (ui_status->get_canceled() /*&& !full_sync*/) { + if (m_ui_status->get_canceled() /*&& !full_sync*/) { return false; } // Offer update installation. if (full_sync) { const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_TEXT_BOX_YES_NO, repos, ui_status); + m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_TEXT_BOX_YES_NO, repos, m_ui_status.get()); } - bool res = !ui_status->get_canceled(); + bool res = !m_ui_status->get_canceled(); return res; } @@ -99,28 +96,35 @@ PresetUpdater::UpdateResult PresetUpdaterWrapper::check_updates_on_user_request( { assert(!m_modal_thread.joinable()); cancel_worker_thread(); - PresetUpdaterUIStatus* ui_status = new PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_5_TRIES); - Slic3r::ScopeGuard sg([ui_status]() { delete ui_status; }); + + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_5_TRIES); + // TRN: Headline of Progress dialog - GUI::ProgressUpdaterDialog* dialog = new GUI::ProgressUpdaterDialog(ui_status, parent, _L("Checking for Configuration Updates")); - ui_status->set_handler(dialog); + GUI::ProgressUpdaterDialog* dialog = new GUI::ProgressUpdaterDialog(m_ui_status.get(), parent, _L("Checking for Configuration Updates")); + m_ui_status->set_handler(dialog); VendorMap vendors_copy = preset_bundle->vendors; std::string failed_paths; PresetUpdater::UpdateResult updater_result = PresetUpdater::UpdateResult::R_ALL_CANCELED; - auto worker_body = [ui_status, this, vendors_copy, &failed_paths]() + auto worker_body = [this, vendors_copy, &failed_paths]() { - if (!m_preset_archive_database->sync_blocking(ui_status)) { - ui_status->end(); + if (!m_preset_archive_database->sync_blocking(m_ui_status.get())) { + m_ui_status->end(); return; } - if (ui_status->get_canceled()) { ui_status->end(); return; } + if (m_ui_status->get_canceled()) { + m_ui_status->end(); + return; + } m_preset_archive_database->extract_archives_with_check(failed_paths); const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - m_preset_updater->sync_blocking(vendors_copy, repos, ui_status); - if (ui_status->get_canceled()) { ui_status->end(); return; } + m_preset_updater->sync_blocking(vendors_copy, repos, m_ui_status.get()); + if (m_ui_status->get_canceled()) { + m_ui_status->end(); + return; + } m_preset_updater->update_index_db(); - ui_status->end(); + m_ui_status->end(); }; m_modal_thread = std::thread(worker_body); @@ -131,18 +135,18 @@ PresetUpdater::UpdateResult PresetUpdaterWrapper::check_updates_on_user_request( m_modal_thread.join(); parent->RemoveChild(dialog); dialog->Destroy(); - ui_status->set_handler(nullptr); + m_ui_status->set_handler(nullptr); // Only now it is possible to work with ui_status, that was previously used in worker thread. - if (std::string s = ui_status->get_error(); !s.empty()) { - std::string err_text = GUI::format(_u8L("Failed to download %1%"), ui_status->get_target()); + if (std::string s = m_ui_status->get_error(); !s.empty()) { + std::string err_text = GUI::format(_u8L("Failed to download %1%"), m_ui_status->get_target()); GUI::ErrorDialog err_msg(nullptr, s, false); err_msg.ShowModal(); return PresetUpdater::UpdateResult::R_ALL_CANCELED; } - if (ui_status->get_canceled()) { + if (m_ui_status->get_canceled()) { return PresetUpdater::UpdateResult::R_ALL_CANCELED; } @@ -155,7 +159,7 @@ PresetUpdater::UpdateResult PresetUpdaterWrapper::check_updates_on_user_request( err_msg.ShowModal(); } // preset_updater::config_update does show wxDialog - updater_result = m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_TEXT_BOX, m_preset_archive_database->get_selected_archive_repositories(), ui_status); + updater_result = m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_TEXT_BOX, m_preset_archive_database->get_selected_archive_repositories(), m_ui_status.get()); return updater_result; } @@ -164,10 +168,11 @@ PresetUpdater::UpdateResult PresetUpdaterWrapper::check_updates_on_startup(const if (m_modal_thread.joinable()) { return PresetUpdater::UpdateResult::R_ALL_CANCELED; } - PresetUpdaterUIStatus ui_status(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + // preset_updater::config_update does show wxDialog m_preset_updater->update_index_db(); - return m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_NOTIFICATION, m_preset_archive_database->get_selected_archive_repositories(), &ui_status); + return m_preset_updater->config_update(old_slic3r_version, PresetUpdater::UpdateParams::SHOW_NOTIFICATION, m_preset_archive_database->get_selected_archive_repositories(), m_ui_status.get()); } void PresetUpdaterWrapper::on_update_notification_confirm() @@ -175,29 +180,30 @@ void PresetUpdaterWrapper::on_update_notification_confirm() if (m_modal_thread.joinable()) { return; } - PresetUpdaterUIStatus ui_status(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + // preset_updater::on_update_notification_confirm does show wxDialog const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - m_preset_updater->on_update_notification_confirm(repos, &ui_status); + m_preset_updater->on_update_notification_confirm(repos, m_ui_status.get()); } bool PresetUpdaterWrapper::install_bundles_rsrc_or_cache_vendor(std::vector bundles, bool snapshot/* = true*/) const { - PresetUpdaterUIStatus ui_status(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); - const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - return m_preset_updater->install_bundles_rsrc_or_cache_vendor(bundles, repos, &ui_status, snapshot); + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); + return m_preset_updater->install_bundles_rsrc_or_cache_vendor(bundles, repos, m_ui_status.get(), snapshot); } void PresetUpdaterWrapper::sync_preset_updater(wxEvtHandler* end_evt_handler, const PresetBundle* preset_bundle) { cancel_worker_thread(); - m_ui_status = new PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); + m_ui_status->reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY); VendorMap vendors_copy = preset_bundle->vendors; auto worker_body = [ this, vendors_copy, end_evt_handler]() { const SharedArchiveRepositoryVector &repos = m_preset_archive_database->get_selected_archive_repositories(); - m_preset_updater->sync_blocking(vendors_copy, repos, this->m_ui_status); + m_preset_updater->sync_blocking(vendors_copy, repos, m_ui_status.get()); if (this->m_ui_status->get_canceled()) { return; } wxCommandEvent* evt = new wxCommandEvent(EVT_CONFIG_UPDATER_SYNC_DONE); wxQueueEvent(end_evt_handler, evt); @@ -214,11 +220,6 @@ void PresetUpdaterWrapper::cancel_worker_thread() } else assert(false); m_worker_thread.join(); - - if (m_ui_status) { - delete m_ui_status; - m_ui_status = nullptr; - } } } @@ -226,7 +227,11 @@ const std::map Pr {PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_5_TRIES, {500ms, 5s, 4}}, {PresetUpdaterUIStatus::PresetUpdaterRetryPolicy::PURP_NO_RETRY, {0ms}} }; -PresetUpdaterUIStatus::PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy policy) +PresetUpdaterUIStatus::PresetUpdaterUIStatus() +{ +} + +void PresetUpdaterUIStatus::reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy policy) { if (auto it = policy_map.find(policy); it != policy_map.end()) { m_retry_policy = it->second; @@ -234,7 +239,13 @@ PresetUpdaterUIStatus::PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdate assert(false); m_retry_policy = {0ms}; } + + m_canceled = false; + m_evt_handler = nullptr; + m_error_msg.clear(); + m_target.clear(); } + bool PresetUpdaterUIStatus::on_attempt(int attempt, unsigned delay) { if (attempt == 1) { @@ -279,7 +290,7 @@ void ProgressUpdaterDialog::on_set_status(const PresetUpdaterStatusMessageEvent& { if (!Pulse(evt.data)) { set_cancel(true); - } + } } void ProgressUpdaterDialog::on_end(const PresetUpdaterStatusSimpleEvent& evt) { diff --git a/src/slic3r/Utils/PresetUpdaterWrapper.hpp b/src/slic3r/Utils/PresetUpdaterWrapper.hpp index bf21df057e..328b71de20 100644 --- a/src/slic3r/Utils/PresetUpdaterWrapper.hpp +++ b/src/slic3r/Utils/PresetUpdaterWrapper.hpp @@ -40,10 +40,12 @@ public: PURP_NO_RETRY, }; // called from PresetUpdaterWrapper - PresetUpdaterUIStatus(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy policy); + PresetUpdaterUIStatus(); ~PresetUpdaterUIStatus(){} void set_handler(wxEvtHandler* evt_handler) {m_evt_handler = evt_handler;} + void reset(PresetUpdaterUIStatus::PresetUpdaterRetryPolicy policy); + // called from worker thread bool on_attempt(int attempt, unsigned delay); void set_target(const std::string& target); @@ -152,7 +154,7 @@ private: // m_worker_thread runs on background while m_modal_thread runs only when modal window exists. std::thread m_worker_thread; - PresetUpdaterUIStatus* m_ui_status {nullptr}; + std::unique_ptr m_ui_status; std::thread m_modal_thread; }; From 292885e9d14da90bf4582e56caadb2b73d41a970 Mon Sep 17 00:00:00 2001 From: David Kocik Date: Wed, 5 Feb 2025 13:29:00 +0100 Subject: [PATCH 3/3] Timer unique pointer --- src/slic3r/GUI/UserAccountCommunication.cpp | 9 ++------- src/slic3r/GUI/UserAccountCommunication.hpp | 4 ++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/slic3r/GUI/UserAccountCommunication.cpp b/src/slic3r/GUI/UserAccountCommunication.cpp index ff386ff1a0..395986400d 100644 --- a/src/slic3r/GUI/UserAccountCommunication.cpp +++ b/src/slic3r/GUI/UserAccountCommunication.cpp @@ -177,8 +177,8 @@ UserAccountCommunication::UserAccountCommunication(wxEvtHandler* evt_handler, Ap : wxEvtHandler() , m_evt_handler(evt_handler) , m_app_config(app_config) - , m_polling_timer(new wxTimer(this)) - , m_token_timer(new wxTimer(this)) + , m_polling_timer(std::make_unique(this)) + , m_token_timer(std::make_unique(this)) { Bind(wxEVT_TIMER, &UserAccountCommunication::on_token_timer, this, m_token_timer->GetId()); Bind(wxEVT_TIMER, &UserAccountCommunication::on_polling_timer, this, m_polling_timer->GetId()); @@ -225,12 +225,7 @@ UserAccountCommunication::UserAccountCommunication(wxEvtHandler* evt_handler, Ap UserAccountCommunication::~UserAccountCommunication() { m_token_timer->Stop(); - delete m_token_timer; - m_token_timer = nullptr; - m_polling_timer->Stop(); - delete m_polling_timer; - m_polling_timer = nullptr; if (m_thread.joinable()) { // Stop the worker thread, if running. diff --git a/src/slic3r/GUI/UserAccountCommunication.hpp b/src/slic3r/GUI/UserAccountCommunication.hpp index 96904e28a5..645af09961 100644 --- a/src/slic3r/GUI/UserAccountCommunication.hpp +++ b/src/slic3r/GUI/UserAccountCommunication.hpp @@ -86,7 +86,7 @@ private: bool m_thread_stop { false }; bool m_thread_wakeup{ false }; bool m_window_is_active{ true }; - wxTimer* m_polling_timer; + std::unique_ptr m_polling_timer; std::string m_code_verifier; wxEvtHandler* m_evt_handler; @@ -95,7 +95,7 @@ private: std::string m_username; bool m_remember_session { true }; // if default is true, on every login Remember me will be checked. - wxTimer* m_token_timer; + std::unique_ptr m_token_timer; std::time_t m_next_token_refresh_at{0}; void wakeup_session_thread();