Prevent invalid rich text icon resource element in editor log from infinitely recursing

[PierceLBrooks]

I observed the problem that this solution addresses occasionally while attaching Android Studio's debugger to the editor app process on my device. Without these changes, the error handler for the editor log UI will sometimes recurse infinitely due to the validity check inside of the call for adding an icon image into a rich text node.

• Bugsquad edit, closes: #85976

[AThousandShips]

Can you please open an issue report for this, or link to one if one exists, to help track the issue (so it doesn't get lost if this PR is closed for some reason, or a different solution is required)

[akien-mga]

The change looks fine, but indeed it would be good to understand why this happens. These should be icons from the editor theme so unless a custom editor theme is used with broken icons, I expect it should work ok.

[PierceLBrooks]

@AThousandShips I have filed an issue here... https://github.com/godotengine/godot/issues/85976

[YuriSizov]

So as others said, while the fix is not bad in and of itself, it seems to be hiding the real issue. I don't have a setup to test Android builds as suggested in the issue, so I can only offer a guess. And my guess would be that something tries to log a message way too early, before the editor log is available. Or, perhaps, in a context where it is not properly initialized.

In any case, I would not merge this PR right now, not until we understand what's calling the editor log at a point in time where it's not ready to display its content correctly. cc @m4gr3d maybe?

[PierceLBrooks]

I suggest the introduction of a "log audit" preprocessor guard around these new conditional tokens such that whenever in the future there is an opportunity to track down the root problematic log message call as you've recommended @YuriSizov , its misbehavior is not hidden when the build system is provided with an appropriate flag.

[akien-mga]

I don't have the setup to easily reproduce this as it seems to be a fairly convoluted scenario, but indeed as suggested above the issue is likely that some errors are emitted too early in that scenario (and those errors should likely be fixed separately).

But it's a sign that the error handler relies on having its theme initialized, when it hasn't been yet until it enters the tree.

So maybe we should defer registering it as error handler to NOTIFICATION_ENTER_TREE?

diff --git a/editor/editor_log.cpp b/editor/editor_log.cpp
index c0c26bbfeb..cc69b164d3 100644
--- a/editor/editor_log.cpp
+++ b/editor/editor_log.cpp
@@ -132,6 +132,13 @@ void EditorLog::_notification(int p_what) {
 		case NOTIFICATION_ENTER_TREE: {
 			_update_theme();
 			_load_state();
+
+			// Register as error handler after theme init, to ensure we can show colors/icons.
+			add_error_handler(&eh);
+		} break;
+
+		case NOTIFICATION_EXIT_TREE: {
+			remove_error_handler(&eh);
 		} break;
 
 		case NOTIFICATION_THEME_CHANGED: {
@@ -506,15 +513,10 @@ EditorLog::EditorLog() {
 
 	eh.errfunc = _error_handler;
 	eh.userdata = this;
-	add_error_handler(&eh);
 
 	current = Thread::get_caller_id();
 }
 
-void EditorLog::deinit() {
-	remove_error_handler(&eh);
-}
-
 EditorLog::~EditorLog() {
 	for (const KeyValue<MessageType, LogFilter *> &E : type_filter_map) {
 		// MSG_TYPE_STD_RICH is connected to the std_filter button, so we do this
diff --git a/editor/editor_log.h b/editor/editor_log.h
index 03a0a071c6..a513bdf925 100644
--- a/editor/editor_log.h
+++ b/editor/editor_log.h
@@ -187,7 +187,6 @@ public:
 	void add_message(const String &p_msg, MessageType p_type = MSG_TYPE_STD);
 	void set_tool_button(Button *p_tool_button);
 	void register_undo_redo(UndoRedo *p_undo_redo);
-	void deinit();
 
 	void clear();
 
diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp
index 827e2c7bf6..f6e2f1db90 100644
--- a/editor/editor_node.cpp
+++ b/editor/editor_node.cpp
@@ -689,7 +689,6 @@ void EditorNode::_notification(int p_what) {
 			}
 			editor_data.save_editor_external_data();
 			FileAccess::set_file_close_fail_notify_callback(nullptr);
-			log->deinit(); // Do not get messages anymore.
 			editor_data.clear_edited_scenes();
 		} break;
 

Not sure about the implications on multiple registration, so I added a deinit call in EXIT_TREE, which likely made the external one in EditorNode redundant, unless it really needs to happen at this time (but I think EditorLog would leave the tree before EditorNode so it's probably best that it cleans after itself like this?).

---

Edit: Well I tested my diff, it seems to break printing errors and warnings in the EditorLog :(

So deferring it like this doesn't seem to be a good option, at least not without further work. I assume it makes registering the error handler too late and somehow others then take precedence?

[akien-mga]

@AThousandShips I wonder if you may not have fixed the infinite recursion issue when printing errors from the error handlers? If so this change would not be needed anymore, the bug is likely still present but should just print one error to stderr.

[AThousandShips]

Can't test myself but it should have fixed it unless there's something else going on here

[PierceLBrooks]

Should we really be so confident that this other fix will remain solvent for the issue at hand and not simply later down the road erode enough for there to be a need for these two easy conditional guards to be reintroduced anyways? This is quite a nuanced problem, and if this pull request gets closed then it'll be harder for whomever ends up dealing with reoccurrences to rediscover the existing solution, particularly for those who have not dug into the bowels of the Android core for the engine and are just trying to debug other things agnostically. Why not at least also integrate the Occam's Razor case approach that I've offered?

[AThousandShips]

Should we really be so confident that this other fix will remain solvent for the issue at hand and not simply later down the road erode enough for there to be a need for these two easy conditional guards to be reintroduced anyways?

Yes if it's a matter of recursion of errors that does fix it, it won't "erode", it's a stable fix, assuming it does solve this

[akien-mga]

Why not at least also integrate the Occam's Razor case approach that I've offered?

Basically our philosophy to be able to maintain such a big codebase is to avoid hiding issues, but make sure we can either fix them fully when discovered, or keep them clearly visible (e.g. with ERR_FAIL_COND(...) types of errors) to encourage fixing them later.

Adding these if cases here will make it fail silently and we won't be aware that there may be a deeper problem in the codebase that may manifest itself in other areas. Then we're playing whack-a-mole with a core design issue, especially if we can't use something like ERR_BREAK(icon.is_null()); to communicate in the logs that a bug was encountered - as this would also recurse and cause the same issue.

That being said, in this case since this is just a cosmetic feature, I think it wouldn't hurt to also add what this PR suggests. But it should apply to all uses of icon, not just the one that currently can print an error. Who knows if tool_button->set_icon(icon); might change in the future and no longer accept invalid icons as valid arguments?

So it should be:

if (icon.is_valid()) {
	log->add_image(icon);
	log->add_text(" ");
	tool_button->set_icon(icon);
}

Or, maybe better:

if (icon.is_null()) {
	fprintf(stderr, "Couldn't retrieve theme icons for editor log error/warning messages.\n");
	break;
}
log->add_image(icon);
log->add_text(" ");
tool_button->set_icon(icon);

This makes sure we're still aware of the issue if it happens, but prevents the recursion issue by not using the logging API that tries to push messages to the EditorLog.