Fix crash on treeitem free

[BrotherShort]

Fix #107950

[bs-mwoerner]

Thanks for looking into this! I'm not sure this can be fixed in the engine, though. The crash in the issue is ultimately caused by the GDScript code freeing the node that p_selected points to while the tree is being iterated. So checking

ERR_FAIL_INDEX(p_col, p_selected->cells.size());

still accesses undefined memory via the now invalid p_selected. This may come back with a value that fails the check, or with a value that passes it, or it may cause an access violation/segmentation fault trying to read from that location.

I suppose if we wanted to add engine-side support to catch this, we could set a flag at the start of the iteration and throw an error if ~TreeItem() is invoked while its tree is being iterated.

[KoBeWi]

The crash happens in this loop:

godot/scene/gui/tree.cpp

Lines 2984 to 2989 in 9b22b41

	while (c) {
	if (c->is_visible()) {
	select_single_item(p_selected, c, p_col, p_prev, r_in_range, p_current->is_collapsed() || p_force_deselect);
	}
	c = c->next;
	}

For some reason, it won't stop iterating when a TreeItem is selected, even though checking other items is pointless.

[bs-mwoerner]

The idea may be to iterate the entire tree to handle both selections and deselections (which may have to happen after the newly selected item).

The crash is caused by a (user-defined) signal handler freeing the c pointer if c == p_selected during the loop. That's not a major problem if c was the last item because then c->next may still hold a null pointer and the loop simply ends.

If c wasn't the last item or its memory has been overwritten since being freed, c->next will read some non-zero value and the loop continues, passing the invalid pointer in the next call to select_single_item via p_selected. This will then likely crash when doing p_selected->cells.write[p_col], because _cowdata.size() is likely to return zero when called on a recently destructed instance, making any p_col value fail the index check.

[KoBeWi]

Then ideally freeing TreeItems should be blocked inside that loop, but that might be not possible. Which leaves the current solution the only feasible one I suppose.

[bs-mwoerner]

I mean, TreeItem already has Tree as a friend class as well as a reference to the tree it's in, so I suppose it wouldn't be unreasonable to have some magic interaction between the two that produces an error when trying to delete an item while its tree is iterating. That might make the problem materialize at the location of the actual bug instead of some time later in the engine code. As I understand it, you can call cancel_free() during a predelete notification to actually prevent an object from being freed and thus reliably prevent the crash.

p_selected->cells.write[p_col] internally does the same check as the PR but uses CRASH_BAD_INDEX instead of ERR_FAIL_INDEX, so we're turning the crash into an error, but only if the freed memory is still intact. If the signal handler has made some additional allocations that overwrite the freed item, then c->next is just some random memory value and it'll crash one way or another anyway.

[KoBeWi]

As I understand it, you can call cancel_free() during a predelete notification to actually prevent an object from being freed and thus reliably prevent the crash.

Ah yeah, this might work.