Skip to content

[3.x] Fix index being out of sync in Node::remove_child #77483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
+40 −0
Open

[3.x] Fix index being out of sync in Node::remove_child #77483

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions scene/main/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,6 +1346,46 @@ void Node::remove_child(Node *p_child) {
remove_child_notify(p_child);
p_child->notification(NOTIFICATION_UNPARENTED);

// Note that calling _set_tree() could have caused
// another node to be deleted from the same parent,
// the childlist could be different, and thus
// idx by this point is not guaranteed to be correct.
// We therefore recalculate idx here, for safety, if it has changed.

// Child count could have changed.
child_count = data.children.size();

if ((p_child->data.pos != idx) || (idx >= child_count) || (data.children[idx] != p_child)) {
Copy link
Member Author

@lawnjelly lawnjelly Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write a comment here, but just to make it clear for review, there are three possibilities here (for detecting a change to p_child's index):

  • The child itself knows that its index has changed
  • The previous index is now outside the range of the new child count
  • The child of the parent given by the index is no longer p_child (this can be checked providing idx < child_count in the previous)

(The first could possibly be omitted, but its cheap to check)

idx = -1;

// Reget the childen, the list may have changed...
children = data.children.ptrw();

// If the new p_child->data.pos is correct...
if (p_child->data.pos >= 0 && p_child->data.pos < child_count) {
if (children[p_child->data.pos] == p_child) {
idx = p_child->data.pos;
}
}

// If data.pos is incorrect, it still may be one of the children...
if (idx == -1) {
for (int i = 0; i < child_count; i++) {
if (children[i] == p_child) {
idx = i;
break;
}
}
}

// This should not happen unless something has gone horribly wrong,
// but just in case prevent a crash.
// By this point, the child will have been partially removed,
// so normal behaviour cannot be guaranteed
// if this ERR FAIL occurs.
ERR_FAIL_COND_MSG(idx == -1, vformat("Cannot remove child node '%s' as it is no longer a child of this node.", p_child->get_name()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe in this context? I might be misunderstanding this change, it seems that if we mess this up we can't really be sure that p_child is still pointing to what we think it is?

Copy link
Member Author

@lawnjelly lawnjelly May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p_child is still pointing to the correct child, the issue is that the idx of the child (i.e. position in the childlist) can change during the pre-deletion, which is the mechanism of the bug in the issue.

The ERR_FAIL here should not happen unless something really bad is going on, like p_child is being removed as a child during the pre-delete. This is very likely not possible, but this ERR_FAIL is more a fail safe to prevent a crash in this scenario, and indicate where the problem is occurring. As this if clause should be pretty rare anyway, there's no cost to adding an ERR_FAIL here.

Copy link
Member Author

@lawnjelly lawnjelly May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah there is a potential bug here though, I forgot to reget child_count as it could have changed. Will fix. 👍

DONE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reiterate, p_child will still point to the same child in the same area of memory, as children are non-relocatable - it is only the index that may change.

}

data.children.remove(idx);

//update pointer and size
Expand Down