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 on startup #18300

Merged
merged 2 commits into from
Jan 26, 2025
Merged

fix crash on startup #18300

merged 2 commits into from
Jan 26, 2025

Conversation

zisoft
Copy link
Collaborator

@zisoft zisoft commented Jan 26, 2025

In dt_iop_gui_cleanup_module() the mutex gui_lock is destroyed. The orientation module (src/iop/flip.c) has gui_data initialized with NULL, so the cleanup crashes here.

In `dt_iop_gui_cleanup_module()` the mutex `gui_lock` is destroyed.
The orientation module (`src/iop/flip.c`) has `gui_data` initialized
with NULL, so the cleanup crashes here.
@dterrahe
Copy link
Member

Alternatively, make sure the gui->lock is always initialised by moving

dt_pthread_mutex_init(&module->gui_lock, NULL);

from _iop_gui_alloc to dt_iop_gui_init.

That avoids depending on a coincidental relationship between gui_data and gui_lock.

@zisoft zisoft force-pushed the fix-startup-crash branch from 6e64ebc to ca37ed4 Compare January 26, 2025 20:59
@TurboGit TurboGit added this to the 5.2 milestone Jan 26, 2025
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes labels Jan 26, 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 merged commit 3616961 into darktable-org:master Jan 26, 2025
6 checks passed
@zisoft zisoft deleted the fix-startup-crash branch January 27, 2025 06:13
@zisoft
Copy link
Collaborator Author

zisoft commented Jan 27, 2025

@dterrahe: This does not clearly fix the problem.

Now I get an assertion failure in src/iop/levels.c on startup.

darktable/src/iop/levels.c

Lines 613 to 623 in 526cd08

void gui_init(dt_iop_module_t *self)
{
dt_iop_levels_gui_data_t *g = IOP_GUI_ALLOC(levels);
dt_iop_gui_enter_critical_section(self);
g->auto_levels[0] = DT_LEVELS_UNINIT;
g->auto_levels[1] = DT_LEVELS_UNINIT;
g->auto_levels[2] = DT_LEVELS_UNINIT;
g->hash = 0;
dt_iop_gui_leave_critical_section(self);

dt_iop_gui_enter_critical_section() tries to lock the mutex gui_lock which is now not initialized.

The initialization sequence checks for hidden modules, dt_iop_gui_init() is not called in all cases.

@zisoft
Copy link
Collaborator Author

zisoft commented Jan 27, 2025

Revert of ca37ed4 and using dd36220 works.

@TurboGit
Copy link
Member

Revert of ca37ed4 and using dd36220 works.

Ok, let's do that for now then. TIA.

@dterrahe
Copy link
Member

Fully understood that you don't want to spend time now to find the correct solution to the issue. Unfortunately, my setup seems to be more forgiving and does not throw an assertion on failed initialisation, so I'm not able to debug myself. What's your platform/compiler/profile; maybe I can replicate.

A few thoughts.

  • I did not pay too close attention to ca37ed4 yesterday (and assumed it wouldn't make a difference) but the mutex could be initialised before calling gui_init. But also...
  • during gui_init I believe no other threads can be accessing "shared" variables yet, so I wonder if dt_iop_gui_enter/leave_critical_section is really required here.
  • module->gui_init should not really be called directly, but always via dt_iop_gui_init. As far as I can tell, there's currently only one exception, in _create_deleted_modules which should be converted
      {
        ++darktable.gui->reset;
        module->gui_init(module);
        --darktable.gui->reset;
      }

replaced by

        dt_iop_gui_init(module);

Was that the call that caused your crash?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants