MantisBT
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-28 11:26
Reporterkgv 
Assigned Tompv 
PrioritynormalSeverityminor 
StatusfeedbackResolutionopen 
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);
    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;
}
TagsNo tags attached.
Test case number
Attached Files

- Relationships

-  Notes
(0091711)
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;
    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?
(0091713)
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?
(0091714)
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.
(0091715)
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?
(0091829)
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.
(0091838)
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;
// ...

// ...
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);
(0091840)
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.

(0091854)
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.
(0091856)
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:
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.
(0091972)
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.
(0092272)
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
(0092285)
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

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

(0092340)
git (administrator)
2020-05-25 20:58

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
(0092356)
git (administrator)
2020-05-27 11:02

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

(0092360)
git (administrator)
2020-05-27 16:10

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

(0092381)
git (administrator)
2020-05-28 11:26

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


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


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker