Widget style can be nullptr in ui::Theme::calcMinSize()

[sentry-io]

Sentry Issue: ASEPRITE-1CT

EXCEPTION_ACCESS_VIOLATION_READ / 0x6c: Fatal Error: EXCEPTION_ACCESS_VIOLATION_READ / 0x6c
  File "theme.cpp", line 689, in ui::Theme::calcMinSize
    sz.w = style->minSize().w;
  File "widget.cpp", line 206, in ui::Widget::setStyle
    m_minSize = m_theme->calcMinSize(this, style);
  File "grid.cpp", line 128, in ui::Grid::setStyle
    Widget::setStyle(style);
  File "button_set.cpp", line 205, in app::ButtonSet::{ctor}::__l2::<lambda>::operator()
    setStyle(SkinTheme::instance()->styles.buttonset());
  File "type_traits", line 1576, in std::invoke
    return static_cast<_Callable&&>(_Obj)();
...
(17 additional frame(s) were not displayed)

[martincapello]

I've been looking at this and I was able to reproduce it when this two conditions were met:

1. A custom theme is used and it is missing the "buttonset" style (it might be missing other styles introduced in v1.3.x as well).
2. The default Aseprite theme is missing the "buttonset" style as well (I had to manually delete it before running Aseprite for testing).

Hence, I'm not sure how this crash could have happened...the only thing I can imagine is some kind of mis-installation of the 1.3-rc6 version on top of the 1.2.40 (or some 1.2.x) version...like having 1.3-rc6 binaries configured to use the theme folders from a 1.2.40 installation...

[martincapello]

So, @dacap what do you think about warning the user that their Aseprite is pointing to a wrong default theme version?

[dacap]

From the Sentry breadcrumbs it looks like the theme that was being used was https://github.com/lettdev/aseprite-than (v1.2.5). So it looks like the extension wasn't overwriting the default theme, but I'm not quite sure. Probably you could give a try to that theme and check how it works.

About the styles, some code was added in 0ae8bc9 to copy all the missing styles: https://github.com/aseprite/aseprite/blob/84bd6b69f5fece6a0b3c6de143ab7459264a4a19/src/app/ui/skin/skin_theme.cpp#L120C22-L120C22 from the default theme. So the only way to have a missing style is if the default theme is broken as you said.

If you don't find an issue with the than theme I'll check this later.

[dacap]

what do you think about warning the user that their Aseprite is pointing to a wrong default theme version?

Not sure how to detect that the default theme is broken (how can we detect that the files are modified if we don't have the original one?)

[martincapello]

From the Sentry breadcrumbs it looks like the theme that was being used was https://github.com/lettdev/aseprite-than (v1.2.5). So it looks like the extension wasn't overwriting the default theme, but I'm not quite sure. Probably you could give a try to that theme and check how it works.

That's right, I have been testing with that theme, the same version.

About the styles, some code was added in 0ae8bc9 to copy all the missing styles: https://github.com/aseprite/aseprite/blob/84bd6b69f5fece6a0b3c6de143ab7459264a4a19/src/app/ui/skin/skin_theme.cpp#L120C22-L120C22 from the default theme. So the only way to have a missing style is if the default theme is broken as you said.

If you don't find an issue with the than theme I'll check this later.

If the default theme has the styles missing in the than theme (which the default theme has by default on any new installation), I wasn't able to find any issue. It was only when I removed a style from the default theme that was also missing in the than theme when I was able to reproduce this.

[martincapello]

what do you think about warning the user that their Aseprite is pointing to a wrong default theme version?

Not sure how to detect that the default theme is broken (how can we detect that the files are modified if we don't have the original one?)

Oh, I didn't mean to detect when the theme is broken...but now I realize that I was just thinking in something that doesn't exists actually...I was imagining that we had a version attribute (for the <theme> element) that we could use to check if the default theme is the right one for the build...this would only allow us to detect the the case where the user might have mis-installed Aseprite in some way.

But now that I think, we could introduce a new attribute "checksum" for the <theme> element, so we can detect if the theme was modified. Or maybe we could generate the checksum and have its value in some constant macro to use it when running Aseprite (which should calculate the checksum and compare it with the constant).