Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0031075Open CASCADE[OCCT] OCCT:Application Frameworkpublic2019-10-17 10:532020-05-22 16:37
Assigned Tompv 
PlatformOSOS Version
Product Version[OCCT] 7.0.0 
Target Version[OCCT] 7.5.0*Fixed in Version 
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);

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

    if (!aXCAF.IsNull())
      if (aXCAF->HasOpenCommand())

      aXCAF->Main().Root().ForgetAllAttributes (Standard_True);
      //myXdeApp->Close (aXCAF);
  std::cout << "  ==== Iteration #" << k << "\n" << OSD_MemInfo::PrintInfo() 
<< "\n";
  return 0;
TagsNo tags attached.
Test case number
Attached Files

- Relationships

-  Notes
mpv (developer)
2020-04-22 10:34

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;

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);
  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 (manager)
2020-04-22 10:54

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 (developer)
2020-04-22 11:16

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 (developer)
2020-04-22 14:36

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 (manager)
2020-04-28 22:34

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 (developer)
2020-04-29 10:21

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

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

// ...

// crash
TDataStd_Integer::Set(myMainLab, 6);
kgv (developer)
2020-04-29 10:35
edited on: 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 (manager)
2020-04-29 12:41

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 (developer)
2020-04-29 13:04

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:

--> 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 (manager)
2020-05-07 09:43

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 (administrator)
2020-05-20 20:14

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 (administrator)
2020-05-21 10:38

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 (administrator)
2020-05-22 16:37

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

- 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 Relationship added related to 0031072
2019-10-17 10:54 kgv Product Version => 7.0.0
2019-10-17 11:02 kgv Relationship added related to 0029269
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 View Revisions
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

Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker