Problem with Body and abortCommand()
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
-
- Posts: 554
- Joined: Sat Apr 07, 2012 2:42 am
Problem with Body and abortCommand()
It seems that the structure of the Body feature clashes with what abortCommand() does. Here is an example:
I create a Sketch and then click to create a Pad which opens the Pad dialog. Then I choose to reject the dialog. This crashes FC. Here is what happens:
The Body at the point where I click Cancel has the following structure:
Tip -> Pad
Model: [Sketch, Pad]
Then TaskDlgPadParameters::reject() is called, which calls abortCommand().
abortCommand() first removes the Pad by calling Document::_remObject() which only removes the Pad from its internal data structure, but does not handle Properties of other features that might be pointing to the Pad. So now Body.Tip and Body.Model both contain an invalid reference to the Pad.
After this, abortCommand() resets the Tip feature to point to the Sketch. This triggers an onChanged() call which goes to Tree.cpp DocumentItem::slotChangeObject() with the Body's viewprovider as an argument. This method asks the Body to claimChildren(), and since the Body.Model still contains an invalid pointer to the Pad, there is a crash
Because Document::remObject() removes the Pad from any Properties referencing it, there is no crash if I OK the pad dialog, and then delete the Pad from the tree.
Why do we call _remObject() from abortCommand(), and not remObject()?
I create a Sketch and then click to create a Pad which opens the Pad dialog. Then I choose to reject the dialog. This crashes FC. Here is what happens:
The Body at the point where I click Cancel has the following structure:
Tip -> Pad
Model: [Sketch, Pad]
Then TaskDlgPadParameters::reject() is called, which calls abortCommand().
abortCommand() first removes the Pad by calling Document::_remObject() which only removes the Pad from its internal data structure, but does not handle Properties of other features that might be pointing to the Pad. So now Body.Tip and Body.Model both contain an invalid reference to the Pad.
After this, abortCommand() resets the Tip feature to point to the Sketch. This triggers an onChanged() call which goes to Tree.cpp DocumentItem::slotChangeObject() with the Body's viewprovider as an argument. This method asks the Body to claimChildren(), and since the Body.Model still contains an invalid pointer to the Pad, there is a crash
Because Document::remObject() removes the Pad from any Properties referencing it, there is no crash if I OK the pad dialog, and then delete the Pad from the tree.
Why do we call _remObject() from abortCommand(), and not remObject()?
Re: Problem with Body and abortCommand()
Thats not quit right. Normally when you abort a command all changes to the document get reverted. That means also all properties of the other objects.
This kind of error happens mostly when you open accidentally a second command. So when setting the tip and creating the pad is in a separate transaction it wont work. So take a look if you not
close the command before you open the dialog. Or create a new transaction (OpenCommand()), which close the running one....
This kind of error happens mostly when you open accidentally a second command. So when setting the tip and creating the pad is in a separate transaction it wont work. So take a look if you not
close the command before you open the dialog. Or create a new transaction (OpenCommand()), which close the running one....
Stop whining - start coding!
-
- Posts: 554
- Joined: Sat Apr 07, 2012 2:42 am
Re: Problem with Body and abortCommand()
There is one openCommand in Command.cpp, a commitCommand in TaskDlgDatum.cpp::accept() and a abortCommand() in reject() so that should be OK.
But I am setting the Tip and Model in the python method BodyImp.cpp Body::insertFeature() with C++ code. Do I have to use doCommand() instead ?
From what I see, abortCommand() does 3 things:
1. It removes the Pad from the Document
2. It resets the Tip property of the Body
3. It resets the Model property of the Body
The problem is that in step 2 Body::claimChildren() is called which accesses the Model property of the Body, which at that point has a pointer to the already removed Pad stored in it
But I am setting the Tip and Model in the python method BodyImp.cpp Body::insertFeature() with C++ code. Do I have to use doCommand() instead ?
From what I see, abortCommand() does 3 things:
1. It removes the Pad from the Document
2. It resets the Tip property of the Body
3. It resets the Model property of the Body
The problem is that in step 2 Body::claimChildren() is called which accesses the Model property of the Body, which at that point has a pointer to the already removed Pad stored in it
Re: Problem with Body and abortCommand()
Mhh,
normally the Undo should do it exactly the other way around as its created. That shouldn't happen...
If you change a property its get recorded wether you do it via C++ or python!
normally the Undo should do it exactly the other way around as its created. That shouldn't happen...
If you change a property its get recorded wether you do it via C++ or python!
Stop whining - start coding!
-
- Posts: 554
- Joined: Sat Apr 07, 2012 2:42 am
Re: Problem with Body and abortCommand()
But it seems that all deletions are done first, then creation of new objects, then adjustment of properties:the Undo should do it exactly the other way around as its created
Code: Select all
void Transaction::apply(Document &Doc, bool forward)
{
std::map<const DocumentObject*,TransactionObject*>::iterator It;
//for (It= _Objects.begin();It!=_Objects.end();++It)
// It->second->apply(Doc,const_cast<DocumentObject*>(It->first));
for (It= _Objects.begin();It!=_Objects.end();++It)
It->second->applyDel(Doc,const_cast<DocumentObject*>(It->first));
for (It= _Objects.begin();It!=_Objects.end();++It)
It->second->applyNew(Doc,const_cast<DocumentObject*>(It->first));
for (It= _Objects.begin();It!=_Objects.end();++It)
It->second->applyChn(Doc,const_cast<DocumentObject*>(It->first),forward);
}
Code: Select all
0 ?? /lib/i386-linux-gnu/libc.so.6 0 0x224e6b6
1 std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) /usr/lib/i386-linux-gnu/libstdc++.so.6 0 0x2159b9b
2 Gui::DocumentItem::slotChangeObject Tree.cpp 838 0x5c67f0
...
omitted a lot of boost:: stuff
.....
29 boost::signal2<void, App::DocumentObject const&, App::Property const&, boost::last_value<void>, int, std::less<int>, boost::function<void (App::DocumentObject const&, App::Property const&)> >::operator()(App::DocumentObject const&, App::Property const&) signal_template.hpp 354 0xcff39b
30 App::Document::onChangedProperty Document.cpp 450 0xcf2f23
31 App::DocumentObject::onChanged DocumentObject.cpp 187 0xd35a40
32 Part::Feature::onChanged PartFeature.cpp 139 0xb30b5dd8
33 App::Property::hasSetValue Property.cpp 94 0xd61763
34 App::PropertyLink::Paste PropertyLinks.cpp 162 0xd6eace
35 App::TransactionObject::applyChn Transactions.cpp 240 0xd58b82
36 App::Transaction::apply Transactions.cpp 120 0xd58313
37 App::Document::abortTransaction Document.cpp 347 0xcf2bf2
38 Gui::Document::abortCommand Document.cpp 1149 0x49ca7b
39 Gui::Command::abortCommand Command.cpp 395 0x4e011c
40 PartDesignGui::TaskDlgPadParameters::reject TaskPadParameters.cpp 512 0xa3c70ef4
Code: Select all
std::map<std::string, DocumentObjectItem*>::iterator it = ObjectMap.find(objectName);
if (it != ObjectMap.end()) {
// use new grouping style
std::set<QTreeWidgetItem*> children;
std::vector<App::DocumentObject*> group = view.claimChildren(); // <============ This accesses the Model property with the invalid pointer to Pad
for (std::vector<App::DocumentObject*>::iterator jt = group.begin(); jt != group.end(); ++jt) {
if(*jt){
const char* internalName = (*jt)->getNameInDocument();
if (internalName) {
std::map<std::string, DocumentObjectItem*>::iterator kt = ObjectMap.find(internalName); // <=========== Here it crashes when accessing the invalid pointer
Re: Problem with Body and abortCommand()
Ok,
in that case we have the problem that two proerties are changed, Tip and Model. There fore the tree get signaled twice for object changed and the first time the Model property is unchanged.
The savest way (though bit slow) is to check on each pointer returned from claimChildren() if its still in the document. That prevent the crash securely.
I would do a isIn() method in the Document which check if the pointer is in the document.
In similar cases some time ago we also discussed preventing signaling as long as a undo/redo runs, buts that would lead to extensive clean ups afterward. Also I'm not sure it would disrupt to many execution paths which relay on the signaling....
So I would go for pointer checking in tree.cpp.
in that case we have the problem that two proerties are changed, Tip and Model. There fore the tree get signaled twice for object changed and the first time the Model property is unchanged.
The savest way (though bit slow) is to check on each pointer returned from claimChildren() if its still in the document. That prevent the crash securely.
I would do a isIn() method in the Document which check if the pointer is in the document.
In similar cases some time ago we also discussed preventing signaling as long as a undo/redo runs, buts that would lead to extensive clean ups afterward. Also I'm not sure it would disrupt to many execution paths which relay on the signaling....
So I would go for pointer checking in tree.cpp.
Stop whining - start coding!
-
- Posts: 554
- Joined: Sat Apr 07, 2012 2:42 am
Re: Problem with Body and abortCommand()
Putting the isIn() check in slotChangeObject() prevents this crash - but now there is another one.
After creating a Pad, canceling the dialog, when I try to again create a Pad with the same sketch, FC crashes when calling updateActive() in Command.cpp. The point it crashes is in Document::recompute():
Adding another isIn() call solves the problem. But it does look as though something was not cleanly removed from the Document.
After creating a Pad, canceling the dialog, when I try to again create a Pad with the same sketch, FC crashes when calling updateActive() in Command.cpp. The point it crashes is in Document::recompute():
Code: Select all
for (std::list<Vertex>::reverse_iterator i = make_order.rbegin();i != make_order.rend(); ++i) {
DocumentObject* Cur = d->vertexMap[*i];
if (!Cur) continue;
#ifdef FC_LOGFEATUREUPDATE
std::clog << Cur->getNameInDocument() << " dep on: " ;
#endif
Code: Select all
if (!Cur || !isIn(Cur)) continue;
Re: Problem with Body and abortCommand()
Mhh, this bug showed up not long ago.
When recomputing the document a graph data structure is build up to determine if we have a DAG and get the recompute order.
Former we build up that DAG right before re-computation.
With the Assembly branch I made it more persistent. In the assembly we surly need that information of the DAG very often, to determine inLists or the path of a object right from the top.
There fore the DAG get not deleted. But we have to invalidate it if there are changes, so it can be recomputed the next time needed. Seams the invalidation (deletion) is not working in case of Undo....
When recomputing the document a graph data structure is build up to determine if we have a DAG and get the recompute order.
Former we build up that DAG right before re-computation.
With the Assembly branch I made it more persistent. In the assembly we surly need that information of the DAG very often, to determine inLists or the path of a object right from the top.
There fore the DAG get not deleted. But we have to invalidate it if there are changes, so it can be recomputed the next time needed. Seams the invalidation (deletion) is not working in case of Undo....
Stop whining - start coding!