Author Topic: BUG: Memory Leak at cbProject::VirtualFolderAdded  (Read 12325 times)

Offline lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
BUG: Memory Leak at cbProject::VirtualFolderAdded
« on: January 05, 2014, 03:59:51 pm »
I'm using c::b 12.11
there is a bug at the function:
bool cbProject::VirtualFolderAdded(wxTreeCtrl* tree, wxTreeItemId parent_node, const wxString& virtual_folder)
{
    wxString foldername = GetRelativeFolderPath(tree, parent_node);
    foldername << virtual_folder;
    foldername.Replace(_T("/"),  wxString(wxFILE_SEP_PATH), true);
    foldername.Replace(_T("\\"), wxString(wxFILE_SEP_PATH), true);
    if (foldername.Last() != wxFILE_SEP_PATH)
        foldername << wxFILE_SEP_PATH;

    for (size_t i = 0; i < m_VirtualFolders.GetCount(); ++i)
    {
        if (m_VirtualFolders.StartsWith(foldername))
        {
            cbMessageBox(_("A virtual folder with the same name already exists."),
                        _("Error"), wxICON_WARNING);
            return false;
        }
    }
    m_VirtualFolders.Add(foldername);

    FileTreeData* ftd = new FileTreeData(this, FileTreeData::ftdkVirtualFolder);   
ftd->SetProjectFile(0);
    ftd->SetFolder(foldername);

    int vfldIdx = Manager::Get()->GetProjectManager()->VirtualFolderIconIndex();

    AddTreeNode(tree, foldername, m_ProjectNode, true, FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);
    if (!tree->IsExpanded(parent_node))
        tree->Expand(parent_node);

    SetModified(true);

//    Manager::Get()->GetLogManager()->DebugLog(F(_T("VirtualFolderAdded: %s: %s"), foldername.c_str(), GetStringFromArray(m_VirtualFolders, _T(";")).c_str()));
    return true;
}

the ftd pointer MUST be delete when leaved the function, is that right?

in the function AddTreeNode, it malloc a new FileTreeData and copy the content of this pointer, so there is a memory leak here.



Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6077
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #1 on: January 05, 2014, 04:17:06 pm »
Code
AddTreeNode(tree, foldername, m_ProjectNode, true, FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);
My guess is that this pointer is hold in the tree, so when the tree node destroys, ftd will be deleted, this is only my guess.
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 lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #2 on: January 06, 2014, 04:02:27 pm »
I compiled C::B using vs2012, and when I execute the above code, there is a memory leak message when i close C::B.
but if i delete the ftd pointer here, it's all OK.



Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6077
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #3 on: January 06, 2014, 04:31:28 pm »
I compiled C::B using vs2012, and when I execute the above code, there is a memory leak message when i close C::B.
but if i delete the ftd pointer here, it's all OK.
Thanks for the test.
Quote
in the function AddTreeNode, it malloc a new FileTreeData and copy the content of this pointer, so there is a memory leak here.
Oh, it is definitely a bug here as you said. :)

BTW: Some one may be interested to see a VC project file for C::B source code, how do you did that? Share it? Thanks.


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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6077
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #4 on: January 07, 2014, 06:52:24 am »
Some information:

At SVN revision 9194(sure it is after 12.11 release), those code was changed to something like:
Quote
Revision: b48f1749e8766e286b4d8c908aac933cae1b6ac8
Author: fuscated <fuscated@2a5c6006-c6dd-42ca-98ab-0921f2732cef>
Date: 2013-7-9 7:06:38
Message:
* sdk: Move lots of code related to the projects tree from cbProject to ProjectManagerUI

git-svn-id: http://svn.code.sf.net/p/codeblocks/code/trunk@9194 2a5c6006-c6dd-42ca-98ab-0921f2732cef
----
Modified: src/include/cbproject.h
Modified: src/include/projectfile.h
Modified: src/sdk/cbproject.cpp
Modified: src/src/projectmanagerui.cpp
Modified: src/src/projectmanagerui.h


Code
void ProjectManagerUI::OnAddVirtualFolder(cb_unused wxCommandEvent& event)
{
    wxString fld = wxGetTextFromUser(_("Please enter the new virtual folder path:"), _("New virtual folder"));
    if (fld.IsEmpty())
        return;

    wxTreeItemId sel = GetTreeSelection();
    if (!sel.IsOk())
        return;

    FileTreeData* ftd = (FileTreeData*)m_pTree->GetItemData(sel);
    if (!ftd)
        return;

    cbProject* prj = ftd->GetProject();
    if (!prj)
        return;

    ProjectVirtualFolderAdded(prj, m_pTree, sel, fld);
//    RebuildTree();
}

So, do you think it still have memory leak about ftd?
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 lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #5 on: January 08, 2014, 04:04:25 pm »
Quote
BTW: Some one may be interested to see a VC project file for C::B source code, how do you did that? Share it? Thanks.

sure, of cause i can share the vs project after i compile all the code of 12.11.
there is still something wrong in my project, and there are still some memory leak in the project, so I must view and debug now.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9723
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #6 on: January 08, 2014, 06:16:04 pm »
Honestly: I am not sure if this is really an issue: The FileTreeData is user data attached to a UI element in the project tree. IMHO it gets automatically destroyed if the UI (and therefore this element) is being removed.

You should consult the wxWidgets docs accordingly.

Edit: See here:
http://docs.wxwidgets.org/trunk/classwx_tree_item_data.html
Especially:
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 08, 2014, 06:25:41 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 lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #7 on: January 22, 2014, 03:55:48 pm »
the wxTreeItemData object will be destroyed only when it's attached to a FileTreeItem,
but in the code here, the ftd is not attached to any FileTreeItem, so it will NOT freed automatically.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9723
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #8 on: January 22, 2014, 06:18:01 pm »
the wxTreeItemData object will be destroyed only when it's attached to a FileTreeItem,
but in the code here, the ftd is not attached to any FileTreeItem, so it will NOT freed automatically.
What do you think does "AddTreeNode(tree, foldername, m_ProjectNode, true, FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);" do?
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 stahta01

  • Lives here!
  • ****
  • Posts: 7785
    • My Best Post
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #9 on: January 22, 2014, 06:36:34 pm »
the wxTreeItemData object will be destroyed only when it's attached to a FileTreeItem,
but in the code here, the ftd is not attached to any FileTreeItem, so it will NOT freed automatically.
What do you think does "AddTreeNode(tree, foldername, m_ProjectNode, true, FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);" do?

I think the issue is inside this function; but, I do NOT know enough C++ to be sure. And the OP is NOT very clear in his posts.

FileTreeData* CompilerGCC::DoSwitchProjectTemporarily() in the file src\plugins\compilergcc\compilergcc.cpp

Edit: But, I thought C++ use of new() let this stuff work better than C malloc; the OP complaining about malloc implies he is not a C++ person also.
So, if you say it right its enough for me.

Tim S.
« Last Edit: January 22, 2014, 06:42:05 pm by stahta01 »
C Programmer working to learn more about C++ and Git.
On Windows 10 64 bit and Windows 11 64 bit.
--
When in doubt, read the CB WiKi FAQ. http://wiki.codeblocks.org

Offline lights_joy

  • Multiple posting newcomer
  • *
  • Posts: 13
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #10 on: January 23, 2014, 03:21:03 am »
the wxTreeItemData object will be destroyed only when it's attached to a FileTreeItem,
but in the code here, the ftd is not attached to any FileTreeItem, so it will NOT freed automatically.
What do you think does "AddTreeNode(tree, foldername, m_ProjectNode, true, FileTreeData::ftdkVirtualFolder, true, vfldIdx, ftd);" do?

Let's see the AddTreeNode function:

Code
wxTreeItemId cbProject::AddTreeNode(wxTreeCtrl*                    tree,
                                    const wxString&                text,
                                    const wxTreeItemId&            parent,
                                    bool                           useFolders,
                                    FileTreeData::FileTreeDataKind folders_kind,
                                    bool                           compiles,
                                    int                            image,
                                    FileTreeData*                  data)
{
    // see if the text contains any path info, e.g. plugins/compilergcc/compilergcc.cpp
    // in that case, take the first element (plugins in this example), create a sub-folder
    // with the same name and recurse with the result...

    wxTreeItemId ret;

    if (text.IsEmpty())
        return ret;

    wxString path = text;

    // special case for windows and files on a different drive
    if ( platform::windows && (path.Length() > 1) && (path.GetChar(1) == _T(':')) )
        path.Remove(1, 1);

    // avoid empty node names in case of UNC paths, then, at least the first two chars are slashes
    while ((path.Length() > 1) && (path.GetChar(0) == _T('\\') || path.GetChar(0) == _T('/')) )
        path.Remove(0, 1);

    if (path.IsEmpty())
        return ret;

    int pos = path.Find(_T('/'));
    if (pos == -1)
        pos = path.Find(_T('\\'));
    if (useFolders && pos >= 0)
    {
        // ok, we got it. now split it up and re-curse
        wxString folder = path.Left(pos);
        // avoid consecutive path separators
        while (path.GetChar(pos + 1) == _T('/') || path.GetChar(pos + 1) == _T('\\'))
            ++pos;
        path = path.Right(path.Length() - pos - 1);

        wxTreeItemIdValue cookie = 0;

        wxTreeItemId newparent = tree->GetFirstChild(parent, cookie);
        while (newparent)
        {
            wxString itemText = tree->GetItemText(newparent);
            if (itemText.Matches(folder))
                break;
            newparent = tree->GetNextChild(parent, cookie);
        }

        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  = Manager::Get()->GetProjectManager()->FolderIconIndex();
            int vfldIdx = Manager::Get()->GetProjectManager()->VirtualFolderIconIndex();

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

            FileTreeData* ftd = new FileTreeData(*data);
            ftd->SetKind(folders_kind);
            if (folders_kind != FileTreeData::ftdkVirtualFolder)
                ftd->SetFolder(m_CommonTopLevelPath + GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            else
                ftd->SetFolder(GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            ftd->SetProjectFile(0);
            int idx = folders_kind != FileTreeData::ftdkVirtualFolder ? fldIdx : vfldIdx;
            newparent = tree->InsertItem(parent, newparent, folder, idx, idx, ftd);
        }
        ret = AddTreeNode(tree, path, newparent, true, folders_kind, compiles, image, data);
    }
    else
    {
        ret = tree->AppendItem(parent, text, image, image, data);
        if (!compiles)
            tree->SetItemTextColour(ret, wxSystemSettings::GetColour(wxSYS_COLOUR_GRAYTEXT));
    }
    return ret;
}
i created a virtual fold named 'test4', the first time called this function, the input parameter was:
   text = "test4\"
   useFolders = true
   data = a new allocated pointer
the following code will be executed:
            FileTreeData* ftd = new FileTreeData(*data);
            ftd->SetKind(folders_kind);
            if (folders_kind != FileTreeData::ftdkVirtualFolder)
                ftd->SetFolder(m_CommonTopLevelPath + GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            else
                ftd->SetFolder(GetRelativeFolderPath(tree, parent) + folder + wxFILE_SEP_PATH);
            ftd->SetProjectFile(0);
            int idx = folders_kind != FileTreeData::ftdkVirtualFolder ? fldIdx : vfldIdx;
            newparent = tree->InsertItem(parent, newparent, folder, idx, idx, ftd);

the new ftd was attached to the tree, but data NOT            
then call AddTreeNode second time:
        ret = AddTreeNode(tree, path, newparent, true, folders_kind, compiles, image, data);
the input parameter was
   text = ""
   useFolders = true
   data = a pointer that was passed at first time

then because text is a blank string, it will return quickly:
    if (text.IsEmpty())
        return ret;

you see, the data pointer input first time was NOT attached to any tree item, so this is the problem!
« Last Edit: January 23, 2014, 03:23:14 am by lights_joy »

Offline dmoore

  • Developer
  • Lives here!
  • *****
  • Posts: 1576
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #11 on: January 23, 2014, 06:06:21 am »
So this code

Code
            FileTreeData* ftd = new FileTreeData(*data);
...
            newparent = tree->InsertItem(parent, newparent, folder, idx, idx, ftd);

Is copying the contents of data into a new structure, but the chunk pointed to by data never gets deleted because it is the copy (ftd) that gets attached to the tree?

Offline dmoore

  • Developer
  • Lives here!
  • *****
  • Posts: 1576
Re: BUG: Memory Leak at cbProject::VirtualFolderAdded
« Reply #12 on: January 23, 2014, 06:09:23 am »
Code
    if (text.IsEmpty())
        return ret;

yes, and I see that this chunk is problematic too because (presumably) data is never deleted by the caller.