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

Don't add new ref to the System instance which is being destroyed (fix aseprite/aseprite#4811) #115

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

dacap
Copy link
Member

@dacap dacap commented Nov 28, 2024

Regression introduced in 3bea531, when os::instance() was refactored to os::System::instance() to return a SystemRef instead of a System*, and ~WindowWin() destructor asks for the current system() (and WindowWin::system() is trying to create a new System reference, but the System is being destroyed as it was completely unreferred, i.e. ref=0.)

@dacap dacap assigned dacap and martincapello and unassigned dacap Nov 28, 2024
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

SystemRef System::instance()
{
ASSERT(!g_is_being_destroyed);
Copy link
Collaborator

Choose a reason for hiding this comment

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

warning: no header providing "ASSERT" is directly included [misc-include-cleaner]

os/common/system.cpp:7:

- #ifdef HAVE_CONFIG_H
+ #include "base/debug.h"
+ #ifdef HAVE_CONFIG_H

@@ -66,6 +66,9 @@

namespace os {

// Extern raw reference to current system instance.
extern System* g_instance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

warning: no header providing "os::System" is directly included [misc-include-cleaner]

os/win/window.cpp:7:

- #ifdef HAVE_CONFIG_H
+ #include "os/system.h"
+ #ifdef HAVE_CONFIG_H

@aseprite-bot
Copy link
Collaborator

Hi there!

One or more of the commit messages in this PR do not match our code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@dacap
Copy link
Member Author

dacap commented Dec 2, 2024

Just in case, this can be tested with the new view_benchmark.exe benchmark: https://github.com/aseprite/aseprite/blob/beta/src/ui/view_benchmark.cpp (the benchmark crashes at the end without this patch).

@dacap
Copy link
Member Author

dacap commented Dec 2, 2024

My bad, the bug is reproducible with the editor_benchmark: https://github.com/aseprite/aseprite/blob/beta/src/app/editor_benchmark.cpp

@dacap dacap assigned dacap and unassigned martincapello Dec 5, 2024
@dacap
Copy link
Member Author

dacap commented Dec 6, 2024

After some @martincapello testing, other issues were found but we weren't able to reproduce the bug again. I'll merge this PR as it is as at least is a real mitigation of Sentry crash reports from aseprite/aseprite#4811.

…prite#4811)

Regression introduced in 3bea531,
when os::instance() was refactored to os::System::instance() to return
a SystemRef instead of a System*, and ~WindowWin() destructor asks for
the current system() (and WindowWin::system() is trying to create a
new System reference, but the System is being destroyed as it was
completely unreferred, i.e. ref=0.)
@aseprite-bot
Copy link
Collaborator

Hi there!

One or more of the commit messages in this PR do not match our code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@dacap dacap merged commit b78acc4 into aseprite:beta Dec 6, 2024
10 of 11 checks passed
@dacap dacap deleted the fix-clear-events branch December 6, 2024 02:46
@dacap dacap linked an issue Dec 6, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash clearing laf-os event queue recursively (?)
3 participants