Author Topic: BUG: Memory leak at ProjectManagerUI::BuildProjectTree  (Read 6703 times)

Offline lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« on: January 22, 2014, 12:07:00 pm »
I'm using c::b 13.12, there is a memory leak at ProjectManagerUI::BuildProjectTree:

void ProjectManagerUI::BuildProjectTree(cbProject* project, cbTreeCtrl* tree, const wxTreeItemId& root, int ptvs, FilesGroupsAndMasks* fgam)
{
....

    // Now add any virtual folders
    const wxArrayString& virtualFolders = project->GetVirtualFolders();
    for (size_t i = 0; i < virtualFolders.GetCount(); ++i)
    {
        ftd = new FileTreeData(project, FileTreeData::ftdkVirtualFolder);
        ftd->SetFolder(virtualFolders);
        ProjectAddTreeNode(project, tree, virtualFolders, project->GetProjectNode(), true,
                           FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);
    }
.....
}

in the function ProjectAddTreeNode, it allocates a new ftd and copy the content.
so the ftd pointer here will cause a memory leak.

wxTreeItemId ProjectAddTreeNode(cbProject *project, wxTreeCtrl* tree,  const wxString& text, const wxTreeItemId& parent,
                                bool useFolders, FileTreeData::FileTreeDataKind folders_kind, bool compiles, int image,
                                FileTreeData* data)
{
.....

    if (useFolders && pos >= 0)
    {
            .........    

        if (!newparent)
        {
            // in order not to override wxTreeCtrl to sort alphabetically but the
            // folders be always on top, we just search here where to put the new folder...
            int fldIdx  = cbProjectTreeImages::FolderIconIndex();
            int vfldIdx = cbProjectTreeImages::VirtualFolderIconIndex();

            newparent = ProjectFindNodeToInsertAfter(tree, folder, parent, true);

            FileTreeData* ftd = new FileTreeData(*data);
            ftd->SetKind(folders_kind);
            if (folders_kind != FileTreeData::ftdkVirtualFolder)
                ftd->SetFolder(project->GetCommonTopLevelPath() + GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            else
                ftd->SetFolder(GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            ftd->SetProjectFile(nullptr);
            int idx = folders_kind != FileTreeData::ftdkVirtualFolder ? fldIdx : vfldIdx;
            newparent = tree->InsertItem(parent, newparent, folder, idx, idx, ftd);
        }
        ret = ProjectAddTreeNode(project, tree, path, newparent, true, folders_kind, compiles, image, data);
    }
.......    
}



Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5915
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #1 on: January 22, 2014, 12:55:50 pm »
It looks like the bug you recently reported BUG: Memory Leak at cbProject::VirtualFolderAdded goes here now. :)

If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #2 on: January 22, 2014, 06:09:16 pm »
And I am telling again: These objects are de-allocated by wxWidgets on destruction of the tree.

Its a user-data to wx tree items. If you delete them you may even cause crashes... please read the documentation accordingly. I've posted them in another thread dealing with the same issue...

Edit: Read here:
http://docs.wxwidgets.org/trunk/classwx_tree_item_data.html

Quote
The main advantage of having this class is that wxTreeItemData objects are destroyed automatically by the tree and, as this class has virtual destructor, it means that the memory and any other resources associated with a tree item will be automatically freed when it is deleted.
« Last Edit: January 22, 2014, 06:11:08 pm by MortenMacFly »
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #3 on: January 22, 2014, 06:35:36 pm »
And I am telling again: These objects are de-allocated by wxWidgets on destruction of the tree.

The problem here is, that the items are created with new and when they are added, they get copied with the default copy contructor.
The treeitems created with the copy ctor will be destroyed by wxWidgets, but the items created with new will persist and lead to a memory leak.

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #4 on: January 22, 2014, 08:23:59 pm »
I just uplodaded a valgrind logfile, that shows many (possible) memory leaks, many of them in wxTreeCtrl related code.

It's just codeblocks without plugins, build against wx 2.8.12 on Fedor 20 64-bit with gcc 4.8.

http://apt.jenslody.de/downloads/codeblocks18065.vg.xz
It has a little more than 8 MB (more than 600 MB unzipped).

Offline sodev

  • Regular
  • ***
  • Posts: 497
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #5 on: January 22, 2014, 08:40:38 pm »
It *might* lead to a memory leak, the original FileTreeData object gets passed to a recursive call to ProjectAddTreeNode() that does not add it to a tree in every case, however i don't know if these cases can actually happen in the way this method gets called in that context. Without any deeper understanding of the code i would say it would be a good idea to delete the object inside ProjectAddTreeNode() when it returns without adding it to a tree.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #6 on: January 26, 2014, 02:57:25 pm »
I agree with sodev. This is a patch that implements the suggestion. I don't see any other problem with the function.
Does someone know how to make C::B execute the changed paths?
I've set some breakpoints on the added delete calls, but I couldn't make C::B hit them with my projects.

Code
Index: src/src/projectmanagerui.cpp
===================================================================
--- src/src/projectmanagerui.cpp        (revision 9609)
+++ src/src/projectmanagerui.cpp        (working copy)
@@ -2535,7 +2535,10 @@ wxTreeItemId ProjectAddTreeNode(cbProject* project, wxTreeCtrl* tree,  const wxS
     wxTreeItemId ret;

     if (text.IsEmpty())
+    {
+        delete data;
         return ret;
+    }

     wxString path = text;

@@ -2548,7 +2551,10 @@ wxTreeItemId ProjectAddTreeNode(cbProject* project, wxTreeCtrl* tree,  const wxS
         path.Remove(0, 1);

     if (path.IsEmpty())
+    {
+        delete data;
         return ret;
+    }

     int pos = path.Find(_T('/'));
     if (pos == -1)

@jens: I can't download your valgrind log file. :(
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #7 on: January 26, 2014, 03:43:15 pm »
After looking at the other topic about the same issue, I've been able to reproduce it, so I'm committing this change.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: BUG: Memory leak at ProjectManagerUI::BuildProjectTree
« Reply #8 on: January 26, 2014, 03:58:02 pm »
@jens: I can't download your valgrind log file. :(

Should work now, sorry.