View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0031075Open CASCADEOCCT:Application Frameworkpublic2019-10-17 10:532020-12-02 17:12
Reporterkgv Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version7.0.0 
Target Version7.5.0Fixed in Version7.5.0 
Summary0031075: Application Framework - reading STEP file into TDocStd_Document leads to memory leaks
DescriptionReading STEP file into TDocStd_Document leads to memory leaks in application using tools in straight-forward way.

The simple code creating TDocStd_Document and filling it with STEPCAFControl_Reader results in memory allocated somewhere and not released automatically on releasing the Handle.

Based on experience of several projects, clean up procedure looks unexpected and unclear:
- Working session holds memory in global structures,
  so that application needs calling special methods of XSControl_WorkSession directly.
- TDocStd_Document automatic destruction doesn't release memory.
  Special tricks like ForgetAllAttributes() should be used to actually release (majority) of allocated memory.

static Standard_Integer xtest(Draw_Interpretor& di, Standard_Integer argc, const char** argv)
{
  static int k = 0;
  //for (int k = 0; k < 100; k++)
  {
    STEPCAFControl_Reader aReader;
    if (aReader.ReadFile("test.stp") != IFSelect_RetDone)
    {
      std::cout << "FAILURE\n";
      return 1;
    }

    //Handle(TDocStd_Application) myXdeApp = XCAFApp_Application::GetApplication();

    TCollection_ExtendedString aFormat; // "BinXCAF"
    Handle(TDocStd_Document) aXCAF = new TDocStd_Document (aFormat);
    //myXdeApp->NewDocument (aFormat, aXCAF);
    aReader.Transfer(aXCAF);

    Handle(XSControl_WorkSession) aWS = aReader.Reader().WS();
    if (Handle(XSControl_TransferReader) aTransferReader = aWS->TransferReader())
    {
      if (Handle(Transfer_TransientProcess) aMapReader = aTransferReader->TransientProcess())
      {
        aMapReader->Clear();
      }
      aTransferReader->Clear (-1);
    }

    if (!aXCAF.IsNull())
    {
      if (aXCAF->HasOpenCommand())
      {
        aXCAF->AbortCommand();
      }

      aXCAF->Main().Root().ForgetAllAttributes (Standard_True);
      //myXdeApp->Close (aXCAF);
      aXCAF.Nullify();
    }
  }
  std::cout << "  ==== Iteration #" << k << "\n" << OSD_MemInfo::PrintInfo() << "\n";
  ++k;
  return 0;
}
Steps To Reproducebugs caf bug31075
TagsNo tags attached.
Test case numberbugs/caf/bug31075

Relationships

parent of 0031681 closedbugmaster Community Foundation Classes - Shared Libraries Cannot Be Loaded 
related to 0030563 closedabv Community Coding - Standard_Type vptr error 

Activities

mpv

2020-04-22 10:34

developer   ~0091711

This is because in the data tree of the document there is a back-reference to the document is stored in the attribute TDataStd_Owner (located on the root label).

It keeps a document handle in a field because otherwise (if just a pointer is stored) it should be dangerous: it returns a document handle, but if user don't create handle of the document, it will destruct the document on the handle release.

So, the document handle is never released and document destructor is never called. As a consequence, there must be some explicit call that removes this attribute of removes the reference from the document. As it is done in the TDocStd_Application::Close

  Handle(TDocStd_Owner) Owner;
  if (aDoc->Main().Root().FindAttribute(TDocStd_Owner::GetID(),Owner)) {
    Handle(TDocStd_Document) emptyDoc;
    Owner->SetDocument(emptyDoc);
  }

Or as it is done in your example:

  aXCAF->Main().Root().ForgetAllAttributes (Standard_True);

Actually, there may be less attributes removed:

  aXCAF->Main().Root().ForgetAllAttributes (Standard_False);
or
  aXCAF->Main().Root().ForgetAttribute (TDocStd_Owner::GetID());


So, I propose to add a comment in TDocStd_Document constructor: if it is created outside of application, in the end of the lifecycle this method must be called to clear data. Or perhaps, to add a method in the document that must be called in the end, named "Clean" or so.

KGV, what do you think?

abv

2020-04-22 10:54

manager   ~0091713

Mikhail, we all know why and how this problem occurs, the question is to improve our classes so that users *do not need to know* about this implementation detail, i.e. have document destroyed in normal intuitive usage scenario. One clear possibility is to store plain pointer instead of handle to document in the Owner attribute. Do you think it is impossible for some reasons?

mpv

2020-04-22 11:16

developer   ~0091714

I've already answered about the plain pointer storage:

  It keeps a document handle in a field because otherwise (if just a pointer is stored) it should be dangerous: it returns a document handle, but if user don't create handle of the document, it will destruct the document on the handle release.

So, if we keep a pointer, it should return a pointer and every caller can not create a handle, which is a bad case, I guess.

mpv

2020-04-22 14:36

developer   ~0091715

One of the solution of having pointer in Owner attribute may be to prevent user from creation of TDocStd_Document using constructor, but provide a static method for creation, which returns the Handle to it. This will cause at least some problems if he don't plan to use Handle for keeping the document and makes crash earlier if he does this anyway (when the handle is released - after the static creation-method call or on exit from the method).

However, creation of document outside of the application is not very often and suggested approach, so, it may be reasonable.

Anyway this will change API and require some porting-actions, documentation update, etc. What do you think? Is it worth to fix this problem in that way?

abv

2020-04-28 22:34

manager   ~0091829

Mikhail, can you please give example of use case reproducing potentially dangerous situation that you describe? In my understanding, when you create TDocStd_Document, you will keep handle to it, and as soon as it goes out of scope, it shall be destroyed (together with "owner" and all other stuff inside). This is clearly the expected behavior in OOP and C++, which is not provided by existing broken implementation.

mpv

2020-04-29 10:21

developer   ~0091838

TDocStd_Document* aXCAF = new TDocStd_Document ("BinXCAF");
myMainLab = aXCAF->Main();
TDataStd_Integer::Set(myMainLab, 5);
Handle(TDocStd_Owner) Owner;
// ...

// ...
myMainLab.Root().FindAttribute(TDocStd_Owner::GetID(),Owner);
// if GetDocument returns Handle: it creates a handle and releases it immediately, handle count=0, destroy document
Owner->GetDocument();
// if GetDocument returns pointer: it creates a handle and releases it somewhere (like on method exit)
Handle(TDocStd_Document) aXCAF3 = Owner->GetDocument();
aXCAF3.Nullify();
// ...

// ...

// crash
TDataStd_Integer::Set(myMainLab, 6);

kgv

2020-04-29 10:35

developer   ~0091840

Last edited: 2020-04-29 10:36

 > TDocStd_Document* aXCAF = new TDocStd_Document ("BinXCAF");
> // if GetDocument returns Handle: it creates a handle and releases it immediately, handle count=0, destroy document

OCCT has interfaces which implicitly require objects to be created by Handle - because other object receive arguments taking Handles. From my point of view, it is better allowing code misusing API to crash rather than to keep it in memory indefinitely.

abv

2020-04-29 12:41

manager   ~0091854

Mikhail, can you explain the logic of your example when you do not create handle when you create document but do use it when you get pointer to it?

Well, even if this broken logic is applied, in your example the programmer will get the document destroyed unexpectedly, which will very likely show up very soon and is easy to debug. Current implementation leads to hidden and uncontrollable memory leaks which are very hard to identify and fix.

mpv

2020-04-29 13:04

developer   ~0091856

I don't want to make a logic example, I just want to say that this may cause crash in some application, even the currently working. Applications are different, so, we can not predict all architectures.

Like this:

TDocStd_Document* aXCAF = new TDocStd_Document("BinXCAF");
myMainLab = aXCAF->Main();

// then forget the document and keep labels and attributes only

// In some method the document is needed again:
myMainLab.Root().FindAttribute(TDocStd_Owner::GetID(),Owner);
Owner->GetDocument()->NewCommand();

--> crash on the next reference to document

For me such use-case is possible.

So, one of my proposal to protect constructor of TDocStd_Document and provide a static method to return Handle seems reasonable. In case Owner don't contain Handle, this seems the only way to have crash as soon as possible when the last Handle to the document is released, but referencing to the document content is still happened.

abv

2020-05-07 09:43

manager   ~0091972

Mikhail, note that if in your example you create the document on stack, you will get a crash on exit from the scope.

We do not need to support any possible logic and use cases, we need first of all to support the approach adopted within our own library, i.e. OCCT. Here we do use Handles to provide consistent work with dynamically allocated data, and this is essential.

This implies that

- You shall use Handle to store pointers to dynamically allocated objects inheriting Standard_Transient (and in general, shall never create such objects on stack)

- When the (last) Handle is destroyed, the object is destroyed too.

Current implementation of TDocStd_Document violates these basic assumptions, so it must be corrected.

Indeed I have nothing against providing static method Create to ensure that the document is created always dynamically and manipulated by Handle.

git

2020-05-20 20:14

administrator   ~0092272

Branch CR31075 has been created by mpv.

SHA-1: 73f9b6673bcf3533ef2b2652ab2d1d465204c98a


Detailed log of new commits:

Author: mpv
Date: Wed May 20 20:15:56 2020 +0300

    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.
    
    Added test bugs caf bug31075

git

2020-05-21 10:38

administrator   ~0092285

Branch CR31075 has been updated by mpv.

SHA-1: e090f87cec8c69de7d394620bda8f128553c23a9


Detailed log of new commits:

Author: mpv
Date: Thu May 21 10:39:07 2020 +0300

    # correct the test due to the fact that Owner does not add a ref count now

git

2020-05-22 16:37

administrator   ~0092304

Branch CR31075 has been updated by mpv.

SHA-1: ceadb458a032213cf2f9229230dcf4c3d237f0f7


Detailed log of new commits:

Author: mpv
Date: Fri May 22 16:39:14 2020 +0300

    # an additional fix for exiting from an application when a transaction is still opened: it references to the Standard_Type when theResitry map is already destroyed

git

2020-05-25 20:58

administrator   ~0092340

Branch CR31075_1 has been created by mpv.

SHA-1: 23e68e6897246255de315cec80cb683caad453f5


Detailed log of new commits:

Author: mpv
Date: Mon May 25 20:59:33 2020 +0300

    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
    Added a new test bugs caf bug31075

git

2020-05-27 11:02

administrator   ~0092356

Branch CR31075_1 has been updated by mpv.

SHA-1: d0a83e9d2b44f64a69156b57f6a8ed3104e78bd4


Detailed log of new commits:

Author: mpv
Date: Wed May 27 11:03:41 2020 +0300

    # Fix for the unit tests bugs caf bug21231 and bugs xde bug22776 : referencing the Document and Data from TObj when Data is destroyed

git

2020-05-27 16:10

administrator   ~0092360

Branch CR31075_1 has been updated by mpv.

SHA-1: ebbe176ca3fbb682293ab4a46ad390e46bbbe7bf


Detailed log of new commits:

Author: mpv
Date: Wed May 27 16:11:36 2020 +0300

    # Fix for the unit tests bugs caf bug21231 and bugs xde bug22776 : referencing the Document and Data from TObj when Data is destroyed

git

2020-05-28 11:26

administrator   ~0092381

Branch CR31075_1 has been updated by mpv.

SHA-1: d4ea01c66a9a4f41972a91f0dc50a15863835ba8


Detailed log of new commits:

Author: mpv
Date: Thu May 28 11:27:49 2020 +0300

    # Fix for the unit test bugs xde bug30779: ForgetAll causes Backup, that creates the temporary Handle(TDF_Data) and calls destructor the second time

git

2020-06-16 13:33

administrator   ~0092555

Branch CR31075_1 has been updated by mpv.

SHA-1: 804ede7e5ebf5517bf2d53e4a1003a06193f7f0e


Detailed log of new commits:

Author: mpv
Date: Tue Jun 16 13:35:23 2020 +0300

    # fix crash of the bug22776 caused by incorrect memory management on close of the document

git

2020-06-18 10:22

administrator   ~0092567

Branch CR31075_2 has been created by mpv.

SHA-1: 3ffdb3fda4ae189e9398a1f0c3c96ca8896575f3


Detailed log of new commits:

Author: mpv
Date: Thu Jun 18 10:24:07 2020 +0300

    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

mpv

2020-06-18 15:24

developer   ~0092573

Branch CR31075_2 of OCCT and PRODUCTS contains fix of this issue: TDocStd_Owner contains just a pointer to document and methods for setting pointers (in case handle can not be created). Also, several places are corrected to avoid crash of application in case some document was not closed before exit from the application.
Unit-tests "bugs xde bug22776" in OCCT and "dxf read bug22396" in PRODUCTS were updated to close the document before exit, because otherwise there appeared a crash in destruction of visualization objects (destroyed on a library unload and after on visual attributes in document destruction).

An new unit-test "bugs caf bug31075" is added.

All tests are passed:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR31075_2-CR31075_2-MPV-OCCT-Debian80-64-opt-test-compare/1/
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR31075_2-CR31075_2-MPV-OCCT-Windows-64-VC14-opt-test-compare/1/
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR31075_2-CR31075_2-MPV-Products-Debian80-64-opt-test-compare/1/
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR31075_2-CR31075_2-MPV-Products-Windows-64-VC14-opt-test-compare/1/

szy

2020-06-19 11:32

manager   ~0092575

Mikhail,
There is another way how to fix the issue. From my point of view - more simple.
TDocStd_Document has the method BeforeClose().
You can simply add to Owner method ReleaseDocument() nullifying myDocument variable.
And add a line calling this method to TDocStd_Document::BeforeClose().
Consider this approach, please.

mpv

2020-06-19 11:43

developer   ~0092576

The problem is that they don't want to call "Close" (because they did not open it), so method "BeforeClose" will be never called. We may use only destructor to free the data structure, but destructor is called only when all Handles are nullified, which is not the case if Owner contains Handle to the document as a field.

szy

2020-06-19 12:19

manager   ~0092577

Reviewed.

bugmaster

2020-06-20 13:34

administrator   ~0092599

Combination -
OCCT branch : IR-2020-06-19
master SHA - ef779ae0da0ded44c5045762768fe5f5eaff6b04
a206de37fbfa0bf71bd534ae47192bbec23b8522
Products branch : IR-2020-06-19 SHA - 9e1feabf4ca514e1647ce6acf381182c6ac85c9c
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17159.860000000117 / 17161.410000000207 [-0.01%]
Products
Total CPU difference: 11197.400000000112 / 11196.290000000083 [+0.01%]
Windows-64-VC14:
OCCT
Total CPU difference: 18670.90625 / 18756.21875 [-0.45%]
Products
Total CPU difference: 13012.71875 / 13044.46875 [-0.24%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2020-06-20 13:42

administrator   ~0092604

Branch CR31075_2 has been deleted by inv.

SHA-1: 3ffdb3fda4ae189e9398a1f0c3c96ca8896575f3

git

2020-06-20 13:42

administrator   ~0092606

Branch CR31075_1 has been deleted by inv.

SHA-1: 804ede7e5ebf5517bf2d53e4a1003a06193f7f0e

git

2020-06-20 13:42

administrator   ~0092615

Branch CR31075 has been deleted by inv.

SHA-1: ceadb458a032213cf2f9229230dcf4c3d237f0f7

Related Changesets

occt: master ef779ae0

2020-06-18 07:24:07

mpv


Committer: bugmaster Details Diff		 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		 Affected Issues
0031075
mod - src/QABugs/QABugs_1.cxx Diff File
mod - src/Standard/Standard_Type.cxx Diff File
mod - src/TDF/TDF_Data.cxx Diff File
mod - src/TDocStd/TDocStd_Document.cxx Diff File
mod - src/TDocStd/TDocStd_Document.hxx Diff File
mod - src/TDocStd/TDocStd_Owner.cxx Diff File
mod - src/TDocStd/TDocStd_Owner.hxx Diff File
mod - src/TPrsStd/TPrsStd_DriverTable.cxx Diff File
add - tests/bugs/caf/bug31075 Diff File
mod - tests/bugs/xde/bug22776 Diff File

Issue History

Date Modified Username Field Change
2019-10-17 10:53 kgv New Issue
2019-10-17 10:53 kgv Assigned To => mpv
2019-10-17 10:54 kgv Product Version => 7.0.0
2020-04-22 10:34 mpv Note Added: 0091711
2020-04-22 10:34 mpv Assigned To mpv => kgv
2020-04-22 10:34 mpv Status new => feedback
2020-04-22 10:54 abv Note Added: 0091713
2020-04-22 10:55 abv Assigned To kgv => mpv
2020-04-22 11:16 mpv Note Added: 0091714
2020-04-22 14:36 mpv Note Added: 0091715
2020-04-22 14:36 mpv Assigned To mpv => abv
2020-04-28 22:34 abv Note Added: 0091829
2020-04-28 22:55 abv Assigned To abv => mpv
2020-04-29 10:21 mpv Note Added: 0091838
2020-04-29 10:21 mpv Assigned To mpv => abv
2020-04-29 10:35 kgv Note Added: 0091840
2020-04-29 10:36 kgv Note Edited: 0091840
2020-04-29 12:41 abv Note Added: 0091854
2020-04-29 12:42 abv Assigned To abv => mpv
2020-04-29 13:04 mpv Note Added: 0091856
2020-05-06 12:12 mpv Assigned To mpv => abv
2020-05-07 09:43 abv Note Added: 0091972
2020-05-07 09:43 abv Assigned To abv => mpv
2020-05-20 20:14 git Note Added: 0092272
2020-05-21 10:38 git Note Added: 0092285
2020-05-22 16:37 git Note Added: 0092304
2020-05-25 20:58 git Note Added: 0092340
2020-05-27 11:02 git Note Added: 0092356
2020-05-27 16:10 git Note Added: 0092360
2020-05-28 11:26 git Note Added: 0092381
2020-06-16 13:33 git Note Added: 0092555
2020-06-18 10:22 git Note Added: 0092567
2020-06-18 15:24 mpv Note Added: 0092573
2020-06-18 15:24 mpv Assigned To mpv => szy
2020-06-18 15:24 mpv Status feedback => resolved
2020-06-18 15:24 mpv Steps to Reproduce Updated
2020-06-19 11:32 szy Note Added: 0092575
2020-06-19 11:32 szy Assigned To szy => mpv
2020-06-19 11:32 szy Status resolved => feedback
2020-06-19 11:43 mpv Note Added: 0092576
2020-06-19 11:43 mpv Assigned To mpv => szy
2020-06-19 11:43 mpv Status feedback => resolved
2020-06-19 12:19 szy Note Added: 0092577
2020-06-19 12:19 szy Assigned To szy => bugmaster
2020-06-19 12:19 szy Status resolved => reviewed
2020-06-20 13:34 bugmaster Note Added: 0092599
2020-06-20 13:34 bugmaster Status reviewed => tested
2020-06-20 13:35 bugmaster Test case number => bugs/caf/bug31075
2020-06-20 13:36 bugmaster Changeset attached => occt master ef779ae0
2020-06-20 13:36 bugmaster Status tested => verified
2020-06-20 13:36 bugmaster Resolution open => fixed
2020-06-20 13:42 git Note Added: 0092604
2020-06-20 13:42 git Note Added: 0092606
2020-06-20 13:42 git Note Added: 0092615
2020-07-21 18:49 BenjaminBihler Relationship added parent of 0031681
2020-08-28 14:46 kgv Relationship added related to 0030563
2020-12-02 16:40 emo Fixed in Version => 7.5.0
2020-12-02 17:12 emo Status verified => closed