Fix and regression test for crash during node group updates #104696

Lukas Tönne · 2023-02-13T14:19:39+01:00

Lukas Tönne commented

2023-02-13 14:19:39 +01:00

Node tree updates can crash if the tree contains a node group that points at an
"undefined" tree type. This can happen if the tree is linked from a library
and the path is lost, or if a custom (python) tree is used and the script is not
run.

The fix is to check if the node group type is valid ("registered") and return an empty list otherwise.

A regression test has been added to test updates with undefined group trees in future.

Node tree updates can crash if the tree contains a node group that points at an "undefined" tree type. This can happen if the tree is linked from a library and the path is lost, or if a custom (python) tree is used and the script is not run. The fix is to check if the node group type is valid ("registered") and return an empty list otherwise. A regression test has been added to test updates with undefined group trees in future.

👍 1

Brecht Van Lommel added this to the Nodes & Physics project 2023-02-13 19:00:26 +01:00

Lukas Tönne requested review from Jacques Lucke 2023-03-07 10:46:42 +01:00

Jacques Lucke requested changes 2023-03-07 10:58:57 +01:00

source/blender/blenkernel/intern/node_test.cc

						
				@ -0,0 +27,4 @@

				namespace blender::bke::node::tests {

				struct NodeTreeTestContext {

Jacques Lucke commented

2023-03-07 10:58:44 +01:00

Did you use some other tests as reference here? How to decide what goes into NodeTreeTestContext and what into NodeTreeTest?

Generally, I think the fix can be committed first without this test, but it's interesting to see how we could test this. Wouldn't mind adding this test separately, but I'd want to check if this is really initializing all the things that would have to be initialized.

Did you use some other tests as reference here? How to decide what goes into `NodeTreeTestContext` and what into `NodeTreeTest`? Generally, I think the fix can be committed first without this test, but it's interesting to see how we could test this. Wouldn't mind adding this test separately, but I'd want to check if this is really initializing all the things that would have to be initialized.

Lukas Tönne commented

2023-03-08 11:30:18 +01:00

Yes, i copied most of this from other tests, e.g. blend_file_loading_base_test.cc, lib_id_test.cc.

I only used the necessary parts for making node trees ID blocks and updating them. All the stuff in SetUpTestSuite/TearDownTestSuite is for one-off initialization that can stay over multiple tests. The "context" is temporary stuff (make new Main and a context) that gets thrown away after each test. Could do that in the virtual SetUp/TearDown for each fixture, but i like the explicitness of the "context". No strong opinion.

Yes, i copied most of this from other tests, e.g. `blend_file_loading_base_test.cc`, `lib_id_test.cc`. I only used the necessary parts for making node trees ID blocks and updating them. All the stuff in `SetUpTestSuite`/`TearDownTestSuite` is for one-off initialization that can stay over multiple tests. The "context" is temporary stuff (make new `Main` and a context) that gets thrown away after each test. Could do that in the virtual `SetUp`/`TearDown` for each fixture, but i like the explicitness of the "context". No strong opinion.

Lukas Tönne commented

2023-03-08 12:11:17 +01:00

Here's a for just the fix, into the release branch: #105564

source/blender/blenkernel/intern/node_tree_anonymous_attributes.cc

						
				@ -23,6 +23,11 @@ static const aal::RelationsInNode &get_relations_in_node(const bNode &node, Reso

				{

				  if (node.is_group()) {

				    if (const bNodeTree *group = reinterpret_cast<const bNodeTree *>(node.id)) {

				      /* Undefined tree types have no relations */

Jacques Lucke commented

2023-02-13 14:50:13 +01:00

Comment style, missing dot. Same in other places.

LukasTonne marked this conversation as resolved

source/blender/blenkernel/intern/node_tree_anonymous_attributes.cc

						
				@ -24,2 +24,4 @@

				  if (node.is_group()) {

				    if (const bNodeTree *group = reinterpret_cast<const bNodeTree *>(node.id)) {

				      /* Undefined tree types have no relations */

				      if (!ntreeIsRegistered(const_cast<bNodeTree *>(group))) {

Jacques Lucke commented

2023-03-07 10:55:56 +01:00

Better change ntreeIsRegistered to take a const node tree.

Better change `ntreeIsRegistered` to take a `const` node tree.

Iliya Katushenock commented

2023-03-07 12:21:53 +01:00

This has already been done, so you can simply remove the const cast

LukasTonne marked this conversation as resolved

Lukas Tönne force-pushed nodes-undefined-group-tree-update-crash from b83c2a2548 to 6677d69fbc

2023-03-08 11:58:31 +01:00

Compare

Jacques Lucke commented

2023-04-07 11:55:39 +02:00

I kind of don't like the fact that adding node tests requires another Blender entry point that needs to call BKE_idtype_init and the like... This does not feel like a scalable solution. Comments like this (in BlendfileLoadingBaseTest::SetUpTestCase) also show that this approach can be quite brittle and can lead to a fair amount of code duplication:

Minimal code to make loading a blendfile and constructing a depsgraph not crash, copied from main() in creator.c.

I'd be more happy with a solution that just starts up Blender as usual and then runs some test code. This is the approach used by Python tests and by extension also geometry nodes tests.

Duplicating the initialization and teardown logic once for tests (besides main and WM_exit_ex) might be ok, but more than that seems to be like too much duplication for me...

I kind of don't like the fact that adding node tests requires another Blender entry point that needs to call `BKE_idtype_init` and the like... This does not feel like a scalable solution. Comments like this (in `BlendfileLoadingBaseTest::SetUpTestCase`) also show that this approach can be quite brittle and can lead to a fair amount of code duplication: > Minimal code to make loading a blendfile and constructing a depsgraph not crash, copied from main() in creator.c. I'd be more happy with a solution that just starts up Blender as usual and then runs some test code. This is the approach used by Python tests and by extension also geometry nodes tests. Duplicating the initialization and teardown logic once for tests (besides `main` and `WM_exit_ex`) might be ok, but more than that seems to be like too much duplication for me...

Lukas Tönne closed this pull request

2024-04-19 14:18:47 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix and regression test for crash during node group updates #104696

Pull request closed