Skip to content

Commit

Permalink
PD: Fix crash when adding sketch to loft via tree view
Browse files Browse the repository at this point in the history
The underlying problem is the method DocumentItem::updateItemSelection() where
the selection is altered. This may cause the destruction and recreation of the
DocumentObjectItems so that the passed pointer can become dangling.

The issue is fixed in two steps:
1. Add the method 'DocumentObjectItem *findItem(App::DocumentObject* obj, const std::string& subname) const'
   to safely re-access the item.
2. Add a boolean flag 'dirtyFlag' and the methods insertItem() and removeItem() to DocumentObjectData.
   This is needed to check when the iterator over the container becomes invalid.
  • Loading branch information
wwmayer authored and yorikvanhavre committed May 6, 2024
1 parent 92a9b30 commit 4ea1ad5
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 12 deletions.
91 changes: 79 additions & 12 deletions src/Gui/Tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ using DocumentObjectItems = std::set<DocumentObjectItem*>;

class Gui::DocumentObjectData {
public:
bool dirtyFlag {};
DocumentItem* docItem;
DocumentObjectItems items;
ViewProviderDocumentObject* viewObject;
Expand Down Expand Up @@ -243,6 +244,24 @@ class Gui::DocumentObjectData {
label2 = viewObject->getObject()->Label2.getValue();
}

void insertItem(DocumentObjectItem* item)
{
items.insert(item);
dirtyFlag = true;
}

void removeItem(DocumentObjectItem* item)
{
auto it = items.find(item);
if (it == items.end()) {
assert(0);
}
else {
items.erase(it);
dirtyFlag = true;
}
}

const char* getTreeName() const {
return docItem->getTreeName();
}
Expand Down Expand Up @@ -4544,12 +4563,18 @@ void DocumentItem::clearSelection(DocumentObjectItem* exclude)
// Block signals here otherwise we get a recursion and quadratic runtime
bool ok = treeWidget()->blockSignals(true);
FOREACH_ITEM_ALL(item);
_v.second->dirtyFlag = false;
if (item == exclude) {
if (item->selected > 0)
item->selected = -1;
else
item->selected = 0;
updateItemSelection(item);
// The set has been changed while calling updateItemSelection
// so that the iterator has become invalid -> Abort
if (_v.second->dirtyFlag) {
break;
}
}
else {
item->selected = 0;
Expand Down Expand Up @@ -4586,7 +4611,13 @@ void DocumentItem::updateSelection(QTreeWidgetItem* ti, bool unselect) {
updateSelection(ti->child(i));
}

void DocumentItem::updateItemSelection(DocumentObjectItem* item) {
void DocumentItem::updateItemSelection(DocumentObjectItem* item)
{
// Note: In several places of this function the selection is altered and the notification of
// the selection observers can trigger a recreation of all DocumentObjectItem so that the
// passed 'item' can become a dangling pointer.
// Thus,'item' mustn't be accessed any more after altering the selection.
// For further details see the bug analsysis of #13107
bool selected = item->isSelected();
bool checked = item->checkState(0) == Qt::Checked;

Expand Down Expand Up @@ -4654,23 +4685,30 @@ void DocumentItem::updateItemSelection(DocumentObjectItem* item) {
Gui::Selection().rmvSelection(docname, objname, subname.c_str());
return;
}

auto vobj = item->object();
selected = false;
if (!item->mySubs.empty()) {
for (auto& sub : item->mySubs) {
if (Gui::Selection().addSelection(docname, objname, (subname + sub).c_str()))
if (Gui::Selection().addSelection(docname, objname, (subname + sub).c_str())) {
selected = true;
}
}
}
if (!selected) {
item->mySubs.clear();
if (!Gui::Selection().addSelection(docname, objname, subname.c_str())) {
item->selected = 0;
item->setSelected(false);
item->setCheckState(false);
// Safely re-access the item
DocumentObjectItem* item2 = findItem(vobj->getObject(), subname);
if (item2) {
item2->selected = 0;
item2->setSelected(false);
item2->setCheckState(false);
}
return;
}
}
getTree()->syncView(item->object());
getTree()->syncView(vobj);
}

App::DocumentObject* DocumentItem::getTopParent(App::DocumentObject* obj, std::string& subname) {
Expand Down Expand Up @@ -4717,6 +4755,39 @@ App::DocumentObject* DocumentItem::getTopParent(App::DocumentObject* obj, std::s
return topParent;
}

DocumentObjectItem *DocumentItem::findItem(App::DocumentObject* obj, const std::string& subname) const
{
auto it = ObjectMap.find(obj);
if (it == ObjectMap.end()) {
return nullptr;
}

// There is only one instance in the tree view
if (it->second->items.size() == 1) {
return *(it->second->items.begin());
}

// If there are multiple instances use the one with the same subname
DocumentObjectItem* item {};
for (auto jt : it->second->items) {
std::ostringstream str;
App::DocumentObject* topParent = nullptr;
jt->getSubName(str, topParent);
if (topParent) {
if (!obj->redirectSubName(str, topParent, nullptr)) {
str << obj->getNameInDocument() << '.';
}
}

if (subname == str.str()) {
item = jt;
break;
}
}

return item;
}

DocumentObjectItem* DocumentItem::findItemByObject(
bool sync, App::DocumentObject* obj, const char* subname, bool select)
{
Expand Down Expand Up @@ -5017,7 +5088,7 @@ DocumentObjectItem::DocumentObjectItem(DocumentItem* ownerDocItem, DocumentObjec
setFlags(flags() | Qt::ItemIsEditable | Qt::ItemIsUserCheckable);
setCheckState(false);

myData->items.insert(this);
myData->insertItem(this);
++countItems;
TREE_LOG("Create item: " << countItems << ", " << object()->getObject()->getFullName());
}
Expand All @@ -5026,11 +5097,7 @@ DocumentObjectItem::~DocumentObjectItem()
{
--countItems;
TREE_LOG("Delete item: " << countItems << ", " << object()->getObject()->getFullName());
auto it = myData->items.find(this);
if (it == myData->items.end())
assert(0);
else
myData->items.erase(it);
myData->removeItem(this);

if (myData->rootItem == this)
myData->rootItem = nullptr;
Expand Down
1 change: 1 addition & 0 deletions src/Gui/Tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ class DocumentItem : public QTreeWidgetItem, public Base::Persistence
App::DocumentObject *obj, const char *subname, bool select=false);

DocumentObjectItem *findItem(bool sync, DocumentObjectItem *item, const char *subname, bool select=true);
DocumentObjectItem *findItem(App::DocumentObject* obj, const std::string& subname) const;

App::DocumentObject *getTopParent(App::DocumentObject *obj, std::string &subname);

Expand Down

0 comments on commit 4ea1ad5

Please sign in to comment.