Skip to content

masks: don't let mask manager rows hand out stale module pointers - #22055

Open
da-phil wants to merge 1 commit into
darktable-org:masterfrom
da-phil:pl/prevent_mask_manager_from_handing_out_stale_module_pointers
Open

da-phil wants to merge 1 commit into
darktable-org:masterfrom
da-phil:pl/prevent_mask_manager_from_handing_out_stale_module_pointers

Conversation

@da-phil

@da-phil da-phil commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

masks: don't let mask manager rows hand out stale module pointers

Tree rows cache raw dt_iop_module_t pointers but outlive the modules,
which are freed on instance delete, image change and darkroom leave.
Selecting a group row then called flags() through a dangling pointer,
and deleting a shape crashed in dt_iop_is_hidden().

  • _lib_masks_get_values(): only return a module still present in
    dev->iop; NULL attributes the edit to the mask manager
  • _forms_structure_hash(): include the modules and their mask ids so a
    changed module set forces a rebuild
  • invalidate the cached hash on image change and darkroom entry: the
    allocator may hand back the freed addresses and hash identical
  • gui_update(): drop a pending selection module that died before the
    deferred update ran
  • _tree_delete_shape(): stop clobbering the row's module with the one of
    the previous row

Speculatively fixes #22050 #21852

Disclaimer: this change was co-created with Claude.

@da-phil
da-phil force-pushed the pl/prevent_mask_manager_from_handing_out_stale_module_pointers branch from 48974d6 to 1c65474 Compare August 29, 2026 19:49
@ralfbrown ralfbrown added bugfix pull request fixing a bug scope: UI user interface and interactions labels Aug 29, 2026
@TurboGit

Copy link
Copy Markdown
Member

Since mask manager will be removed in 5.8 and replaced by the new flexi masking maybe this is just not necessary.

@da-phil

da-phil commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

I guess it also depends if that gets into the way of the flexi masking development, like constantly producing merge conflicts. If not, I don't see why we should not make sure existing bugs are fixed until the module can be fully replaced by a new one. What do you think about that?

@da-phil
da-phil force-pushed the pl/prevent_mask_manager_from_handing_out_stale_module_pointers branch from 1c65474 to 4b70837 Compare September 1, 2026 22:40
Tree rows cache raw dt_iop_module_t pointers but outlive the modules,
which are freed on instance delete, image change and darkroom leave.
Selecting a group row then called flags() through a dangling pointer,
and deleting a shape crashed in dt_iop_is_hidden().

- _lib_masks_get_values(): only return a module still present in
  dev->iop; NULL attributes the edit to the mask manager
- _forms_structure_hash(): include the modules and their mask ids so a
  changed module set forces a rebuild
- invalidate the cached hash on image change and darkroom entry: the
  allocator may hand back the freed addresses and hash identical
- gui_update(): drop a pending selection module that died before the
  deferred update ran
- _tree_delete_shape(): stop clobbering the row's module with the one of
  the previous row
@da-phil
da-phil force-pushed the pl/prevent_mask_manager_from_handing_out_stale_module_pointers branch from 4b70837 to b4b1e63 Compare September 1, 2026 22:42
@da-phil
da-phil marked this pull request as ready for review September 2, 2026 07:58
@da-phil

da-phil commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Because the fix in #21861 was going into a very similar direction, I merged the changes into this PR.

@masterpiga do these changes collide with your work on flexi masking?
I thought it would be still nice to fix outstanding issues with the mask manager for folks who use the master branch as their daily driver such as myself. I'm rarely switching back to a release, only if master isn't usable at all for actual work.

@masterpiga

masterpiga commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@da-phil thanks for checking, no worries. I am keeping my branch closely synced with master, and of course if there is something broken in master it should be fixed 👍

That said, I hope to start the iter for merging flexi sooner than later, so these changes may not be very long lived.

@da-phil

da-phil commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@da-phil thanks for checking, no worries. I am keeping my branch closely synced with master, and of course if there is something broken in master it should be fixed 👍

That said, I hope to start the iter for merging flexi sooner than later, so these changes may not be very long lived.

Sounds good, thanks for the fast reply 🙏
@TurboGit then I'd leave it to @hats-np to check if those changes fix the crashes he experiences while using the manager.

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 scope: UI user interface and interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash when deleting mask in mask manager

4 participants