MantisBT - Open CASCADE
View Issue Details
0031075Open CASCADE[OCCT] OCCT:Application Frameworkpublic2019-10-17 10:532020-05-22 16:37
kgv 
mpv 
normalminor 
feedbackopen 
[OCCT] 7.0.0 
[OCCT] 7.5.0* 
0031075: Application Framework - reading STEP file into TDocStd_Document leads to memory leaks
Reading 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;
}
No tags attached.
Issue History
2019-10-17 10:53kgvNew Issue
2019-10-17 10:53kgvAssigned To => mpv
2019-10-17 10:54kgvRelationship addedrelated to 0031072
2019-10-17 10:54kgvProduct Version => 7.0.0
2019-10-17 11:02kgvRelationship addedrelated to 0029269
2020-04-22 10:34mpvNote Added: 0091711
2020-04-22 10:34mpvAssigned Tompv => kgv
2020-04-22 10:34mpvStatusnew => feedback
2020-04-22 10:54abvNote Added: 0091713
2020-04-22 10:55abvAssigned Tokgv => mpv
2020-04-22 11:16mpvNote Added: 0091714
2020-04-22 14:36mpvNote Added: 0091715
2020-04-22 14:36mpvAssigned Tompv => abv
2020-04-28 22:34abvNote Added: 0091829
2020-04-28 22:55abvAssigned Toabv => mpv
2020-04-29 10:21mpvNote Added: 0091838
2020-04-29 10:21mpvAssigned Tompv => abv
2020-04-29 10:35kgvNote Added: 0091840
2020-04-29 10:36kgvNote Edited: 0091840bug_revision_view_page.php?bugnote_id=91840#r22871
2020-04-29 12:41abvNote Added: 0091854
2020-04-29 12:42abvAssigned Toabv => mpv
2020-04-29 13:04mpvNote Added: 0091856
2020-05-06 12:12mpvAssigned Tompv => abv
2020-05-07 09:43abvNote Added: 0091972
2020-05-07 09:43abvAssigned Toabv => mpv
2020-05-20 20:14gitNote Added: 0092272
2020-05-21 10:38gitNote Added: 0092285
2020-05-22 16:37gitNote Added: 0092304

Notes
(0091711)
mpv   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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