Textures are leaked on context shutdown

[rokups]

I have multiple calls to GenerateTexture() of renderer interface, but ReleaseTexture() is never called, even when RmlUI is shut down. Even if i call Rml::ReleaseTextures() before Rml::Shutdown().

[rokups]

To elaborate, this is how i tried to shut down RmlUi and it was leaking textures:

        delete Rml::GetRenderInterface();
        delete Rml::GetSystemInterface();
        delete Rml::GetFileInterface();
        Rml::ReleaseTextures();
        Rml::Shutdown();

When we delete RenderInterface before calling Rml::ReleaseTextures();, RenderInterface tries to free textures from it's destructor. Since my subclass object destructor has already run, we can no longer call my implementation of ReleaseTexture() and Rml::RenderInterface::ReleaseTexture (which is empty) is called instead. Basically calling virtuals from a destructor is not a good idea.

This works:

        Rml::ReleaseTextures();
        delete Rml::GetRenderInterface();
        delete Rml::GetSystemInterface();
        delete Rml::GetFileInterface();
        Rml::Shutdown();

Do we have some documentation detailing shutdown procedure that i missed? This really has to be documented as it is very not straightforward. Maybe even documented above Rml::Shutdown() because it is vital to not miss such details.

[mikke89]

The interfaces must stay alive until after the call to Rml::Shutdown.

This would be the correct order for you:

        Rml::ReleaseTextures();
        Rml::Shutdown();
        delete Rml::GetRenderInterface();
        delete Rml::GetSystemInterface();
        delete Rml::GetFileInterface();

I've tried to document the lifetime requirements several places. Perhaps they could be more strongly worded, I'm all ears on suggestions on how to improve the documentation (or pull requests).

[rokups]

Right, i missed those, my fault :)

But in that case Rml::Shutdown() could be improved. It calls this:

void TextureDatabase::Shutdown()
{
	delete texture_database;
}

Which would eventually call ReleaseTextures() from destructor and that is not correct. Something within Rml::Shutdown() should call Rml::ReleaseTextures();, then if ordering of interfaces is followed resources would be properly released. Also i think Rml::Shutdown() should assert if interface pointers are null already. That would guard against accidental mistakes of people freeing these interfaces too early (if they set their pointers to null anyway).

[mikke89]

Yeah, there seems to be an issue here that generated textures are not released. These textures are not owned by the texture database, but rather (in case of font textures) by the font engine. There is a broader issue here as well that font textures aren't released when they are no longer in use, we should have some functionality for garbage collecting them.

It does make sense to call ReleaseTextures as part of the internal shutdown procedure, on the other hand, if there are any texture copies hanging around then there will be problems if they are used after that. So ideally, all textures should already have been destroyed. Perhaps a better approach is to assert that all textures are in fact gone already at the point of deleting the texture database.

I'll try to come up with some solutions, in the meantime we can call ReleaseTextures() before shutting down to release them from the GPU. But clearly this should not be necessary and needs a solution.

We might as well put some asserts on the interface pointers, but I'm guessing most will just leave them hanging. We could also employ ObserverPtrs to catch this situation properly (perhaps only in debug mode).

[mikke89]

Alright, there was actually an issue here with the internal shutdown order. Some reordering and the font textures should now be released during shutdown.

There could potentially be other generated textures hanging around, so I added a log error in debug mode to look for any leaking textures. Let me know if you ever encounter this :)

Btw, my code in the post above will not work, because Shutdown() will clear the pointers to the interfaces. The order should be correct, but you will need to acquire the pointers first.

There is still the issue of garbage collecting font textures, but that is probably more suitable to discuss in another issue.

[andreasschultes]

I think, some trouble is caused by TextureDatabase::ReleaseTextures(this) in the destructor because it calls the virtual member function ReleaseTextures, but the override implementation is not called if it called from a destructor, so the empty function in the RenderInterface is called, and the texture is leaked. My current workaround is to call TextureDatabase::ReleaseTextures(this) from the RenderInterface implementation class destructor. But not sure if this workaround is a correct c++ standard behavior. Anyway TextureDatabase.h is also not a public interface of RmlUI.

My fast google search for virtual function destructors issues:
Samsung/GearVRf#1567

Should made this a separate bug?

[mikke89]

I was a bit confused at first, but I see the issue now: RenderInterface::~RenderInterface (virtual) -> TextureDatabase::ReleaseTextures (non-virtual) -> TextureResource::Release (non-virtual) -> RenderInterface::ReleaseTexture (virtual, same object as the initial destructor).

So indeed, this is problematic. However, this is really only a problem if the render interface is destroyed before the call to Rml::Shutdown, otherwise it is stopped by a guard in the first call. And, this is already undefined behavior. So this is really just bonus trouble for people already in trouble.

My suggestion is to just remove this call entirely from the destructor, as I don't see the point of it.

Yes, I'd prefer to continue the discussion in a new issue, thanks!