Strange problem using XmlElement objects


#1

Hey
I’ve come across a strange problem and a couple of confusing memory leaks when using XmlElement objects. I’m sure it’s a just simple problem that I don’t understand simply from my lack on knowledge on using the XmlElement class.

In a nut shell, I am trying to implement a system that allows settings within an app to be saved into a number of ‘presets’ which can then be saved as a group to disk. The saved file can then be loaded back into the app and split back into these separate presets. I’m sure you’re familiar with this idea. In my program I’ve created an OwnedArray of XmlElement objects (which are the ‘presets’) which get given child elements which hold data/settings. When the presets need saving to disk each of these objects in the OwnedArray are added as child elements to another XmlElement which is then saved to disk as an XmlDocument. Below is a simple example of my program:

.h file:

[code]//
// DataStorage.h
// sdaJuce
//
// Created by Liam Meredith-Lacey on 25/11/2011.
// Copyright 2011 UWE. All rights reserved.
//

#ifndef H_DATASTORAGE
#define H_DATASTORAGE

#include <juce.h>

class DataStorage
{
public:
DataStorage();
~DataStorage();

void save();
void load();

void savePreset (int presetNumber);

private:
OwnedArray presetData;

};

#endif //H_DATASTORAGE[/code]

.cpp file

[code]//
// DataStorage.cpp
// sdaJuce
//
// Created by Liam Meredith-Lacey on 25/11/2011.
// Copyright 2011 UWE. All rights reserved.
//

#include “DataStorage.h”

DataStorage::DataStorage()
{
//init OwnedArray
for (int i = 0; i < 3; i++)
{
presetData.add(new XmlElement(“PRESET_” + String(i)));
}
}

DataStorage::~DataStorage()
{

}

void DataStorage::save()
{
//open FileChooser
FileChooser saveFileChooser(“Create a file to save…”, File::getSpecialLocation (File::userDesktopDirectory), “*.tst”);

if (saveFileChooser.browseForFileToSave(false))
{
    //create a file based on users selection
    File savedFile(saveFileChooser.getResult());
    String stringFile = savedFile.getFullPathName(); 
    stringFile = stringFile + ".tst"; 
    savedFile = stringFile;
    
    bool overwrite = true;
    
    //check to see if the file already exists
    if (savedFile.existsAsFile())
        overwrite = AlertWindow::showOkCancelBox(AlertWindow::WarningIcon, "This File Already Exists!", "Are you sure you want to overwrite this file?");

    
    if (overwrite == true)
    {
        savedFile.deleteFile();
        savedFile.create();
        
        //create new XmlElement
        XmlElement *savedData = new XmlElement ("SAVED_DATA");
        
        //add presetData XmlElements as Child XmlElements
        for (int i = 0; i < 3; i++)
        {
            std::cout << "Child element being added to SAVED_DATA\n";
            savedData->addChildElement(presetData[i]);
        }
        std::cout << "All Child elements have been added to SAVED_DATA\n";
    
        //create document
        String xmlDoc = savedData->createDocument(String::empty, false);
        //add to saved File
        savedFile.appendText(xmlDoc);
    }
}

}

void DataStorage::load()
{
//open FileChooser
FileChooser loadFileChooser(“Select a .tst file to load…”, File::getSpecialLocation(File::userDesktopDirectory), “*.tst”);

if (loadFileChooser.browseForFileToOpen())
{
    //get file selected by the user
    File loadedFile (loadFileChooser.getResult());
    
    //parse selected XmlDocument to an XmlElement
    XmlElement *loadedXml = XmlDocument::parse(loadedFile);
    
    if (loadedXml != nullptr && loadedXml->hasTagName("SAVED_DATA"))
    {
        //load the child elements of loadedXml and put them in the presetData objects
        for (int i = 0; i < 3; i++)
        {
            //clear the xmlelement for the current preset number
            presetData[i]->deleteAllChildElements();
            
            std::cout << "Child element being loaded from SAVED_DATA\n";
            
            //put the loaded xml data into the xmlelement for the current preset
            presetData.insert (i, loadedXml->getChildByName("PRESET_" + String(i)));
        }
    }
}

}

//adds child XmlElement’s to the presetData XmlElements
void DataStorage::savePreset (int presetNumber)
{
//clear presetData object
presetData[presetNumber]->deleteAllChildElements();

//create child XmlElement
XmlElement *testData1  = new XmlElement ("TEST_DATA_1");
//give it some data
testData1->setAttribute("A", 1);
testData1->setAttribute("B", 2);
testData1->setAttribute("C", 3);
//add as a child to presetData
presetData[presetNumber]->addChildElement(testData1);

//create child XmlElement
XmlElement *testData2  = new XmlElement ("TEST_DATA_2");
//give it some data
testData2->setAttribute("A", 4.0);
testData2->setAttribute("B", 5.0);
testData2->setAttribute("C", 6.0);
//add as a child to presetData
presetData[presetNumber]->addChildElement(testData2);

//create child XmlElement
XmlElement *testData3  = new XmlElement ("TEST_DATA_3");
//give it some data
testData3->setAttribute("A", "Seven");
testData3->setAttribute("B", "Eight");
testData3->setAttribute("C", "Nine");
//add as a child to presetData
presetData[presetNumber]->addChildElement(testData3);

}
[/code]

Here are the two problems I’m getting:

  1. After running either the load() or save() functions I get a ‘Leaked object detected: 1 instance of class XmlElement’ error.
  2. When trying to run either the load() or save() functions for a second time the program get’s stuck indefinitely in the function, usually within the functions for loops.

Any answers to why I’m running into these to problems would be greatly appreciated; I’ve been trying to fix this for many many hours!
Thanks.


#2
this gets never deleted, whenever you type "new" a portion of heap memory will be allocated, try use "delete saveData" at the end

or use a scoped pointer:
ScopedPointer<XmlElement> savedData = new XmlElement ("SAVED_DATA");

or just create it on the stack
XmlElement savedData("SavedData");

this gets never deleted, whenever you type “new” a portion of heap memory will be allocated, try use “delete saveData” at the end

or use a scoped pointer:
ScopedPointer savedData = new XmlElement (“SAVED_DATA”);

or just create it on the stack
XmlElement savedData(“SavedData”);


#3

oops, if you add elements that are owned by another ownedArray, you will get double delete exceptions, … you have have to choose a different design


#4

chkn, I have tried using ScopedPointers and delete like you have suggested but this usually ends in a BAD_EXC_ACCESS error within the LinkedListPointer class, which I’ve looked into and don’t fully understand what it is, but I’m getting the impression that this error is caused by the double delete exceptions that i’m creating?

I have also tried using a standard array of pointers to XmlElement objects (XmlElement *presetData[3]:wink: with no OwnedArray’s whatsoever, but this too causes the program to get stuck indefinitely in the load() and save() functions the second time they are run.

Any other thoughts??


#5

if you use ScopedPointer don’t use delete (the first one takes care of that).
Also i’d recommend using the ValueTree class it’s much easier, then just if you want to write xml files use the createXml() method and that’s it.


#6

The problem is that you are copying nested elements without removing them from their parents so they get deleted twice e.g.

XmlElement *loadedXml = XmlDocument::parse(loadedFile); and presetData.insert (i, loadedXml->getChildByName("PRESET_" + String(i)));

Now the child you added to presetData will be deleted twice, once when loadedXml gets deleted (if its in a scoped pointer) and when presetData gets deleted.

Try either removing the child you insert from loadedXml, removeChildElement (XmlElement *childToRemove, bool shouldDeleteTheChild) or create a deep copy of it.

I think I would do something like:

ScopedPointer<XmlElement> loadedXml = XmlDocument::parse(loadedFile); XmlElement* childToInsert = loadedXml->getChildByName("PRESET_" + String(i)); presetData.insert (i, childToInsert); loadedXml->removeChildElement (childToInsert, false);

or if its not a large Xml or time critical:

ScopedPointer<XmlElement> loadedXml = XmlDocument::parse(loadedFile); XmlElement* childToInsert = loadedXml->getChildByName("PRESET_" + String(i)); presetData.insert (i, new XmlElement (*childToInsert));


#7

Thanks dave - very clearly explained.

(Personally, I’d probably use an Array instead of an OwnedArray.)


#8

Thanks Dave, this seems to be solving it!
I can now ‘load’ time after time without any problems however I’m yet to figure out how to apply the solution to the save() function to get rid of the issue once and for all. Any final hints?


#9

You’re just not deleting the XmlElement you create, put it in a scoped pointer like the other one.

[code]change
XmlElement *savedData = new XmlElement (“SAVED_DATA”);

to

ScopedPointer savedData = new XmlElement (“SAVED_DATA”);
[/code]

To be safe, you “usually” put everything you create with new in a ScopedPointer. Its easier to start this way and then get double deletion asserts if something else takes ownership of the object, all the Juce methods that do this specify it explicitly in the docs though.


#10

Thanks Dave, it’s all fixed now!
I understood the ScopedPointer stuff fine but what I was really struggling to see was that the presetData objects were being deleted too soon as I wasn’t removing them as child elements of any new XmlElement objects that they were being copied too.
Many many thanks!!


#11

Hi all,

Soryy, but I need to bump this. I have encountered a similar problem recently which I could not find the solution yet. I am using an xml file to save and load presets (which has a size around 1.5mb). Whenever I call one of these save/load functions, I get a memory leak around that size (1.5-2mb) and cannot find out why. Here is the code snippet that I am using for saving a preset:

void CosmosProcessor::setXml(int press){

	String presetName = "Preset_" + String(press);
	ScopedPointer<XmlElement> xmlState = XmlDocument::parse(LoadFile(myBank));
	
	// Preset element
	XmlElement * xmlPreset = xmlState->getChildByName(presetName);
	XmlElement * xmlCurrent;

	// Meso
	xmlCurrent = xmlPreset->getChildByName("Meso");

	xmlCurrent->setAttribute("PresetName", presetname);
	xmlCurrent->setAttribute("PresetNo",press);
	xmlCurrent->setAttribute("SampleFilePathA",mySamplePathA);
	xmlCurrent->setAttribute("SampleFilePath",mySamplePath);

	for (int iii=0; iii<10; iii++) {
		xmlCurrent->setAttribute("mesoMute"+String(iii+1)+"param",myMesoMutes[iii]);
		xmlCurrent->setAttribute("microMute"+String(iii+1)+"param",myMicroMutes[iii]);
	}

	// MesoP
	xmlCurrent = xmlPreset->getChildByName("MesoP");

	xmlCurrent->setAttribute("SampleFilePathA",mySamplePathAP);
	xmlCurrent->setAttribute("SampleFilePath",mySamplePathP);

	// Micro
	xmlCurrent = xmlPreset->getChildByName("Micro");

	xmlCurrent->setAttribute("MicroCellDensity",myMicroDensity);
	xmlCurrent->setAttribute("MicroDensityDist",myMicroDensMod);
.
.
.
.
.
	xmlState->writeToFile(LoadFile(myBank), "");

	presetNames[press] = presetname;

	//xmlState->deleteAllChildElements();
}

and the file loading function (in case it might be related) :

[code]static File LoadFile(String path)
{
#if JUCE_WINDOWS
path = File::getSpecialLocation(File::currentApplicationFile).getParentDirectory().getFullPathName() + “/data/” + path;
#elif JUCE_MAC
path = File::getSpecialLocation(File::currentApplicationFile).getFullPathName() + “/Contents/Resources/data/” + path;
#endif

return File(path);

}[/code]

I suppose all the XmlElement objects - that xmlPreset and xmlCurrent pointers point to - are deleted automatically when xmlState deletes itself at the end, since they all are children of xmlState. Am I missing something?


#12

Any suggestions?


#13

How are you detecting your leak? Are you sure it’s the XmlElement object? If so all JUCE classes use an internal leak detecter that should assert upon application shutdown, is this what you’re seeing? Does it say something like “***Leaked object detected, 1 instance of XmlElement”? If not then you might actually be leaking something else. As a general rule of thumb you should always include the JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR in all your classes (unless you explicitly provide a copy constructor and assignment method) this will help you detect what objects you are actually leaking.


#14

Thanks for the response. As a matter of fact, I cannot really see those assertions as I am working on an audio plugin (no idea really how to debug without host or so). I would appreciate if you know a way to do that. Anyway, so I am just looking at the memory usage from task manager. Yet, it’s unlikely that the leak is elsewhere. It appears only when I call the function explicitly, and as you can see from the code, I assume it has nothing really dependent to the rest of the code. Having said that, does it seem to you that my code is leak-free?


#15

After a quick look it doesn’t appear to leak but there is a section emitted and I wouldn’t say that the code is completely robust. What it the File returned from LoadFile isn’t a valid XML document, the parsing will fail and return a nullptr which you then de-reference?

I don’t really think you should be using the task manager as any indication of anything, who knows what it does? What it caches, how often it updates etc. Use your debugger, that’s what it’s there for. Yes, seeing an ever increasing amount of memory usage in the Task Manager is probably an indication of a leak but its not where I would go to identify it.

What you need to do is run a host as an executable which your debugger attaches to and then load your plugin. Something simple is usually best, perhaps try the JUCE plugin host demo. The other alternative is to create a stand alone version of your plugin and build it as an app then transfer the everything except the app class to your plugin. Have a look at the StandaloneFilterWindow class.

I mainly develop on the Mac so when doing plugin work usually launch AU Lab (because it’s minimal and quick to launch) with a session that loads my plugin as a command line argument so it loads everything automatically and attaches it to the debugger. Can’t remember exactly how to do the same in VS but I’m sure some more Windows orientated users on here can help or have a quick Google.


#16

Thank you for the suggestion, I guess Juce plugin host demo would do the trick. :slight_smile: I was unable to debug in that way because it was not possible to load a plugin with a command line argument in the host I was testing.