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

Fix crash when scrolling in smooth mode on empty thumbtable #18359

Conversation

victoryforce
Copy link
Collaborator

Fixes #18357.

…able

There are different ways to check if the thumbtable is empty (for example,
whether the thumbnail list is NULL). But we check whether the thumbnail size
is zero as this is exactly what we are interested in, because later we would
have to divide by this value, so we protect ourselves from division by zero.
@victoryforce victoryforce added this to the 5.0.1 milestone Feb 4, 2025
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Thanks!

@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: UI user interface and interactions labels Feb 4, 2025
@TurboGit TurboGit merged commit 93fc2ec into darktable-org:master Feb 4, 2025
6 checks passed
@TurboGit
Copy link
Member

TurboGit commented Feb 4, 2025

Merged in master & 5.0.x branch.

@victoryforce victoryforce deleted the smooth-scrolling-crash-on-empty-thumbtable branch February 4, 2025 19:13
@victoryforce
Copy link
Collaborator Author

Regarding the release notes. An entry in the release notes should have been added if this crash was in version 5.0.0 and we fixed it. But it's unlikely that such an obvious problem could have been missed when testing the smooth scrolling PR. So I checked if 5.0.0 crashed and... no, it didn't. Running it under the debugger showed that in 5.0.0 when entering _event_scroll_compressed the thumbnail size (table->thumb_size) was non-zero even on an empty thumbtable.

So something changed between the release and a fairly recent development snapshot.

@TurboGit @jenshannoschwalm Does this change ring a bell to you? It seems that the zero as the thumbnail size on an empty thumbtable is a side effect of some recent change in the darktable codebase. Was this expected?

@jenshannoschwalm
Copy link
Collaborator

Unfortunately no.

@TurboGit
Copy link
Member

TurboGit commented Feb 6, 2025

No, no idea except maybe a change of Gtk+ version?

@victoryforce
Copy link
Collaborator Author

No, no idea except maybe a change of Gtk+ version?

I highly doubt that changing the library version could have such an effect. Everything is possible, but... My guess is that this is a side effect of some change in darktable code.

I'll bisect and find the culprit commit.

@victoryforce
Copy link
Collaborator Author

It seems that the zero as the thumbnail size on an empty thumbtable is a side effect of some recent change in the darktable codebase.

Yes, it was a side effect of the following commit:

c17601182375760792219d1a1b253fbad7ba8b9f is the first bad commit
commit c17601182375760792219d1a1b253fbad7ba8b9f
Author: Ralf Brown <[email protected]>
Date:   Sat Jan 18 14:09:25 2025 -0500

    rework splash screen to avoid keep-above

 src/common/darktable.c | 29 +++++++++++++++--------------
 src/control/control.c  |  8 ++++----
 src/dtgtk/expander.c   |  8 +++++---
 src/gui/gtk.c          | 56 +++-----------------------------------------------------
 src/gui/splash.c       | 17 +----------------
 src/gui/splash.h       |  2 --
 src/views/lighttable.c |  9 +++++----
 src/views/map.c        |  7 ++-----
 8 files changed, 35 insertions(+), 101 deletions(-)

It looks like we somehow lost a call to some initialization code that was responsible for calculating thumb_size with these changes. @ralfbrown Any ideas?

I don't think this is a serious regression, because although we ended up with a division-by-zero crash, it was actually a problem with the code that was dividing without proper safety checks. Rather, it is worth noting a certain architectural complexityy here if unrelated changes can have such unpredictable consequences.

@ralfbrown
Copy link
Collaborator

Lots of initializations are triggered by the initial realization of darktable's main window, which now happens much later.... Someone early in dt's development got overly clever with having GUI callbacks initiate things rather than simply making a direct call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash in lighttable when continuously scrolling mouse
4 participants