Problem with Body and abortCommand()

[jrheinlaender]

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()?

[jriegel]

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....

[jrheinlaender]

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

[jriegel]

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!

[jrheinlaender]

the Undo should do it exactly the other way around as its created

But it seems that all deletions are done first, then creation of new objects, then adjustment of properties:

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);
}

Here is a stack trace of the crash when the Model property is reset:

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	

This is the line where the crash occurs in Tree.cpp:

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

[jriegel]

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.

[jrheinlaender]

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():

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

Adding another isIn() call solves the problem. But it does look as though something was not cleanly removed from the Document.

Code: Select all

if (!Cur || !isIn(Cur)) continue;

[jriegel]

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....