Fix T83592: Crash when deleting or reloading linked scene.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Jan 8 2021, 1:25 PM.

Details

Reviewers

Bastien Montagne (mont29)
Campbell Barton (campbellbarton)

Maniphest Tasks

T83592: Crash on linked scene reload while this scene is active

Commits

rBdff51d418a64: Fix: window manager can reference scenes and workspaces

Summary

This is my first attempt in fixing T83592. There might be a better solution, but at least this shows where the problem is.

There are two issues:

BKE_library_id_can_use_idtype reported that the window manager cannot point to any IDs. This is wrong, because the window manager contains windows, which point to scene data blocks.
Remapping a scene that is active in some window to null, results in a window without a scene, which no (almost no?) code seems to handle.

Fixing the BKE_library_id_can_use_idtype part of this is quite straight forward.

Fixing the remapping is a bit more tricky. I implemented the active-scene-update in a post processing step of id remapping, because I don't know a better place. Suggestions are welcome.
I also found that we can still get to zero scenes when first deleting the scene in the current blend file, and deleting the linked library (which also contains a scene) afterwards.

The code to update the active scene also seems a bit brittle, because it just updates the pointer instead of calling WM_window_set_active_scene.

Diff Detail

Repository: rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Jan 8 2021, 1:25 PM

Jacques Lucke (JacquesLucke) created this revision.

Harbormaster completed remote builds in B12044: Diff 32541.Jan 8 2021, 1:25 PM

Jacques Lucke (JacquesLucke) added a task: T83592: Crash on linked scene reload while this scene is active.Jan 8 2021, 1:26 PM

Jacques Lucke (JacquesLucke) retitled this revision from Fix T83592: crash when deleting or reloading linked scene to Fix T83592: Crash when deleting or reloading linked scene..Jan 8 2021, 1:33 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Jan 8 2021, 1:36 PM

Jacques Lucke (JacquesLucke) added reviewers: Bastien Montagne (mont29), Campbell Barton (campbellbarton).

I also found that we can still get to zero scenes when first deleting the scene in the current blend file, and deleting the linked library (which also contains a scene) afterwards.

Think that instead of forbidding the deletion of the last scene, we should forbid the deletion of the last local scene? Only allowing to have linked scenes in a .blend file makes very little sense to me anyway?

Fixing the remapping is a bit more tricky. I implemented the active-scene-update in a post processing step of id remapping, because I don't know a better place. Suggestions are welcome.

Can't think of any better way to do that, this is same pain as the obdata pointer and such, nothing else to do really.

The code to update the active scene also seems a bit brittle, because it just updates the pointer instead of calling WM_window_set_active_scene.

Indeed... Though if it work for now I would already be happy. Please add a comment about that in libblock_remap_data_postprocess_scene_relink.

And I think WM_window_set_active_scene is mostly about updating other pointers, the depsgraph, etc., which should be properly handled by general remapping code too?

out of curiosity, did you check if that was working fine with several windows open at the same time?

source/blender/blenkernel/intern/lib_query.c
420	Looks like windows can also link to workspaces...
source/blender/blenkernel/intern/lib_remap.c
354–356 ↗	(On Diff #32541)	Maybe enforce `new_window_scene` to be local (i.e. explicitly ignoring linked scenes in that loop)? Would be by default anyway if there is a local one, due to ordering of IDs in their main lists.

This revision now requires changes to proceed.Jan 8 2021, 3:34 PM

implement feedback

I made a patch that ensures that there is at least one *local* scene: D10049.

Unfortunately, it seems like skipping WM_window_set_active_scene does not
work when there are multiple windows...

It crashes because deg_graph->scene_cow == null in DEG_get_evaluated_scene.

_BLI_assert_abort() (/home/jacques/blender-git/blender/source/blender/blenlib/intern/BLI_assert.c:50)
DEG_get_evaluated_scene(const Depsgraph * graph) (/home/jacques/blender-git/blender/source/blender/depsgraph/intern/depsgraph_query.cc:162)
wm_event_do_handlers(bContext * C) (/home/jacques/blender-git/blender/source/blender/windowmanager/intern/wm_event_system.c:3218)
WM_main(bContext * C) (/home/jacques/blender-git/blender/source/blender/windowmanager/intern/wm.c:635)
main(int argc, const char ** argv) (/home/jacques/blender-git/blender/source/creator/creator.c:522)

Harbormaster completed remote builds in B12054: Diff 32551.Jan 8 2021, 4:26 PM

simplify fix

Harbormaster completed remote builds in B12441: Diff 33253.Jan 28 2021, 4:00 PM

I removed lots of stuff from the patch that is not strictly necessary to solve the bug.

So far, I couldn't find a good way to make remapping a scene to null work in the general case.
Deleting scenes still works though, because the active scene is updated at a higher level.

That change sounds good to me in any case.
I was never able to reproduce the crash, so cannot confirm it fixes it.

This revision is now accepted and ready to land.Feb 4 2021, 11:27 AM

Closed by commit rBdff51d418a64: Fix: window manager can reference scenes and workspaces (authored by Jacques Lucke (JacquesLucke)). · Explain WhyFeb 4 2021, 11:31 AM

This revision was automatically updated to reflect the committed changes.

Jacques Lucke (JacquesLucke) added a commit: rBdff51d418a64: Fix: window manager can reference scenes and workspaces.