Virtual function call in destructor ~RenderInterface, can cause texture resources leaks in RenderInterface implementations

[andreasschultes]

Virtual function call in destructor ~RenderInterface, can cause texture resources leaks in RenderInterface implementations

Reason:
RenderInterface::~RenderInterface (virtual) -> TextureDatabase::ReleaseTextures (non-virtual) -> TextureResource::Release (non-virtual) -> RenderInterface::ReleaseTexture (virtual, same object as the initial destructor)
The custom implementation is never not called!

To the usecase:
It is not required to set a default Rml::RenderInterface, every Rml::Context can have it own Rml::Context. If the Rml::Contextis destroyed, the belong Rml::RenderInterface can also be destroyed? I think there are some Font Textures, which are still allocated. I'll check that out later.

TODO:

• Check If the usecase is allowed, do we need a default render interface or can it context dependent?
• Are font textures context dependent?
  Rml::TextureResource creates for each Rml::RenderInterface a texture, with a load from disk or a texture creation callback function. All get function has an Rml::RenderInterface as parameter, if the texture isn't created, than it will be created for the Rml::RenderInterface. In conclusion, the gpu textures can be released if the are not in use by any rendering functions. They can be recreated by request. A call to TextureDatabase::ReleaseTextures(renderInterface) will release all gpu textures belonging to renderInterface. The TextureResource is still alive, and can be used by other Rml::RenderInterfaces even these which are not created until now.
• Fix the virtual destructor Issue

Refs:
#133 (comment)

[mikke89]

Thanks for the detailed report.

So first I want to emphasize what I wrote in the linked issue:

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.

So in normal cases, this is not an issue, as the render interface will/should be kept alive until after the call to Rml::Shutdown.

However, the lifetime requirement when using the non-default render interface for a context is actually tied to the context, as currently stated:

RmlUi/Include/RmlUi/Core/Core.h

Line 99 in 16e6a67

/// @lifetime If specified, the render interface must be kept alive until after the context is destroyed or the call to Rml::Shutdown.

So, this may become a problem if all of the following are true:

1. User creates a non-default render interface and instantiates a context with this render interface.
2. User later destroys the same context.
3. There are some font textures or similar hanging around referencing the render interface tied to the now destroyed context. Not sure if this can actually happen, but we haven't ruled it out.
4. User then destroys the render interface before the call to Rml::Shutdown. (Currently allowed as per the lifetime requirements above).

Then, during 4., we would have a call to a virtual function in the destructor.

Proposed solution

• Change the lifetime requirement so that all render interfaces need to be kept alive until after the call to Rml::Shutdown.
• Remove the function call in the render interface destructor entirely. There is no purpose for this when users respect the above requirement.

I'm not sure if this would cause any trouble for users, I can't really see the use case for creating and destroying lots of render interfaces over and over again, which is the only case where I can see this lifetime requirement matter.

Thoughts?

[rokups]

Proposed solution sounds good. Sprinkle some asserts so that code immediately breaks if new lifetime rules are not honored and its perfect.

[andreasschultes]

The renderInterface contains the gpu resources in it's own format, so if I want for example change it format(compression, mipmaps,...) I can do this with recreating render interface and context. Other cases are GPU switches, offline rendering...
I would prefer to add a Shutdown method to the interface containing TextureDatabase::ReleaseTextures call. This would be only needed to call if Rml::Shutdown wasn't called before.
Another solution would be to export TextureDatabase::ReleaseTextures call, and make it a requirement for the final destructor in the renderInterface.

[mikke89]

Alright, this should now be fixed, let me know if there are any issues.

So, I updated the lifetime requirements, but not as strict as suggested previously. It is still possible to destroy the render interface before the call to Rml::Shutdown, with the following requirements:

/// @lifetime If specified, the render interface must be kept alive until after the call to Rml::Shutdown. Alternatively, the render interface can be
///           destroyed after all contexts it belongs to have been destroyed and a subsequent call has been made to Rml::ReleaseTextures.

I went ahead and changed Rml::ReleaseTextures so that you can call it for only a single interface as well. And also added an assert in debug mode so we will bother the user if they destroy the render interface before all references to it has been cleared.

I hope this solution should cover all your use cases @andreasschultes.

[andreasschultes]

Thanks, I think that is a good solution