Page MenuHome

Fix T83411: Crash when using a workspace/layout data path in a driver
ClosedPublic

Authored by Sergey Sharybin (sergey) on Jan 11 2021, 3:54 PM.

Details

Summary

Building IDs which are not covered by copy-on-write process was not
implemented, which was causing parameters block not present, and, hence
causing crashes in areas which expected parameters to present.

First part of this change is related on making it so Copy-on-Write is
optional for ID nodes in the dependency graph.

Second part is related on using a generic builder for all ID types
which were not covered by Copy-on-Write before.

The final part is related on making it so build_id() is properly
handling ParticleSettings and Grease Pencil Data. Before they were not
covered there at all, and they need special handling because they do
have own build functions.

Not sure it worth trying to split those parts, as they are related to
each other and are not really possible to be tested standalone. Open
for a second opinion though.

Possible nut-tightening is to re-organize build_id() function so
that every branch does return and have an assert at the end, so that
missing ID type in the switch statement is easier to spot even when
using compilers which do not report missing switch cases.

As for question "why not use default" the answer is: to make it more
explicit and clear what is a decision when adding new ID types. We do
not want to quietly fall-back to a non-copy-on-write case for a newly
added ID types.

Since this is not a newly introduced issue, is safer to commit this
patch after 2.92 is branched.

Diff Detail

Repository
rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Jan 11 2021, 3:54 PM
Sergey Sharybin (sergey) created this revision.

Non-UI datablocks are not supposed to be able to reference UI datablocks really. It would not surprise me if that crashes in e.g. undo or loading a .blend file without UI.

Still, as long as we don't explicitly prevent that this seems fine.

This revision is now accepted and ready to land.Jan 11 2021, 4:13 PM

LGTM. I had one remark about the build_generic_id() function name, but it's only minor and if you want to commit without addressing that that's fine by me too. Either way it's an accept.

source/blender/depsgraph/intern/builder/deg_builder_nodes.h
155

To me "generic" doesn't really convey "this ID type doesn't use CoW". The generic nature does become clear when the CoW'ed ID types all have their own build function, and the CoW-less ones all use the same generic one, but that did require me to dive into the build_id() function first.

I think it would be enough to just add a oneliner comment above this declaration, something like:

/* Build function for ID types that need neither their own build_xxx() function nor copy-on-write. */
source/blender/depsgraph/intern/depsgraph.cc
148–150

Nice note 👍

One thing I just realized: the generic, CoW-less datablocks are still shown with CoW component in the depsgraph debug image:

This is the file I used to test, which is the example from T83411 but with an actually valid driver reading workspaces['Layout'].name:

source/blender/depsgraph/intern/builder/deg_builder_nodes.h
155

Thing is, is not really about coverage or not with CoW. The datablock can be CoW-ed, but still need to be build using build_generic_id.

I like the idea of comment /* Build function for ID types that do not need their own build_xxx() function. */.

Do you have suggestion for the name?

source/blender/depsgraph/intern/builder/deg_builder_nodes.h
155

In that case I think the name is fine, especially with that comment in place.

Added comment to the build_generic_id().

One thing I just realized: the generic, CoW-less datablocks are still shown with CoW component in the depsgraph debug image:

@Sybren A. Stüvel (sybren), this is a very nice observation! Makes me happy! :)

This is, unbelievably, expected. The CoW operation will do nothing. It can be done less confusing by applying P1885, but I felt that such change better be done separately.

What are your thoughts here?

P.S. I originally had the P1885 "merged" into this patch, but then notices crashes in particles and was simplifying code to understand what is causing it. Forgot to restore the P1885 in the patch, and forgot to mention the process.. In fact, there is still part of the P1885 left in the depsgraph_id_tag_copy_on_write() (code need to become aware of optional nature of CoW operation). Hope this little history explanation makes sense :)

Makes total sense :)

I think it's good to keep the changes separated as two commits. This one is for fixing a problem, and the other one is for optimizing (removing a no-op node).

I've talked to Brecht. Seems like it's safe enough fix to do for 2.92. Will commit shortly.