Fix crash caused by stale owner

[Sauermann]

Adjust NOTIFICATION_PREDELETE in Node to clean up owned nodes.
Also print a warning, when the owner becomes invalid.
resolve #78736

Updated 2023-11-17: Rebased

[RandomShaper]

I see cases where this is needed, but I think there's still a case to fix: in the MRP in #78736, shouldn't the child_node.reparent(self) break the ownership of child_node by owner_node?

[Sauermann]

I have investigated this case and I was unable to create a crash by reparenting after the changes of this PR.

Still I believe, that there is a bug, that currently reparenting allows a Node to have an owner, that is not an ancestor (which contradicts the owner-description).
I was unable to finish investigating this bug yesterday and intend to look into this again.

[Sauermann]

I have updated the PR with an additional verification, that the owner is always an ancestor and added a warning about this case.

I have also considered as a more strict alternative in Node::add_child, but would consider it as too restrictive:

	ERR_FAIL_COND_MSG(p_child->data.owner && p_child->data.owner != this && !p_child->data.owner->is_ancestor_of(this), vformat("Can't add child '%s' to '%s', owner '%s' would no longer be an ancestor.", p_child->get_name(), get_name(), p_child->data.owner->get_name()));

I have also considered as a less strict alternative to adjust the documentation of Node.owner, but had the impression that this would find not enough attention and users might not easily find the reason, why their node's owner became null:

[b]Note:[/b] The owner of the [Node] is automatically erased, if the node is moved to a place in the scene tree where its owner is no longer an ancestor.

[RandomShaper]

I agree with that approach.

I'm wondering if it should be expected that a node still has owned nodes by the time the destructor is run. Shouldn't all the previous mechanics of the owner and/or owned nodes (pre-delete, exit tree notifications, etc., etc.) have cleaned things up?

[Sauermann]

One of the MRPs in the linked issue sets the owner of a Node, while that node is not in the tree. So tree-notifications don't help in these cases.
Also when handling this during tree-notifications, the owner would always be removed when a Node is relocated in the scene tree, which would be bad for usability.

I was not aware of pre-delete, thanks for bringing this to my attention.
During NOTIFICATION_PREDELETE currently only data.owner is cleared, but not data.owned.
I believe, that it would be conceptually better, if I move the data.owned-clean-up code from ~Node to NOTIFICATION_PREDELETE.

[YuriSizov]

Slightly relevant here: we now have a PR that changes how reparenting works #81506.
But I guess that wouldn't completely eliminate invalid cases we are trying to prevent here? I would err on the side of caution at this point and bump this to 4.3.

But an approval from @RandomShaper would still be appreciated so we can merge it as soon as the next dev cycle starts 🙂

[YuriSizov]

I don't think this scenario should ever be possible. If it's possible, guards are missing elsewhere. But if you want to be extra sure, maybe this should only happen in editor builds?

[RandomShaper]

I'd move this check to add_child() instead, which is anyway the only caller of _add_child_nocheck() (possibly, to a counterpart of _validate_child_name() named _validate_child_future_ownership(node, intended_new_parent)). That way, _add_child_nocheck() still honors its name (althought the no check part was about names, but now we're validating owners, it's good to extend that notion).

Aside, I think that with this we would be catching every possibility of a wrong owner. That said, given that this may have a performance impact, I see value in restricting the check to debug builds, and making it just a check. I mean, keep the owner (even if invalid), not to change behavior across debug and non-debug. After all, I don't think owner chain integrity is an internal requisite of the engine (engine won't crash or misbehave due to an inconsitency there; only the user may get unexpected results).

Finally, I'd suggest changing the wording so erasing/removing owner is expressed as unsetting the owner instead, for enhanced clarity.

[Sauermann]

I have moved the validation of the owner to add_child() within a DEBUG_ENABLED section.

Please note that _add_child_nocheck() is additionally called from SceneState::instantiate. But since this function deals with instantiating nodes, my guess would be, that it's unnecessary to add checks there.

I would prefer to completely prohibit inconsistent states (so that the user no longer gets unexpected results), but based on the performance-argument, I assume that a warning can be considered sufficient.

For good measure, I have also checked, that the MRP in the linked issue still crashes in v4.2.rc.custom_build [80de898] and gets fixed by this PR.

[YuriSizov]

Thanks!

[YuriSizov]

Cherry-picked for 4.2.2.