Memory Leaks (Again?)

I’m using collada-dom (tried both head code and version 1.3.0) via the OpenSceneGraph plugin. So far its done a decent job of importing documents, however I’m finding a bunch of memory leak messages when a visual studio debug session ends. After debugging for a while, the unfreed allocations seem to be the ones done in collada-dom. Am I doing something wrong, or is it just a fact that the library leaks right now. I know this was discussed previously (2006). I’m wondering if the leaks have crept back in.

My code does call the following cleanup at the end:

        if(_dae != NULL){
            delete _dae;
            _dae = NULL;

Is this not enough to clean things up?

I tried loading and releasing a document, but didn’t see any memory leaks in the debug output. I also tried running COLLADA RT under a debugger, but still didn’t see any memory leaks. What version of Visual Studio are you using? I was testing with 2005. Do you have a small test app with full source I can use to reproduce the problem?

Also, as long as you’re using DOM version 1.2.0 or later (1.2.0 was released in Nov 2006), it’s not necessary to call DAE::clear or DAE::cleanup. Just deleting the DAE object is enough.

Thanks for the reply. I’m using Visual Studio 2005 Pro. I don’t have a small test app, because I’m using Collada-DOM via the DAE importer in the OpenSceneGraph library, so the code is dependent on that framework. When I saw the messages about memory leaks when I exited the app, I did the following:

  1. Put a breakpoint in the destructor code thats supposed to clean-up the DAE, and made sure it is being called.

  2. Since the leaking allocation numbers were consistent across runs, I used the debug runtimes to break on those allocations, and they all seemed to be coming from in the collada-dom code.

It could be that I’m using the API incorrectly. Is there any obvious/common pitfalls that could cause this type of memory leaking? I will just go over my code (and the other 3rd party library code) to make sure nothing silly is being done.

The only thing I can think of is if you use a custom plugin of some sort (custom I/O plugin or error handler), you need to delete the object yourself. The DOM doesn’t take ownership of it (I plan to change that at some point).

Other than that, the DOM does a lot of memory management via reference counting, so you’re not responsible for deleting individual elements. I tested the DOM in a few apps and didn’t get any memory leak messages in Visual Studio 2005. Let me know if you figure anything out. As you hinted at, the DOM used to leak memory like crazy, but I believe all of those issues have been fixed.

I PM’d you the details as I was investigating. It seems like it’s the _CMData members of a number of classes (seperately defined in domNode, domGeometry, …) that are at the heart of the leaks. _CMData is defined to be an array of pointers to arrays. The destructors for the various classes (domNode, domGeometry, …) do not do any specialized cleanup of the arrays. So it would seem like the array of pointers is cleaned up nicely, but the arrays being pointed to have been leaked.

I made the following change to one class (domNode) and it reduced the number of leaks reported by visual studio. I did it to another class (domGeometry), and it reduced the number of leaks again. It would seem either something is systematically wrong with the classes, or the way they’re being used.

	virtual ~domNode() {
		for( unsigned int i = 0; i < _CMData.getCount(); i++ ) {
			daeCharArray *CurrentItem = _CMData.get(i);
				delete CurrentItem;
				CurrentItem = NULL;

Is this a leak, or am I mistaken?

Memory ownership in the DOM can be very confusing. This is a good example.

In this particular situation, it looks like domNode (or any dom* object) calls daeMetaElement::addCMDataArray, which in turn exposes the array via daeMetaElement::getMetaCMData. The daeElement destructor calls that function to get the array, and then frees up the data:

// From daeElement::~daeElement
if ( _meta != NULL && _meta->getMetaCMData() != NULL )
	daeTArray< daeCharArray *> *CMData = (daeTArray< daeCharArray *>*)_meta->getMetaCMData()->getWritableMemory(this);
	for ( unsigned int i = 0; i < CMData->getCount(); i++ )
		delete CMData->get(i);

If that data isn’t being freed it’s probably because the elements aren’t being destroyed. As I mentioned, they’re reference counted, so it’s not immediately obvious to me why they wouldn’t be destroyed. Circular references maybe?

Ok. I got it to break in that daeElement destructor for one of my leaking objects. The daeElement seems like its being destroyed correctly. The problem is that when it goes to delete the array elements in that destructor, the array seems empty (zero count)… this is despite the fact that stuff was added to it earlier. I did some more debugging, and it seems like this is a problem with the order in which destructors are called.

Taking a domMesh object as an example. I can see that the _CMData member (which is declared in the domMesh class) is being destructed/cleaned up as part of the ~domMesh automatic destructor. After that, the domElement destructor is called, but by this time, _CMData is empty and the leak has already occurred. The callstack at the time the _CMData array is emptied looks like this:

 	osgdb_daed.dll!daeArray::clear()  Line 36	C++
 	osgdb_daed.dll!daeTArray<daeTArray<char> *>::clear()  Line 155	C++
 	osgdb_daed.dll!daeTArray<daeTArray<char> *>::~daeTArray<daeTArray<char> *>()  Line 145	C++
>	osgdb_daed.dll!domMesh::~domMesh()  Line 212 + 0x60 bytes	C++
 	osgdb_daed.dll!domMesh::`scalar deleting destructor'()  + 0x2b bytes	C++
 	osgdb_daed.dll!daeElement::release()  Line 30 + 0x35 bytes	C++

So, to recap, the destructors are called in this order:

  1. ~domMesh
  2. ~domElement

The _CMData member is automatically destructed in ~domMesh, so by the time ~domElement comes around, it finds an empty array, and fails to properly release the array contents.

Good catch! I’m in the middle of some other stuff now, but I’ll try to look at this tomorrow. Thanks for narrowing it down.

Great. I’m curious why this wasn’t showing up as a leak for you guys, but I’ll just be satisfied to see it fixed. Please let me know when you get a chance to fix it.

I’m not sure why I didn’t see the leaks with VS2005. I should’ve been using valgrind on Linux anyway. Valgrind spotted the _CMData leak as well as a much larger leak that recently slipped into the libxml code. I fixed both leaks in revision 166. Valgrind now reports no leaks in my DOM test app. Thanks again for the help in tracking this down :slight_smile:

Excellent. You’re welcome, I’m happy to help.

I went and updated to the latest code revision in SVN, and I’m having trouble linking my project now. I had a look, and I see you guys moved the lib directories. I adjusted my project settings, but because the libs no longer have different filenames for the release and debug configs, it’s very hard for me to have my release project link to the release version of the lib and the debug project link to the debug version of the lib. I could bring the collada projects into my solution, but I didn’t want to have to do that. I’m going away for the weekend, and I’ll try my hand at it again when I get back.

Thanks again for your help.

Yeah I changed the location of the output files, and the output file names. Any time I make a change that might require adjusting code (this happens rarely) or your build setup (this has been more common), I make a note of it on this page.

Since the debug and release files go to separate output directories, it was redundant to encode that information in the file name itself. If you’re building with Visual Studio, you can add the debug DOM build location to the “Additional Library Directories” field in the linker settings for the debug config of your app, and add the release DOM build location for your release config. Then in the “Additional Dependencies” field, add libcollada_dae.lib and libcollada_dom.lib (or libcolladadom13.lib if you’re linking dynamically). That should work to get all configurations of your app linking against the corresponding DOM files. There’s no need to include the DOM projects in your solution file. Take a look at the COLLADA RT build setup for an example.

You are right, I can add the appropriate dirs to the ‘Additional Dependencies’ field, and since I can specify a different value for that depending on the build config (debug/release), it would work.

The problem is just what happens when I go to share the source code or provide it to a client/customer. My VCPROJ now has a reference to a library dir, and the client/customer probably doesn’t have the same directory structure for libraries. I could specify the path as a relative path, but then the collada-dom would have to be in the same place relative to my project’s code. One way to acheive this would be to distribute the collada-dom code along with my code (via an external libs directory for example), but my preference is if all the libraries my project uses are distributed seperately, and are part of the development environment’s libraries/include path, and my project just references them by filename. This was possible when the debug/release versions had different filenames, but I don’t think I can do it now that they have the same filenames.

Its not a correctness thing, its just an ease-of-use thing.

You have a few options (the first of which you’ve already mentioned):

(a) Distribute the DOM in an “external libs” folder with your library, and use a relative path in your project settings to link against the DOM. I imagine this would really make things simpler for clients of your library, since they won’t have to download the DOM, build it, and alter the Visual Studio library search paths. In my experience, projects that make me track down and setup all their dependencies are much harder to work with.

I’m in the process of changing the DOM to work this way. When you grab the DOM you’ll also get any third-party libraries it needs to build (like libxml) in the form of headers and .libs, so you’ll be able to do an svn checkout and then build with no additional setup required. This obviously makes building the DOM from source much simpler.

(b) Keep your current setup, but link against the release version of the DOM in both configs of your library. So instead of telling your clients to add two DOM paths to the library search paths in Visual Studio (one for debug and one for release), you would tell them to only add a path for the release DOM build. If your clients need to debug the DOM for some reason, they can change the library search path to point to the debug DOM and rebuild. This is sort of how COLLADA Refinery works; Refinery always uses the release DOM, although it comes as a pre-built library with Refinery, so the client doesn’t have to set the DOM up themselves.

Hopefully one of these options works out for you. Let me know what you decide to do. By the way, what library is it that you work on? OSG?

I’m going to attempt to use option (a), but in the grand old tradition of software customers, I’m going to bitch and moan about it. :wink:

Sorry, I hit submit instead of preview on that last message before I was done with it.

My gripe (respectfully put, as I benefit from the use of your library, and as such, am not in a place to tell you how to do things), would be that by adding one little letter to the filename in debug configs, you could enable one more usage pattern or workflow for your library. I see the merits of techniques (a) and (b) that you described, and yes, they do make it easier for a client to build a project from source. However, since collada-dom is but one of the libraries my project depends on (it also has dependencies on OpenSceneGraph and wxWidgets), I have to find a way to use all the libraries in a way that they will all work. While collada-dom might be pushing the ‘libraries bundled with project source’ approach, OSG or wxWidgets may not be as easy to use in that manner.

And in response to your question about what library I work on, I don’t really develop on any library, I’m just doing some contract work for a client that needed a simple 3d app for a niche market. The app would not be a viable option if it had to be built from scratch, so 3rd party dependencies are a big part of the project. As mentioned before, these include OpenSceneGraph, wxWidgets, collada-dom, and zlib.

Again, I do not want to offend, as the availability of collada-dom is of great benefit to me, I just think that this minor change to the project organization makes it harder to use. I realize the needs of one user do not control a project, so I will deal with change and move on. Thank you again for taking care of the memory issue. My last run with the debug version of collada-dom showed no memory leaks.

Thanks for the feedback. I feel that the solution I came up with for configuration naming hits the sweet spot in the trade-off between flexibility and simplicity. Each configuration has its own folder, and then the files inside don’t have to explicitly identify themselves as belonging to a particular config. One important thing to keep in mind is that there are many more configurations than just debug and release. There’s also static lib vs. DLL, Collada 1.3 vs. Collada 1.4, etc.

Another option that might work for you is to do what you were doing before, but instead of requiring users to modify the global Visual Studio include and lib search paths, require them to modify your project settings to point to the right place. Then they could change your project to point to the debug DOM in the debug settings and the release DOM in the release settings. I still think you should be including the dependencies you need (especially since you have several dependencies), but this would probably be better than modifying the global search paths. In general I recommend against relying on the global search paths, since it makes it very difficult to have two different branches of your software which use different versions of the dependencies sitting on the same machine. For example suppose you have libA 1.0 which uses DOM 1.3.0 via the global search paths. Then you come out with libA 2.0 which is going to use DOM 1.4.0, also via the global search paths. Now it’s a pain in the ass to switch between building libA 1.0 and libA 2.0, since you’ll have to go in and switch the global search path to point to the right version of the DOM.

That’s really just another argument for including the dependencies you need with your project. Thanks again for the help tracking down the memory leak.