frontend: Fix canvas removal causing crashes due to recursive remove calls

[dsaedtler]

Description

Makes some changes to how canvas removal is handled to avoid recursion in the middle of an erase() operation on the vector holding frontend-managed canvases.

Also prevents a similar issue when clearing scene data by replacing .clear() with a dedicated method that also ensures the canvas removal signal is properly fired for each removed canvas.

Motivation and Context

See #12728, which this PR superseedes.

How Has This Been Tested?

Reproduced using the following snipped (just added to the end of the OBSBasicSettings::OBSBasicSettings() constructor):

	obs_video_info ovi;
	obs_get_video_info(&ovi);
	OBSCanvasAutoRelease cv = obs_frontend_add_canvas("test", &ovi, PROGRAM);
	obs_frontend_remove_canvas(cv);
	OBSCanvasAutoRelease cv2 = obs_frontend_add_canvas("test2", &ovi, PROGRAM);

Note that the crash only happens in release builds, not debug.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Tweak (non-breaking change to improve existing functionality)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[PatTheMav]

Alright did a lot more debugging today and came up with this solution:

master...PatTheMav:obs-studio:canvas-memory-management-fix

Notes:

• Fixes a potential crash in OBSStudioAPI::obs_frontend_add_canvas, as passing a nullptr as the const char *name will crash in the std::string constructor. It's always good practice not to trust any input sent via a public API and this one seems mandatory to me.
• OBSStudioAPI::obs_frontend_add_canvas explicitly retains a reference to the canvas object while it is "shared" with the API client. This increments the actual reference count to at least "1".
• OBSStudioAPI::obs_frontend_remove_canvas explicitly releases this shared reference once the API client requests its deletion. This should decrement the actual reference count to no less than "0".
• Canvas::~Canvas is a "dumb" destructor now, much like Canvas::Canvas(obs_canvas_t *canvas) is a "dumb" constructor - neither bothers with reference counts or the lifetime of the pointer it holds.
• OBSBasic (via OBSBasic::AddCanvas) is the initial owner (and creator) of the canvas object. Thus OBSBasic::RemoveCanvas needs to be the destructor of the same object. This requires an explicit call to obs_canvas_release to ensure that the implicit "0th" reference it created is released.
• Similarly OBSBasic (via OBSBasic::LoadData and OBS::Canvas::LoadCanvases) is the initial owner (and indirect creator) of the canvas objects created from scene collection data. This requires OBSBasic::ClearSceneData to explicitly release the "0th" references it creates this way. This is potentially the most "confused" part of the current class implementation, but that's due to OBS::Canvas not being allowed to be the true owner of canvas objects (and instead is only created as a "dumb" wrapper around them).

[dsaedtler]

Alright did a lot more debugging today and came up with this solution:

master...PatTheMav:obs-studio:canvas-memory-management-fix

I think in that version you're still double-releasing the canvas, once in obs_frontend_canvas_remove() and then again in RemoveCanvas().

I wanted to maintain the RAII wrapper OBS::Canvas as being what is responsible for ultimately releasing the canvas it is responsible for when it gets destroyed.

[PatTheMav]

Alright did a lot more debugging today and came up with this solution:
master...PatTheMav:obs-studio:canvas-memory-management-fix

I think in that version you're still double-releasing the canvas, once in obs_frontend_canvas_remove() and then again in RemoveCanvas().

I wanted to maintain the RAII wrapper OBS::Canvas as being what is responsible for ultimately releasing the canvas it is responsible for when it gets destroyed.

You are right, I missed that OBSCanvasAutoRelease cv will call "release" even though it never called "retain" and is unbalanced by design (and unintuitive). So the "0th" reference of a libobs object needs to be destroyed explicitly by calling code, either by doing an unbalanced extra "release" manually or using a releasing wrapper like OBSCanvasAutoRelease I guess?

In our test case we do the latter, so we ensure that the reference count goes to -1 to deallocate the object, but the canvases created by OBSBasic::LoadData (Canvas::LoadCanvases) are not tracked because Canvas wraps a non-owning raw pointer instead of a reference-counting RAII wrapper and is itself also not tracking references.

(For that reason OBS::Canvas cannot assume anything about the pointer it gets - it cannot assume that it is reference counted or not and so it should never be allowed to impact the reference count in its destructor).

So you are correct that the call to obs_canvas_release(canvas) in OBSBasic::RemoveCanvas leads to double-deallocation, but OBSBasic::ClearSceneData still needs to call it explicitly to do the manual "release" I mentioned above because every canvas created by Canvas::LoadCanvases uses the raw pointer and thus only has the "0th" reference provided by libobs.

So the call from OBSBasic::RemoveCanvas in my suggestion should be removed, but the one in OBSBasic::ClearSceneData needs to be retained (just like we call obs_canvas_release(data->main_canvas) to also release the "0th" reference of the main canvas). That "implicit uncounted reference held by libobs" tripped me up last time, so it's great that your comment got me to revisit that particular aspect again.

---

We definitely need to revisit the entire implementation as there are too many implicit conversions between raw pointers and wrapped objects and some objects are implicitly owners of a pointer without having any impact on reference counting. But that can happen after fixing the immediate issue.

I shall now write a bit of Rust or Swift code to get that madness out of my mind.. 🫠

[dsaedtler]

• OBSBasic (via OBSBasic::AddCanvas) is the initial owner (and creator) of the canvas object. Thus OBSBasic::RemoveCanvas needs to be the destructor of the same object. This requires an explicit call to obs_canvas_release to ensure that the implicit "0th" reference it created is released.

Otherwise that canvas object would just hang around in memory as nobody holds a reference to it anymore, but libobs reference counting system treats "0 references" as a valid state (and not the state at which destruction should occur). I haven't seen an obvious system in the app that would otherwise take care of deallocating any such "orphaned" objects.

Right but that already works correctly, reference counting was never the problem, just recursion resulting in the vector modified/iterated over while in an invalid state. You're adding another obs_canvas_release() call compared to what already exists, resulting in it being prematurely free'd when using the frontend API.

[RytoEX]

Per off-thread discussion with @PatTheMav , we'll merge this as-is and then attempt to address the other concerns more comprehensively at a later time.