Race Condition in EmbeddedProcess::_try_embed_process

[0x0ACB]

Tested versions

4.4-dev7

System information

Godot v4.4.dev (f2d9746) - Windows 11 (build 26100) - Multi-window, 2 monitors - Direct3D 12 (Forward+) - dedicated NVIDIA GeForce RTX 4080 (NVIDIA; 32.0.15.6614) - Intel(R) Core(TM) i9-14900K (32 threads)

Issue description

There seems to be a race condition somewhere when embedding the game. When running things normally the Game tab shows nothing around 80% of the time. Resizing and none of the buttons in the game tab change anything. If I put a break point on

godot/editor/plugins/embedded_process.cpp

Line 189 in bdf625b

Error err = DisplayServer::get_singleton()->embed_process(window->get_window_id(), current_process_id, get_screen_embedded_window_rect(), is_visible, is_visible && application_has_focus && embedding_grab_focus);

and wait a bit the window gets correctly shown all the time. One other thing I noticed was that if I put godot on my second monitor and launched the game it would show the game on my primary monitor where it should be drawn when it would have been embedded in the editor when the embedding "fails".

I wasn't able to narrow this down further yet but the error branches are never entered even if the Game tab does not display anything.

Steps to reproduce

• enable "Embed Game on Next Play"
• hit play

Minimal reproduction project (MRP)

[Hilderin]

@0x0ACB thanks for testing and reporting this issue.
Unfortunately, I'm not able to reproduce on Windows 11 with 2 monitors.
Is "Make Game Window Floating" enabled? I guess not, just want to be sure.
Did you try with an empty project?
Are you in single window mode?

The fact that the window appears on the wrong monitor is troubing. It seems that the embedding control is not able to get the correct embedded control global position. Can you check the values returned by get_screen_embedded_window_rect()? Are the coords correct?

[0x0ACB]

OK so I had a bit more time to do some more digging and I also remembered a similar issue I had in the past when using godot as an overaly. The y coordinate for the window is calculated incorrectly. My two monitors are stacked vertically not horizontally and it seems that godot has trouble calculating the correct global position.

When embedding is successful with godot running on the bottom monitor or godot is running on my top monitor and the embedding fails. The global position of the game window ends up as: (1870, 1603). When godot is running on the bottom monitor and embedding fails the global position of the game is: (1767, 3031).

"Make Game Window Floating"

I had it disabled for my latest tests but the not showing up on monitor 1 issue does exist whether it is enabled or not. With it enabled the floating window will appear on the bottom monitor though if the godot editor is on the top.

Did you try with an empty project?

I can reproduce the issue with an empty project as well yes.

Are you in single window mode?

no

Can you check the values returned by get_screen_embedded_window_rect()? Are the coords correct?

The coords do seem to be correct however the actual position of the game window does not match the result of get_screen_embedded_window_rect(). The call to SetPosition in DisplayServerWindows::embed_process also applies the same coordinates to the window whether embedding succeeds or fails. What I did notices was that even though SetPosition never returns 0/FALSE the window position of the game does not update when embedding failed. I also noticed that when embedding fails the game is not closed when closing the editor.

I can offer you the code and workarounds I am using for my Overlay class I wrote for godot which has fixes for the issue of appearing on the wrong monitor or not correctly being overlayed on top of the target window maybe it helps. Of particular note is the need to hide the window before applying the styles and then showing the window again since otherwise the calls to SetWindowLong(PtrW) might not take effect immediately. Also I noticed that you are using SetParent to set the parent window. This has some issues when compared to SetWindowLongPtrW(m_engineWnd, GWLP_HWNDPARENT, std::bit_cast<LONG_PTR>(m_targetWindow)); in my experience because it attaches the two input queues for the windows together when they run on different threads.

void overlay::OverlayWIN32::match_size() const
{
    print_verbose("Matching window");
    RECT rect;
    ERR_FAIL_COND(GetClientRect(m_targetWindow, &rect) == FALSE);
    MapWindowPoints(m_targetWindow, nullptr, reinterpret_cast<POINT *>(&rect), 2);

    const HMONITOR hMonitor = MonitorFromWindow(m_targetWindow, MONITOR_DEFAULTTONEAREST);
    MONITORINFO mi = { sizeof(mi) };
    GetMonitorInfoW(hMonitor, &mi);

    const Rect2i screen_rect(mi.rcMonitor.left, mi.rcMonitor.top, mi.rcMonitor.right - mi.rcMonitor.left, mi.rcMonitor.bottom - mi.rcMonitor.top);
    const Rect2i window_rect(rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top);
    print_verbose("Screen rect: " + screen_rect + " Window rect: " + window_rect);

    if(window_rect.position == screen_rect.position && window_rect.size == screen_rect.size)
    {
        // prevent fullscreen
        print_verbose("Preventing fullscreen");
        rect.left += 1;
        rect.top += 1;
        rect.right -= 1;
        rect.bottom -= 1;
    }
    ERR_FAIL_COND(SetWindowPos(m_engineWnd, nullptr, rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, SWP_NOZORDER | SWP_NOACTIVATE | SWP_ASYNCWINDOWPOS) == FALSE);
}


void overlay::OverlayWIN32::attach()
{
    ERR_FAIL_COND(m_attached);
    m_attached = true;

    const auto display = DisplayServer::get_singleton();
    m_engineWnd = std::bit_cast<HWND>(display->window_get_native_handle(DisplayServer::HandleType::WINDOW_HANDLE));

    ShowWindow(m_engineWnd, SW_HIDE);

    bool hasWindow = false;
    if(m_targetWindow)
    {
        DWORD pid = 0;
        if (GetWindowThreadProcessId(m_targetWindow, &pid) != 0)
        {
            if (pid == client::get_instance()->get_process_id())
            {
                hasWindow = true;
            }
        }
    }

    if(hasWindow)
    {
        print_line("Reattaching to existing window");
        MessageQueue::get_singleton()->push_callable(callable_mp(this, &OverlayWIN32::deferred_attach));
        return;
    }

    std::thread([this]()
    {
        print_line("Waiting for window");
        std::optional<ipc::actions::window_created> windowCreated;
        while (!windowCreated)
        {
            windowCreated = ipc::shared_memory::get_instance()->receive_action<ipc::actions::window_created>();
            std::this_thread::sleep_for(std::chrono::milliseconds(100));
        }
        m_targetWindow = std::bit_cast<HWND>(windowCreated->hwnd);
        MessageQueue::get_singleton()->push_callable(callable_mp(this, &OverlayWIN32::deferred_attach));
    }).detach();
}


void overlay::OverlayWIN32::deferred_attach()
{
    print_line("Attaching");

    const auto display = DisplayServer::get_singleton();
    m_engineWnd = std::bit_cast<HWND>(display->window_get_native_handle(DisplayServer::HandleType::WINDOW_HANDLE));

    // disable all kinds of input buffering
    Input::get_singleton()->set_use_accumulated_input(false);
    Input::get_singleton()->set_agile_input_event_flushing(false);

    // Attach to the window
    display->window_set_flag(DisplayServer::WINDOW_FLAG_BORDERLESS, true);
    display->window_set_flag(DisplayServer::WINDOW_FLAG_TRANSPARENT, true);
    SceneTree::get_singleton()->get_root()->set_transparent_background(true);

    auto exstyle = GetWindowLongW(m_engineWnd, GWL_EXSTYLE);
    exstyle = exstyle & ~WS_EX_APPWINDOW & ~WS_EX_TOOLWINDOW;
    exstyle = exstyle | WS_EX_LAYERED | WS_EX_TRANSPARENT | WS_EX_NOACTIVATE;
    SetWindowLongW(m_engineWnd, GWL_EXSTYLE, exstyle);
    SetWindowLongPtrW(m_engineWnd, GWLP_HWNDPARENT, std::bit_cast<LONG_PTR>(m_targetWindow));

    match_size();

    // create input handler window
    static const char* cls_name = "overlay_message";
    UnregisterClassA(cls_name, nullptr);
    WNDCLASSEX wx = { 0 };
    wx.cbSize = sizeof(WNDCLASSEX);
    wx.lpfnWndProc = &OverlayWIN32::StaticMessageWndProc;
    wx.hInstance = nullptr;
    wx.lpszClassName = cls_name;
    CRASH_COND(!RegisterClassExA(&wx));
    m_messageWnd = CreateWindowExA(0, wx.lpszClassName, cls_name, 0, 0, 0, 0, 0, HWND_MESSAGE, nullptr, nullptr, nullptr);
    CRASH_COND(!m_messageWnd);

    ipc::write::window_info windowInfo;
    windowInfo.godot_hwnd = std::bit_cast<uint64_t>(m_engineWnd);
    windowInfo.message_hwnd = std::bit_cast<uint64_t>(m_messageWnd);
    windowInfo.want_input = false;
    display->cursor_set_shape(DisplayServer::CURSOR_BUSY);
    display->cursor_set_shape(DisplayServer::CURSOR_ARROW);
    windowInfo.cursor = reinterpret_cast<uint64_t>(GetCursor());
    ipc::shared_memory::get_instance()->set_window_info(windowInfo);

    ShowWindow(m_engineWnd, SW_SHOW);
    SetActiveWindow(m_engineWnd);

    m_wantInput = true;
    print_line("Attached");
    emit_signal(SNAME("attached"));
}

[Hilderin]

Thanks a lot for that, I'll take take a closer look to your answer and try to fix the issue!

[Hilderin]

Thanks again for time, the code sample and your explanations.

I'm not able to reproduce with 2 monitors stack vertically on my side despite all my different trials and tests.

From what I understand, because the issue is intermittent, one of my hypothesis is that _find_window_from_process_id sometime returns the wrong window handle and that's probablly why the embedded window does not move when calling SetWindowPos.

I created a draft PR why added debug code. Can you try the build from this PR: https://github.com/godotengine/godot/actions/runs/12613127398?pr=101135 and run an empty project, resize the game area and send me the output log please.

I added some debug code to see if the found embedded handle is the correct one by printing the window title and I added the coords calculated and the screen origins. I also modify some parameters to the call to SetWindowPos, I did not know the existance of SWP_ASYNCWINDOWPOS.
You should see something like that:

Note: the parent window handle is passed to the new godot process in command line via the --wid argument and it's used when calling CreateWindowExW in DisplayServerWindows::_create_window. The call to SetParent in DisplayServerWindows::embed_process should never be used. It was only a fallback from my first implementation. I removed it in the PR. In reality DisplayServerWindows::embed_process does not really embed the process but only update it's position and visibility.

[0x0ACB]

The call to SetParent in DisplayServerWindows::embed_process should never be used.

Interesting. It does get called when the embedding fails for me.

15:59:14.875105 [info] embed_process: process not found.
15:59:15.004331 [info] embed_process: process not found.
15:59:15.014370 [info] embed_process: process not found.
15:59:15.093295 [info] embedded window found: no title
15:59:15.095504 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:15.271311 [info] embedded window found: no title
15:59:15.273311 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.077662 [info] embedded window found: no title
15:59:27.079662 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.084175 [info] embedded window found: no title
15:59:27.085176 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.090175 [info] embedded window found: no title
15:59:27.091173 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.096686 [info] embedded window found: no title
15:59:27.097686 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.108197 [info] embedded window found: no title
15:59:27.110201 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.125770 [info] embedded window found: no title
15:59:27.126771 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.142279 [info] embedded window found: no title
15:59:27.143782 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)
15:59:27.158295 [info] embedded window found: no title
15:59:27.159296 [info] embed_process - rect: (1957, 1591), (1149, 644) screen origin: (0, -1440)

those are the debug logs from your PR.

I notice that in your enum proc func you are never actually setting the last error to anything other than success and I think you are using it wrong:

If the function succeeds, the return value is nonzero.

If the function fails, the return value is zero. To get extended error information, call GetLastError.

If EnumWindowsProc returns zero, the return value is also zero. In this case, the callback function should call SetLastError to obtain a meaningful error code to be returned to the caller of EnumWindows.

Your enum callback returns 0/FALSE on success when it should return that on error.

I added some debug prints to the enum windows part and I think that is where the issue is. When embedding fails the editor get the wrong window handle.

There is more than 1 window that has the Editor as its parent. The issue seems to be that I am starting godot with the console wrapper:

16:22:29.845005 [info] PseudoConsoleWindow
16:22:29.967756 [info] Engine

I added a crude filter on the window class and that works.

WCHAR class_name[256];
if (!RealGetWindowClassW(hWnd, class_name, 256)) {
	return TRUE;
}
print_line(String(class_name));
if (String(class_name) != String("Engine")) {
	return TRUE;
}

Also as a side note. Calling CreateWindow with a parent has the same issue SetParent has and can/will cause a lot of input lag since the message queues for the windows will be processed in sync.

[Hilderin]

Great finding!

The conditions in _enum_proc_find_window_from_process_id_callback were problematic and could select a window whose parent was not the searched parent_hWnd. I modified the condition, and if I'm correct, we don't need an additional condition to handle the embedded window.

Can you test again with my modifications, please? Here's the link: https://github.com/godotengine/godot/actions/runs/12621205716
Much appreciated!

Regarding the use of SetWindowLongPtrW(m_engineWnd, GWLP_HWNDPARENT, ...);, I tested it, and it works. However, I did not notice any difference in terms of locking. I experienced the input lag issue you mentioned when I was developing the embedding implementation. If I remember correctly, the real issue was related to the WS_CHILD style. When this style was enabled, the embedded window lagged significantly. I'm not an expert on the subject, and my tests might have been flawed.

I'm hesitant to use GWLP_HWNDPARENT because the Microsoft documentation explicitly advises against it:
https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setwindowlongptra

[0x0ACB]

The doc advises against it because it does not set the parent. If you call SetWindowLongPtr with GWLP_HWNDPARENT it sets the owner of the window not the parent which has some subtle differences. Can't find it right now but there is an article on msdn that explains the differences and that a top level window should always have its owner set not it's parent.

Edit: I tested your PR and it does seem to work now. Didn't even notice the wrong nesting there.

[Hilderin]

I tested your PR and it does seem to work now. Didn't even notice the wrong nesting there.

Thanks again for testing!

The doc advises against it because it does not set the parent. If you call SetWindowLongPtr with GWLP_HWNDPARENT it sets the owner of the window not the parent which has some subtle differences. Can't find it right now but there is an article on msdn that explains the differences and that a top level window should always have its owner set not it's parent.

You are absolutely right, this article explains what you are saying: https://devblogs.microsoft.com/oldnewthing/20100315-00/?p=14613 and based on it, I think setting using CreateWindow without the WS_CHILD does the same thing as using SetWindowLongPtr with GWLP_HWNDPARENT.

[0x0ACB]

Interesting. Probably I forgot about that special case because I didn't want to modify the engine and do it from a module.