From f2780320cc9d782835479d6b1ee4239d73944c16 Mon Sep 17 00:00:00 2001 From: wmayer Date: Tue, 18 Mar 2025 11:41:18 +0100 Subject: [PATCH 1/3] Gui: Fix crash when creating a LCS This is a left-over of the regressions introduced with PR 18126. Thanks to some moderinization of the code base and replacing static with dynamic casts undefined behaviour has changed to well-defined behaviour but now unchecked null pointers. This change does some extra null pointer checks and uses the now correct types for down casting. Hint: Upstream still uses many static casts here that already cause undefined behaviour when creating a LCS. This could be the reason for the possible crashes when deleting a LCS as described in 20261 # Conflicts: # src/Gui/ViewProviderCoordinateSystem.cpp --- src/Gui/ViewProviderCoordinateSystem.cpp | 67 ++++++++++++++---------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index 379a304b76..6ead8919ec 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -72,13 +72,16 @@ ViewProviderCoordinateSystem::~ViewProviderCoordinateSystem() { std::vector ViewProviderCoordinateSystem::claimChildren() const { - auto obj = getObject(); - std::vector childs = obj->OriginFeatures.getValues(); - auto it = std::find(childs.begin(), childs.end(), obj); - if (it != childs.end()) { - childs.erase(it); + if (auto obj = getObject()) { + std::vector childs = obj->OriginFeatures.getValues(); + auto it = std::find(childs.begin(), childs.end(), obj); + if (it != childs.end()) { + childs.erase(it); + } + return childs; } - return childs; + + return {}; } std::vector ViewProviderCoordinateSystem::claimChildren3D() const @@ -106,7 +109,10 @@ void ViewProviderCoordinateSystem::setDisplayMode(const char* ModeName) void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements) { - auto origin = getObject(); + auto origin = getObject(); + if (!origin) { + return; + } bool saveState = tempVisMap.empty(); @@ -163,32 +169,34 @@ double ViewProviderCoordinateSystem::defaultSize() return hGrp->GetFloat("DatumsSize", 25); } -bool ViewProviderCoordinateSystem::isTemporaryVisibility() { +bool ViewProviderCoordinateSystem::isTemporaryVisibility() +{ return !tempVisMap.empty(); } void ViewProviderCoordinateSystem::setPlaneLabelVisibility(bool val) { - auto lcs = getObject(); - for (auto* plane : lcs->planes()) { - auto* vp = dynamic_cast( - Gui::Application::Instance->getViewProvider(plane)); - if (vp) { - vp->setLabelVisibility(val); + if (auto lcs = getObject()) { + for (auto* plane : lcs->planes()) { + auto* vp = dynamic_cast( + Gui::Application::Instance->getViewProvider(plane)); + if (vp) { + vp->setLabelVisibility(val); + } } } - } void ViewProviderCoordinateSystem::applyDatumObjects(const DatumObjectFunc& func) { - auto lcs = getObject(); - const auto& objs = lcs->OriginFeatures.getValues(); - for (auto* obj : objs) { - auto* vp = dynamic_cast( - Gui::Application::Instance->getViewProvider(obj)); - if (vp) { - func(vp); + if (auto lcs = getObject()) { + const auto& objs = lcs->OriginFeatures.getValues(); + for (auto* obj : objs) { + auto* vp = dynamic_cast( + Gui::Application::Instance->getViewProvider(obj)); + if (vp) { + func(vp); + } } } } @@ -207,9 +215,9 @@ void ViewProviderCoordinateSystem::resetTemporarySize() }); } -void ViewProviderCoordinateSystem::updateData(const App::Property* prop) { - auto* jcs = dynamic_cast(getObject()); - if(jcs) { +void ViewProviderCoordinateSystem::updateData(const App::Property* prop) +{ + if (auto* jcs = dynamic_cast(getObject())) { if (prop == &jcs->Placement) { // Update position } @@ -217,11 +225,14 @@ void ViewProviderCoordinateSystem::updateData(const App::Property* prop) { ViewProviderDocumentObject::updateData(prop); } -bool ViewProviderCoordinateSystem::onDelete(const std::vector &) { +bool ViewProviderCoordinateSystem::onDelete(const std::vector &) +{ auto lcs = getObject(); + if (!lcs) { + return false; + } - auto origin = dynamic_cast(lcs); - if (origin && !origin->getInList().empty()) { + if (lcs && !lcs->getInList().empty()) { // Do not allow deletion of origin objects that are not lost. return false; } From 4c66d2e3d4c26fff76311b07977c0a1f00cf599c Mon Sep 17 00:00:00 2001 From: Benjamin Nauck Date: Wed, 11 Jun 2025 08:31:10 +0200 Subject: [PATCH 2/3] Gui: Refactor LCS fixes * use early exit to highlight main execution path * use `auto*` for pointer * use getObject() instead of casting result from getObject() * remove empty updateData() * rename origin to lcs in setTemporaryVisibility for clarity --- src/Gui/ViewProviderCoordinateSystem.cpp | 73 +++++++++++------------- src/Gui/ViewProviderCoordinateSystem.h | 1 - 2 files changed, 34 insertions(+), 40 deletions(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index 6ead8919ec..f5b7c3f1cc 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -72,16 +72,17 @@ ViewProviderCoordinateSystem::~ViewProviderCoordinateSystem() { std::vector ViewProviderCoordinateSystem::claimChildren() const { - if (auto obj = getObject()) { - std::vector childs = obj->OriginFeatures.getValues(); - auto it = std::find(childs.begin(), childs.end(), obj); - if (it != childs.end()) { - childs.erase(it); - } - return childs; + auto* lcs = getObject(); + if (!lcs) { + return {}; } - return {}; + std::vector childs = lcs->OriginFeatures.getValues(); + auto it = std::find(childs.begin(), childs.end(), lcs); + if (it != childs.end()) { + childs.erase(it); + } + return childs; } std::vector ViewProviderCoordinateSystem::claimChildren3D() const @@ -109,8 +110,8 @@ void ViewProviderCoordinateSystem::setDisplayMode(const char* ModeName) void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements) { - auto origin = getObject(); - if (!origin) { + auto* lcs = getObject(); + if (!lcs) { return; } @@ -118,7 +119,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements try { // Remember & Set axis visibility - for(App::DocumentObject* obj : origin->axes()) { + for(App::DocumentObject* obj : lcs->axes()) { if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { if (saveState) { tempVisMap[vp] = vp->isVisible(); @@ -128,7 +129,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements } // Remember & Set plane visibility - for(App::DocumentObject* obj : origin->planes()) { + for(App::DocumentObject* obj : lcs->planes()) { if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { if (saveState) { tempVisMap[vp] = vp->isVisible(); @@ -138,7 +139,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements } // Remember & Set origin point visibility - App::DocumentObject* obj = origin->getOrigin(); + App::DocumentObject* obj = lcs->getOrigin(); if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { if (saveState) { tempVisMap[vp] = vp->isVisible(); @@ -176,27 +177,31 @@ bool ViewProviderCoordinateSystem::isTemporaryVisibility() void ViewProviderCoordinateSystem::setPlaneLabelVisibility(bool val) { - if (auto lcs = getObject()) { - for (auto* plane : lcs->planes()) { - auto* vp = dynamic_cast( - Gui::Application::Instance->getViewProvider(plane)); - if (vp) { - vp->setLabelVisibility(val); - } + auto* lcs = getObject(); + if (!lcs) { + return; + } + for (auto* plane : lcs->planes()) { + auto* vp = dynamic_cast( + Gui::Application::Instance->getViewProvider(plane)); + if (vp) { + vp->setLabelVisibility(val); } } } void ViewProviderCoordinateSystem::applyDatumObjects(const DatumObjectFunc& func) { - if (auto lcs = getObject()) { - const auto& objs = lcs->OriginFeatures.getValues(); - for (auto* obj : objs) { - auto* vp = dynamic_cast( - Gui::Application::Instance->getViewProvider(obj)); - if (vp) { - func(vp); - } + auto* lcs = getObject(); + if (!lcs) { + return; + } + const auto& objs = lcs->OriginFeatures.getValues(); + for (auto* obj : objs) { + auto* vp = dynamic_cast( + Gui::Application::Instance->getViewProvider(obj)); + if (vp) { + func(vp); } } } @@ -215,19 +220,9 @@ void ViewProviderCoordinateSystem::resetTemporarySize() }); } -void ViewProviderCoordinateSystem::updateData(const App::Property* prop) -{ - if (auto* jcs = dynamic_cast(getObject())) { - if (prop == &jcs->Placement) { - // Update position - } - } - ViewProviderDocumentObject::updateData(prop); -} - bool ViewProviderCoordinateSystem::onDelete(const std::vector &) { - auto lcs = getObject(); + auto* lcs = getObject(); if (!lcs) { return false; } diff --git a/src/Gui/ViewProviderCoordinateSystem.h b/src/Gui/ViewProviderCoordinateSystem.h index f18859fdd4..1b3704799b 100644 --- a/src/Gui/ViewProviderCoordinateSystem.h +++ b/src/Gui/ViewProviderCoordinateSystem.h @@ -100,7 +100,6 @@ public: static const uint32_t defaultColor = 0x3296faff; protected: - void updateData(const App::Property*) override; bool onDelete(const std::vector &) override; private: From f32fdcf48b2b9411163c693dbb18ba18080d498c Mon Sep 17 00:00:00 2001 From: Benjamin Nauck Date: Thu, 12 Jun 2025 10:33:56 +0200 Subject: [PATCH 3/3] Gui: Fix regression pointed out in review paddlestroke: > Here we actually need to keep App::Origin. Because we do not allow deletion ONLY of origin objects. Not of normal LCS. While the original code: ```cpp auto origin = dynamic_cast(lcs); if (origin && !origin->getInList().empty()) { ``` ...handles this perfectly fine, intent isn't obvious when reading it. Using `is()` shows intent better and should avoid similar situations in the future. --- src/Gui/ViewProviderCoordinateSystem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index f5b7c3f1cc..5541779578 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -227,7 +227,7 @@ bool ViewProviderCoordinateSystem::onDelete(const std::vector &) return false; } - if (lcs && !lcs->getInList().empty()) { + if (lcs->is() && !lcs->getInList().empty()) { // Do not allow deletion of origin objects that are not lost. return false; }