Prevent crashes when removing Viewport from scene tree in event handler

[RedworkDE]

Fixes #77752

These checks were removed by #77452, only the is_inside_tree is strictly necessary to fix the crash, I re-added the other two just to be on the safe side, I don't think any other the checks in push_unhandled_input can have changed since push_input has checked them.

If during the processing of an event, some event handler removes the viewport associated with the event from the scene tree without marking the event as handled, when the event processing continues it will try to access the tree and crash. This makes this an error instead of an crash.

[KoBeWi]

CC @Sauermann

[Sauermann]

A few lines above, the is_inside_tree check is already performed:

godot/scene/main/viewport.cpp

Line 2956 in 621d68e

ERR_FAIL_COND(!is_inside_tree());

So it looks more like a problem that an event is not set as handled.

[YuriSizov]

In fact, all three of these checks are performed before that line. Unless they can mutate in the process, this doesn't seem correct.

[RedworkDE]

Unless they can mutate in the process

That is exactly the problem here, as the event handlers can have arbitrary effects, including those that violate preconditions of further handling of events.

This PR tries to prevent the further handling by re-checking the preconditions, while #77765 tries to be more specific and stops further processing in just this case by marking the event as handled after hiding the dialog.

[Sauermann]

Unless they can mutate in the process

If an event leads to the mutation of these states, then the event was processed and should be set to as handled.

This PR tries to prevent the further handling by re-checking the preconditions

If that should be the way, then it would be prudent to perform the inside_tree-check (and the other checks) between all input event processing steps.

Currently it is only checked at the beginning of event processing.
With this PR, checking additionally also happens before shortcut input event processing.
The logical conclusion would be to:

• additionally check before gui input processing
• additionally check before unhandled input processing
• additionally check before unhandled key input processing
• additionally check before physics picking

[RedworkDE]

This PR now tries to prevent crashes like #77752 in general by rechecking if the viewport is still in the tree every time when attempting to access the tree after executing arbitrary event handlers. This should stop things from crashing if any other (user) code makes an error like this as well.

#77765 then fixes AcceptDialog specifically and prevents it from triggering these errors.

[akien-mga]

Seems ok, but I wonder if those should be DEV_ASSERTs instead of ERR_FAIL_COND. I wonder what impact it would have on performance when used at runtime, for something which is aimed at catching and prevent engine bugs IIUC.

[RedworkDE]

If these trigger it will crash anyways, so there is no point in making these DEV_ASSERTs and crashing in the assert. I could maybe wrap them in #ifdef DEBUG_ENABLED, but personally I would either add this safety net for all build or just leave it as-is and just close the PR entirely.

[KoBeWi]

ERR_FAIL makes sense, because the crash can be triggered by user code, e.g.

func _input(event: InputEvent) -> void:
	get_viewport().get_parent().remove_child(get_viewport())

is_inside_tree() is inlined bool check, so the performance impact is super minimal.

[Sauermann]

ERR_FAIL makes sense, because the crash can be triggered by user code, e.g.

Godot doesn't catch all possible crashes, that can be caused by user code. In this case the crash can be prevented by the user simply by accepting the event with the following change:

func _input(event: InputEvent) -> void:
	get_viewport().get_parent().remove_child(get_viewport())
	get_viewport().set_input_as_handled()

[YuriSizov]

Godot doesn't catch all possible crashes, that can be caused by user code.

Well, we try to when presented with a repro case. If you end up with this code from KoBeWi and experience the crash, there is no way to know that you can fix it as you describe. So that shouldn't happen.

[YuriSizov]

Thanks!