From ef779ae0da0ded44c5045762768fe5f5eaff6b04 Mon Sep 17 00:00:00 2001 From: mpv Date: Thu, 18 Jun 2020 10:24:07 +0300 Subject: [PATCH] 0031075: Application Framework - reading STEP file into TDocStd_Document leads to memory leaks In the TDocStd_Owner keep simple pointer to TDocStd_Document instead of Handle. This causes automatic destruction of the document without explicit call of Close. In Standard_Type added a static variable theType that initializes theRegistry map earlier. Otherwise exit from Draw interpreter crashes in many test-cases because not-closed transactions are aborted on document handle release from Draw theVariables map. Corrected method for test OCC159bug due to the fact that Owner does not add a ref count now Close the document in the end of bugs xde bug22776 otherwise double remove of visualization objects (on library exit and on visualization attributes remove from the document) causes crash on exit from draw Added a new test bugs caf bug31075 --- src/QABugs/QABugs_1.cxx | 1 + src/Standard/Standard_Type.cxx | 3 ++ src/TDF/TDF_Data.cxx | 8 +++++ src/TDocStd/TDocStd_Document.cxx | 6 +++- src/TDocStd/TDocStd_Document.hxx | 3 ++ src/TDocStd/TDocStd_Owner.cxx | 51 ++++++++++++++++++++++------- src/TDocStd/TDocStd_Owner.hxx | 12 ++++--- src/TPrsStd/TPrsStd_DriverTable.cxx | 2 ++ tests/bugs/caf/bug31075 | 17 ++++++++++ tests/bugs/xde/bug22776 | 2 ++ 10 files changed, 89 insertions(+), 16 deletions(-) create mode 100644 tests/bugs/caf/bug31075 diff --git a/src/QABugs/QABugs_1.cxx b/src/QABugs/QABugs_1.cxx index bce0b3b3f9..244aeb440b 100644 --- a/src/QABugs/QABugs_1.cxx +++ b/src/QABugs/QABugs_1.cxx @@ -86,6 +86,7 @@ static Standard_Integer OCC159bug (Draw_Interpretor& di, Standard_Integer argc, } else { di << "DocOwner1 = NOTNULL\n"; } + OwnerD1.Nullify(); Handle(TDocStd_Application) A = DDocStd::GetApplication(); A->Close(D); diff --git a/src/Standard/Standard_Type.cxx b/src/Standard/Standard_Type.cxx index 9516915110..b18c8b049a 100644 --- a/src/Standard/Standard_Type.cxx +++ b/src/Standard/Standard_Type.cxx @@ -98,6 +98,9 @@ namespace { static registry_type theRegistry; return theRegistry; } + + // To initialize theRegistry map as soon as possible to be destoryed the latest + Handle(Standard_Type) theType = STANDARD_TYPE(Standard_Transient); } Standard_Type* Standard_Type::Register (const char* theSystemName, const char* theName, diff --git a/src/TDF/TDF_Data.cxx b/src/TDF/TDF_Data.cxx index 44ba6da880..44e103c456 100644 --- a/src/TDF/TDF_Data.cxx +++ b/src/TDF/TDF_Data.cxx @@ -110,6 +110,14 @@ myAllowModification (Standard_True) void TDF_Data::Destroy() { AbortUntilTransaction(1); + // Forget the Owner attribute from the root label to avoid referencing document before + // desctuction of the framework (on custom attributes forget). Don't call ForgetAll because + // it may call backup. + while(!myRoot->FirstAttribute().IsNull()) { + static Handle(TDF_Attribute) anEmpty; + Handle(TDF_Attribute) aFirst = myRoot->FirstAttribute(); + myRoot->RemoveAttribute(anEmpty, aFirst); + } myRoot->Destroy (myLabelNodeAllocator); myRoot = NULL; } diff --git a/src/TDocStd/TDocStd_Document.cxx b/src/TDocStd/TDocStd_Document.cxx index f79a5e45e1..e72d5cacb0 100644 --- a/src/TDocStd/TDocStd_Document.cxx +++ b/src/TDocStd/TDocStd_Document.cxx @@ -63,7 +63,11 @@ theList.Remove(it); Handle(TDocStd_Document) TDocStd_Document::Get (const TDF_Label& acces) { - return TDocStd_Owner::GetDocument(acces.Data()); + // avoid creation of Handle(TDF_Data) during TDF_Data destruction + if (acces.Root().HasAttribute()) { + return TDocStd_Owner::GetDocument(acces.Data()); + } + return Handle(TDocStd_Document)(); } //======================================================================= diff --git a/src/TDocStd/TDocStd_Document.hxx b/src/TDocStd/TDocStd_Document.hxx index 0d07b103d1..2454a65252 100644 --- a/src/TDocStd/TDocStd_Document.hxx +++ b/src/TDocStd/TDocStd_Document.hxx @@ -63,6 +63,9 @@ public: //! Constructs a document object defined by the //! string astorageformat. + //! If a document is created outside of an application using this constructor, it must be + //! managed by a Handle. Otherwise memory problems could appear: call of TDocStd_Owner::GetDocument + //! creates a Handle(TDocStd_Document), so, releasing it will produce a crash. Standard_EXPORT TDocStd_Document(const TCollection_ExtendedString& astorageformat); //! the document is saved in a file. diff --git a/src/TDocStd/TDocStd_Owner.cxx b/src/TDocStd/TDocStd_Owner.cxx index aeba4b2b18..9c4e33b7b2 100644 --- a/src/TDocStd/TDocStd_Owner.cxx +++ b/src/TDocStd/TDocStd_Owner.cxx @@ -43,12 +43,31 @@ const Standard_GUID& TDocStd_Owner::GetID() //======================================================================= void TDocStd_Owner::SetDocument (const Handle(TDF_Data)& indata, - const Handle(TDocStd_Document)& D) + const Handle(TDocStd_Document)& doc) { Handle(TDocStd_Owner) A; if (!indata->Root().FindAttribute (TDocStd_Owner::GetID(), A)) { A = new TDocStd_Owner (); - A->SetDocument(D); + A->SetDocument(doc); + indata->Root().AddAttribute(A); + } + else { + throw Standard_DomainError("TDocStd_Owner::SetDocument : already called"); + } +} + +//======================================================================= +//function : SetDocument +//purpose : +//======================================================================= + +void TDocStd_Owner::SetDocument (const Handle(TDF_Data)& indata, + TDocStd_Document* doc) +{ + Handle(TDocStd_Owner) A; + if (!indata->Root().FindAttribute (TDocStd_Owner::GetID(), A)) { + A = new TDocStd_Owner (); + A->SetDocument(doc); indata->Root().AddAttribute(A); } else { @@ -75,7 +94,7 @@ Handle(TDocStd_Document) TDocStd_Owner::GetDocument (const Handle(TDF_Data)& ofd //purpose : //======================================================================= -TDocStd_Owner::TDocStd_Owner () { } +TDocStd_Owner::TDocStd_Owner() { } //======================================================================= @@ -83,19 +102,29 @@ TDocStd_Owner::TDocStd_Owner () { } //purpose : //======================================================================= -void TDocStd_Owner::SetDocument(const Handle( TDocStd_Document)& D) +void TDocStd_Owner::SetDocument (const Handle( TDocStd_Document)& document) { - myDocument = D; + myDocument = document.get(); } +//======================================================================= +//function : SetDocument +//purpose : +//======================================================================= + +void TDocStd_Owner::SetDocument (TDocStd_Document* document) +{ + myDocument = document; +} //======================================================================= //function : Get //purpose : //======================================================================= -Handle(TDocStd_Document) TDocStd_Owner::GetDocument () const -{ return myDocument; +Handle(TDocStd_Document) TDocStd_Owner::GetDocument() const +{ + return Handle(TDocStd_Document)(myDocument); } @@ -104,7 +133,7 @@ Handle(TDocStd_Document) TDocStd_Owner::GetDocument () const //purpose : //======================================================================= -const Standard_GUID& TDocStd_Owner::ID () const { return GetID(); } +const Standard_GUID& TDocStd_Owner::ID() const { return GetID(); } //======================================================================= @@ -112,7 +141,7 @@ const Standard_GUID& TDocStd_Owner::ID () const { return GetID(); } //purpose : //======================================================================= -Handle(TDF_Attribute) TDocStd_Owner::NewEmpty () const +Handle(TDF_Attribute) TDocStd_Owner::NewEmpty() const { Handle(TDF_Attribute) dummy; return dummy; @@ -123,7 +152,7 @@ Handle(TDF_Attribute) TDocStd_Owner::NewEmpty () const //purpose : //======================================================================= -void TDocStd_Owner::Restore(const Handle(TDF_Attribute)&) +void TDocStd_Owner::Restore (const Handle(TDF_Attribute)&) { } @@ -156,6 +185,6 @@ void TDocStd_Owner::DumpJson (Standard_OStream& theOStream, Standard_Integer the { OCCT_DUMP_TRANSIENT_CLASS_BEGIN (theOStream) - OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, myDocument.get()) + OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, myDocument) } diff --git a/src/TDocStd/TDocStd_Owner.hxx b/src/TDocStd/TDocStd_Owner.hxx index 69e5b0534f..3b36f82380 100644 --- a/src/TDocStd/TDocStd_Owner.hxx +++ b/src/TDocStd/TDocStd_Owner.hxx @@ -47,7 +47,9 @@ public: Standard_EXPORT static const Standard_GUID& GetID(); Standard_EXPORT static void SetDocument (const Handle(TDF_Data)& indata, const Handle(TDocStd_Document)& doc); - + + Standard_EXPORT static void SetDocument (const Handle(TDF_Data)& indata, TDocStd_Document* doc); + //! Owner methods //! =============== Standard_EXPORT static Handle(TDocStd_Document) GetDocument (const Handle(TDF_Data)& ofdata); @@ -55,7 +57,9 @@ public: Standard_EXPORT TDocStd_Owner(); Standard_EXPORT void SetDocument (const Handle(TDocStd_Document)& document); - + + Standard_EXPORT void SetDocument (TDocStd_Document* document); + Standard_EXPORT Handle(TDocStd_Document) GetDocument() const; Standard_EXPORT const Standard_GUID& ID() const Standard_OVERRIDE; @@ -83,8 +87,8 @@ protected: private: - - Handle(TDocStd_Document) myDocument; + //! It keeps pointer to the document to avoid handles cyclic dependency + TDocStd_Document* myDocument; }; diff --git a/src/TPrsStd/TPrsStd_DriverTable.cxx b/src/TPrsStd/TPrsStd_DriverTable.cxx index 32771cde24..857ccc9f74 100644 --- a/src/TPrsStd/TPrsStd_DriverTable.cxx +++ b/src/TPrsStd/TPrsStd_DriverTable.cxx @@ -47,6 +47,8 @@ Handle(TPrsStd_DriverTable) TPrsStd_DriverTable::Get() if ( drivertable.IsNull() ) { drivertable = new TPrsStd_DriverTable; + // it must be never destroyed, even this library is unloaded + new Handle(TPrsStd_DriverTable)(drivertable); #ifdef OCCT_DEBUG std::cout << "The new TPrsStd_DriverTable was created" << std::endl; #endif diff --git a/tests/bugs/caf/bug31075 b/tests/bugs/caf/bug31075 new file mode 100644 index 0000000000..e453f0dcb7 --- /dev/null +++ b/tests/bugs/caf/bug31075 @@ -0,0 +1,17 @@ +puts "============" +puts "0031075: Application Framework - reading STEP file into TDocStd_Document leads to memory leaks" +puts "============" + +pload XDE + +puts "Create a new document outside of application, add a shape an release all variables in cycle to see if allocated heap memory grows" +set listmem {} +for {set i 1} {$i < 100} {incr i} { + NewDocument D + box aBox 10 10 10 + XAddShape D aBox + unset aBox + unset D + lappend listmem [meminfo h] + checktrend $listmem 0 0 "Memory leak" +} diff --git a/tests/bugs/xde/bug22776 b/tests/bugs/xde/bug22776 index 9edb189237..aeb15262dc 100755 --- a/tests/bugs/xde/bug22776 +++ b/tests/bugs/xde/bug22776 @@ -122,3 +122,5 @@ if { ${status} == 0} { } checkview -display result -3d -path ${imagedir}/${test_image}.png + +Close D