PopupMenu throws signal connection errors on release build

[worron]

Tested versions

4.2.stable 4.3 dev2

System information

Godot v4.2.1.stable - Manjaro Linux 23.1.3 - X11 - Vulkan (Forward+) - dedicated AMD Radeon RX 6700 XT (RADV NAVI22) () - Intel(R) Xeon(R) CPU E5-2697 v3 @ 2.60GHz (28 Threads)

Issue description

Project release exported for linux throws some errors when user interact with OptionButton and its PopupMenu.

Every time when opened popup menu closing:

ERROR: Attempt to disconnect a nonexistent connection from 'root:<Window#24729617100>'. Signal: 'focus_entered', callable: ''.
   at: _disconnect (core/object/object.cpp:1420)
ERROR: Attempt to disconnect a nonexistent connection from 'root:<Window#24729617100>'. Signal: 'tree_exited', callable: ''.
   at: _disconnect (core/object/object.cpp:1420)

When menu opening after been closed prevously (so every time excluding the very first opening):

ERROR: Signal 'focus_entered' is already connected to given callable '' in that object.
   at: connect (core/object/object.cpp:1358)
ERROR: Signal 'tree_exited' is already connected to given callable '' in that object.
   at: connect (core/object/object.cpp:1358)

This doesn't reproduces in editor. This doesn't reproduces for web export . This doesn't reproduces when project exported with debug option. This doesn't reproduces when project settnig display/window/subwindows/embed_subwindows disabled.

All popup menu features works normally as far I as can tell, but this error spam in logs is kinda annoying.

Steps to reproduce

1. Create new project, add OptionButton to scene.
2. Export project for linux with standard release template.
3. Launch compiled app and open/close button popup menu several times.

Minimal reproduction project (MRP)

Absolutely trivial project with OptionButton as one and only element in single scene. option_button_project.zip

[worron]

Ops, i was thinking binary exported for windows not affected, but my latest windows builds has the same bug. Possibly it reproduces a bit randomly, or I just messed up.

[ShiftDouble]

I'm using 4.2.1 stable and getting the same error message as the first on exported projects. Not sure about OptionButtons, but it's happened to me and other people testing the exported project upon hovering over a UI element with a tooltip, and then moving the mouse cursor away and closing the tooltip.

[worron]

Yes, can reproduce it with tooltip and seems it all the same bug. Except tooltip has problem on closing only, popup menu on both open and close event.

[kii-chan-reloaded]

I ran into this same issue on a project I am working on. After the better part of a day of unsuccessfully trying to work around it, I started digging into the engine's source code to find the issue. I ended up cloning the repo, making a few changes, and building Godot from source. My custom Godot binary did get rid of the errors listed above.

That said, at this time I do not plan to submit a PR with my changes. I am not a C++ developer, and I don't know for sure that my changes didn't introduce a new bug somewhere else. Furthermore, CONTRIBUTING.md suggests that any pull requests should include unit tests to cover the changes, which I can't really do.

I am sharing my changes in hopes that someone better than I am at C++ will be able to use it to get a head start on fixing this bug and closing the issue.

Anyway, I think the problem lies in scene/gui/popup.cpp - more specifically, in _initialize_visible_parents and _deinitialize_visible_parents. These functions don't check if the Callables are already connected before they call connect and disconnect respectively. So, if for some reason the initialization functions are being called twice, we would expect to see the errors listed above when display/window/subwindows/embed_subwindows is enabled, since none of this code runs otherwise. This scenario doesn't really explain all the behaviors that this bug exhibits (why it only happens on non-debug releases, why there are no errors when a Popup is first displayed, why it doesn't happen in the web builds), but I feel like it is at least part of the issue.

So, I changed these two functions in scene/gui/popup.cpp to be as follows:

void Popup::_initialize_visible_parents() {
    if (is_embedded()) {
        visible_parents.clear();

        Window *parent_window = this->get_parent_visible_window();
        while (parent_window) {
            visible_parents.push_back(parent_window);

            Callable focus_entered_callable = callable_mp(this, &Popup::_parent_focused);
            Callable tree_exited_callable = callable_mp(this, &Popup::_deinitialize_visible_parents);

            if (!parent_window->is_connected(SceneStringName(focus_entered), focus_entered_callable)) {
                parent_window->connect(SceneStringName(focus_entered), focus_entered_callable);
            }
            if (!parent_window->is_connected(SceneStringName(tree_exited), tree_exited_callable)) {
                parent_window->connect(SceneStringName(tree_exited), tree_exited_callable);
            }

            parent_window = parent_window->get_parent_visible_window();
        }
    }
}

void Popup::_deinitialize_visible_parents() {
    if (is_embedded()) {
        for (Window *parent_window : visible_parents) {
            Callable focus_entered_callable = callable_mp(this, &Popup::_parent_focused);
                        Callable tree_exited_callable = callable_mp(this, &Popup::_deinitialize_visible_parents);

                        if (parent_window->is_connected(SceneStringName(focus_entered), focus_entered_callable)) {
                                parent_window->disconnect(SceneStringName(focus_entered), focus_entered_callable);
                        }
                        if (parent_window->is_connected(SceneStringName(tree_exited), tree_exited_callable)) {
                                parent_window->disconnect(SceneStringName(tree_exited), tree_exited_callable);
                        }
        }

        visible_parents.clear();
    }
}

As I mentioned above, compiling from the current main commit (4e5ed0b) with these changes made the errors go away in my project. I did not notice any adverse effects in how my project ran that are not already present in the current 4.3beta build (meaning, it did exit with an error about a memory leak, but that also happens when running my project on the "official" 4.3beta builds, so ¯\_(ツ)_\/¯ ).

I hope this helps get this bug fixed in the final 4.3 release.