Fix T89196: Depsgraph use-after-free after scene switching undo
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Jun 21 2021, 2:30 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Jacques Lucke (JacquesLucke)

Commits

rB956c539e597a: Fix T89196: Depsgraph use-after-free after scene switching undo

Summary

Delay depsgraph visibility update tagging until it is known that
graph relations are up to date, and until it is known that the graph
is actually needed to be evaluated.

Diff Detail

Repository: rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Jun 21 2021, 2:30 PM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B15333: Diff 38560.Jun 21 2021, 2:30 PM

Sergey Sharybin (sergey) added reviewers: Brecht Van Lommel (brecht), Jacques Lucke (JacquesLucke).Jun 21 2021, 2:45 PM

Brecht Van Lommel (brecht) accepted this revision.Jun 21 2021, 4:19 PM

This revision is now accepted and ready to land.Jun 21 2021, 4:19 PM

LGTM.
Honestly, I don't know what the visibility update is doing exactly yet. Can't see anything that is obviously wrong here though and the bug is fixed as well.

source/blender/depsgraph/DEG_depsgraph.h
116	The `do_time` input is actually always false, so it probably could be removed (or can become a separate `tag` function). Anyway, that doesn't matter too much now.
source/blender/depsgraph/intern/depsgraph.h
111	typo (`by`). Also, there is no function named `tag_on_visible_update`. Generally, I'm not a big fan having comments say where variables are set. These comments can go out of date very easily and if the variable name isn't too generic, it's generally easy to find where it is set anyway.
source/blender/windowmanager/intern/wm_event_system.c
383	Isn't this what you did in this patch? Can you remove it now?

Addressed comments from Jacques.

I did leave the do_time argument, even though it is unused.
The only place where it is used is on file load. Maybe we need to do some special tag then. But to me it seems logical to keelp "do something for all depsgraph" and "do something for the given depsgraph" in sync from the arguments point of view.