Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid crashes when engine leaks canvas items and friends #85520

Conversation

YuriSizov
Copy link
Contributor

Closes #85495. Code is gently stolen from the Vulkan renderer, which does a similar thing.

Now you'll see this with the MRP on exit:

servers\rendering\renderer_canvas_cull.cpp:2172 - 28 RIDs of type "CanvasItem" were leaked.
WARNING: 28 RIDs of type "CanvasItem" were leaked.
     at: RendererCanvasCull::_free_rids (servers\rendering\renderer_canvas_cull.cpp:2172)

Plus a lot of other leaks.


What happens, basically, is that when there is a leak we are left with some data in the RendererCanvasCull class. When we try to delete it, it attempts to delete all its memory, including RendererCanvasRender::Items it has in store. These contain commands which contain polygons. When you delete polygons, we need to access the RendererCanvasRender singleton. But this singleton is already null at this point (it is removed when the rendering server is finalized, whereas RendererCanvasCull is only removed when the rendering server is deleted).

So we need to do these things earlier and with more grace, and report to the user that they have a leak.

@YuriSizov YuriSizov added bug enhancement topic:rendering crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 29, 2023
@YuriSizov YuriSizov added this to the 4.3 milestone Nov 29, 2023
@YuriSizov YuriSizov requested a review from a team as a code owner November 29, 2023 19:43
@YuriSizov YuriSizov force-pushed the rendering-gracefully-leak-canvas-items branch from 99852f4 to ab10206 Compare November 29, 2023 20:03
@YuriSizov
Copy link
Contributor Author

I also noticed some outdated code that was out of place in relevant files, so I did a little cleanup. Seems like it dates back to 2787ad6#diff-2eaa5fdddb0bc4674976c8bbaeb5aab4c69d5a3c74fb1ead3a00f9e9c06a4f26, but after some additional changes the singleton definition should be moved to the class' own file.

Thanks @clayjohn for looking into it 🙃

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall.
Waiting for rendering team review to merge.

@YuriSizov YuriSizov force-pushed the rendering-gracefully-leak-canvas-items branch from ab10206 to 34ecfff Compare December 20, 2023 12:19
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, it makes perfect sense to add this check. It's helped us lots of times on the 3D side.

WARN_PRINT(vformat("%d RIDs of type \"%s\" were leaked.", owned.size(), p_type));
}
for (const RID &E : owned) {
free(E);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this consistantly on the 3D side but if you search you'll see a few places I added it, but seeing that it can be very difficult to find the cause of the leak simply by a count, I check if a description was recorded for the RID (this is a debug/tools build only feature), than we print the description of the resource being leaked.

This also means off course that we need to get more consistent in recording descriptions when the resource is allocated. Something we can improve on over time.

@akien-mga akien-mga merged commit 673102f into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the rendering-gracefully-leak-canvas-items branch January 25, 2024 15:56
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor Author

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot crashes on shutdown after interacting with a GraphEdit editor plugin.
3 participants