MantisBT - Open CASCADE
View Issue Details
0031075Open CASCADE[OCCT] OCCT:Application Frameworkpublic2019-10-17 10:532020-12-02 17:12
kgv 
bugmaster 
normalminor 
closedfixed 
[OCCT] 7.0.0 
[OCCT] 7.5.0[OCCT] 7.5.0 
bugs/caf/bug31075
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;
}
bugs caf bug31075
No tags attached.
parent of 0031681closed bugmaster Community Foundation Classes - Shared Libraries Cannot Be Loaded 
related to 0030563closed abv Community Coding - Standard_Type vptr error 
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
2020-05-25 20:58gitNote Added: 0092340
2020-05-27 11:02gitNote Added: 0092356
2020-05-27 16:10gitNote Added: 0092360
2020-05-28 11:26gitNote Added: 0092381
2020-06-16 13:33gitNote Added: 0092555
2020-06-18 10:22gitNote Added: 0092567
2020-06-18 15:24mpvNote Added: 0092573
2020-06-18 15:24mpvAssigned Tompv => szy
2020-06-18 15:24mpvStatusfeedback => resolved
2020-06-18 15:24mpvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=23043#r23043
2020-06-19 11:32szyNote Added: 0092575
2020-06-19 11:32szyAssigned Toszy => mpv
2020-06-19 11:32szyStatusresolved => feedback
2020-06-19 11:43mpvNote Added: 0092576
2020-06-19 11:43mpvAssigned Tompv => szy
2020-06-19 11:43mpvStatusfeedback => resolved
2020-06-19 12:19szyNote Added: 0092577
2020-06-19 12:19szyAssigned Toszy => bugmaster
2020-06-19 12:19szyStatusresolved => reviewed
2020-06-20 13:34bugmasterNote Added: 0092599
2020-06-20 13:34bugmasterStatusreviewed => tested
2020-06-20 13:35bugmasterTest case number => bugs/caf/bug31075
2020-06-20 13:36bugmasterChangeset attached => occt master ef779ae0
2020-06-20 13:36bugmasterStatustested => verified
2020-06-20 13:36bugmasterResolutionopen => fixed
2020-06-20 13:42gitNote Added: 0092604
2020-06-20 13:42gitNote Added: 0092606
2020-06-20 13:42gitNote Added: 0092615
2020-07-21 18:49BenjaminBihlerRelationship addedparent of 0031681
2020-08-28 14:46kgvRelationship addedrelated to 0030563
2020-12-02 16:40emoFixed in Version => 7.5.0
2020-12-02 17:12emoStatusverified => closed

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

(0092340)
git   
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   
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   
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   
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

(0092555)
git   
2020-06-16 13:33   
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

(0092567)
git   
2020-06-18 10:22   
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
(0092573)
mpv   
2020-06-18 15:24   
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/ [^]
(0092575)
szy   
2020-06-19 11:32   
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.
(0092576)
mpv   
2020-06-19 11:43   
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.
(0092577)
szy   
2020-06-19 12:19   
Reviewed.
(0092599)
bugmaster   
2020-06-20 13:34   
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
(0092604)
git   
2020-06-20 13:42   
Branch CR31075_2 has been deleted by inv.

SHA-1: 3ffdb3fda4ae189e9398a1f0c3c96ca8896575f3
(0092606)
git   
2020-06-20 13:42   
Branch CR31075_1 has been deleted by inv.

SHA-1: 804ede7e5ebf5517bf2d53e4a1003a06193f7f0e
(0092615)
git   
2020-06-20 13:42   
Branch CR31075 has been deleted by inv.

SHA-1: ceadb458a032213cf2f9229230dcf4c3d237f0f7