Skip to content
New issue

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

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

Already on GitHub? # to your account

Application freezes after returning from Lock screen #680

Open
2 tasks done
usagirei opened this issue Jul 10, 2024 · 6 comments
Open
2 tasks done

Application freezes after returning from Lock screen #680

usagirei opened this issue Jul 10, 2024 · 6 comments
Labels

Comments

@usagirei
Copy link

Requirements

  • This issue doesn't already exist
  • This bug is Not related to compatability with a specific game

Summary

Borderless Gaming will freeze when you lock the computer screen, possibly in other scenarios that fit the same criteria.

Steps to reproduce

  1. Run Application
  2. Go to lock screen windows (Win+L), and wait a few moments (10~15 seconds should do it, if not lock it again and retry)
  3. Come back to frozen application

Technical details

  1. GetForegroundWindow may return NULL under certain conditions, one of which, is when the lock screen is active.

  2. Then GetWindowThreadProcessId will return NULL too (Which happens to be a valid PID, the System Idle process) ,

  3. Then when it tries to create a ProcessDetails class, for an invalid process, which tries to get the window title for an invalid window.

    var handle = Native.GetForegroundWindow();
    Native.GetWindowThreadProcessId(handle, out uint processId);
    var details = new ProcessDetails(Process.GetProcessById((int)processId), handle);

  4. It will then hang indefinitely trying to do so here:

    if (!Native.IsWindow(_windowHandle))
    {
    _windowHandle = Native.GetMainWindowForProcess(Proc).GetAwaiter().GetResult();
    }


Checks for NULL/Invalid handles need to be introduced:

An early return here:

var handle = Native.GetForegroundWindow();

var handle = Native.GetForegroundWindow(); 
if(handle == IntPtr.Zero) 
    return;

And maybe a short circuit here:

if (!Native.IsWindow(_windowHandle))
{
_windowHandle = Native.GetMainWindowForProcess(Proc).GetAwaiter().GetResult();
}

if ((_windowHandle != IntPtr.Zero) && !Native.IsWindow(_windowHandle))
{
    _windowHandle = Native.GetMainWindowForProcess(Proc).GetAwaiter().GetResult();
}

Or ensure a ProcessDetails class can't be created for an invalid process/window

version

9.5.6.1328 - git 3cc4dc6

@usagirei usagirei added the bug label Jul 10, 2024
@snaphat
Copy link

snaphat commented Jan 15, 2025

Thanks for those fixes OP. This application is a bit of a stability nightmare it seems.

The application has basic safety checks missing for various native windows APIs. An additional couple are found as follows...

  • BorderlessGaming\Logic\Windows\Native.cs
EnumWindows(Del, 0);
EnumWindows(Del, 1);

// should be
if (EnumWindows(Del, 0) == 0)
    return;
if (EnumWindows(Del, 1) == 0)
    return;
  • BorderlessGaming\Logic\Windows\Windows.cs
Native.EnumWindows(Del, 0);
Native.EnumWindows(Del, 1);

// should be
if (Native.EnumWindows(Del, 0) == 0)
    return;
if (Native.EnumWindows(Del, 1) == 0)
    return;

The application can also call SendMessage(hWnd, WM_GETTEXT, ...) from the windows process handler (WNDPROC) on windows owned by the application and then deadlock itself because the windows process handler is what handles WM_GETTEXT.

This seems to be done internally via calls to _watcher.Refresh() in MainWindow.cs and I believe elsewhere as well:

Task.WaitAll(_watcher.Refresh());

// this should also just be the following:
_watcher.Refresh().Wait();

Possible locations to alleviate said deadlocks include the following...

  • BorderlessGaming\Logic\Windows\Native.cs::QueryProcessesWithWindows
        // Add import here
        [DllImport("kernel32.dll")]
        public static extern uint GetCurrentProcessId();

        ...

        public static void QueryProcessesWithWindows(Action<ProcessDetails> callback, List<IntPtr> windowPtrSet)
        {
            ... 
            foreach (var ptr in ptrList)
            {
                if (GetWindowRect(ptr, out Rect rect))
                {
                    if (((Rectangle)rect).IsEmpty)
                    {
                        continue;
                    }
                    if (windowPtrSet.Contains(ptr))
                    {
                        continue;
                    }
                    uint processId;
                    GetWindowThreadProcessId(ptr, out processId);
                    var process = ProcessExtensions.GetProcessById((int)processId);
                    if (process == null)
                    {
                        continue;
                    }
                    if (processId == GetCurrentProcessId()) // here
                    {
                        continue;
                    }
                    callback(new ProcessDetails(process, ptr)
                    {
                        Manageable = true
                    });
                }
            }
        }
  • BorderlessGaming\Logic\Windows\ForegroundManager.cs::WinEventProc
        public static void WinEventProc(IntPtr hWinEventHook, uint eventType, IntPtr hwnd, int idObject, int idChild, uint dwEventThread, uint dwmsEventTime)
        {
            if (UserPreferences.Instance.Favorites is not null)
            {
                try
                {
                    var handle = Native.GetForegroundWindow();
                    if (handle == IntPtr.Zero)
                        return;
                    Native.GetWindowThreadProcessId(handle, out uint processId);
                    var process = ProcessExtensions.GetProcessById((int)processId);
                    if (process is null)
                    {
                        return;
                    }
                    if (processId == Native.GetCurrentProcessId()) // here
                    {
                        return;
                    }
...
  • BorderlessGaming\Logic\Windows\Native.cs::GetMainWindowForProcess_EnumWindows
        private static bool GetMainWindowForProcess_EnumWindows(List<IntPtr> ptrList, IntPtr hWndEnumerated,
            uint lParam)
        {
            GetWindowThreadProcessId(hWndEnumerated, out uint processId);
            if (processId == GetCurrentProcessId()) // here
            {
                return true;
            }

            // Later on in this method, it might also be prudent to check if ptrList already contains an hwnd before adding it to the list as well.
...
  • BorderlessGaming\Logic\Windows\Windows.cs::GetMainWindowForProcess_EnumWindows
        private static bool GetMainWindowForProcess_EnumWindows(List<IntPtr> ptrList, IntPtr hWndEnumerated,
            uint lParam)
        {
            Native.GetWindowThreadProcessId(hWndEnumerated, out uint processId);
            if (processId == Native.GetCurrentProcessId()) // here
            {
                return true;
            }

            // Later on in this method, it might also be prudent to check if ptrList already contains an hwnd before adding it to the list as well.
...
  • BorderlessGaming\Logic\Windows\Native.cs::GetWindowTitle
        public static string GetWindowTitle(IntPtr hWnd)
        {
            // Allocate correct string length first
            try
            {
                GetWindowThreadProcessId(hWnd, out uint processID);
                if (processID == GetCurrentProcessId()) // here
                    return "<error>";
                var length = (int)SendMessage(hWnd, WM_GETTEXTLENGTH, IntPtr.Zero, IntPtr.Zero);
                var sbWindowTitle = new StringBuilder(length + 1);
                SendMessage(hWnd, WM_GETTEXT, (IntPtr)sbWindowTitle.Capacity, sbWindowTitle);
                return sbWindowTitle.ToString();
            }
            catch (Exception)
            {
                return "<error>";
            }
        }

Harder to alleviate but the application will also hang if the hWnd provided via SendMessage() is for an external process if that process doesn't handle the WM_GETTEXT properly (e.g. is hanging itself, etc.).


Once the deadlocking is fixed there exist another bug that causes the window list to duplicate because the process list is completely cleared for some reason when Refresh is called so the application loses track of all windows, re-enumerates them, and readds them to some UI list box while deleting all the original ProcessDetails state. This results in both the defective duplicate listing and arguably the worse issue of the application losing track of windows it has already made borderless.

  • BorderlessGaming\Logic\Core\ProcessWatcher.cs
        public Task Refresh()
        {
            //Processes.Clear(); // this needs to be commented out
            return Task.Factory.StartNew(UpdateProcesses);
        }

That's all I've found so far. There may be more deadlocks/livelocks lurking. I'll have to see if it still hangs randomly. Seems to me that the entire code base needs an audit

@snaphat
Copy link

snaphat commented Jan 25, 2025

The above changes I did appear to actually fix all of the instability issues in the application

@andrewmd5
Copy link
Collaborator

The software was completely rewritten and the beta version of that rewrite is on Steam.

@snaphat
Copy link

snaphat commented Jan 25, 2025

I'm using the net8 branch. Based on the reviews on steam, net8 branch, and 10 tag (including steam specific changes), UI, etc. it would seem the application isn't completely rewritten. The steam version also appears to have the same stability issues.

I could be wrong but I'm going to assume the code base internals for 10 are basically identical to the net8 branch based on the above. If so, all of what the OP said and the changes I noted still apply and should largely (if not completely) fix stability issues in the application which are currently resulting in the majority of the negative reviews on steam.

It's also worth noting that if y'all don't intend on fixing repository version in any capacity please say so and modify the readme to indicate that the version on this repository is out of date, contains unfixed bugs, and will not be updated.

@andrewmd5
Copy link
Collaborator

andrewmd5 commented Jan 25, 2025 via email

@snaphat
Copy link

snaphat commented Jan 25, 2025

For others who happen by this post. I think the claim here is that ONLY the beta version of the steam application is a rewrite, only that beta version on steam contains a different code base, and that only it shares no code with this project.

On the other hand, it sounds like the beta version found in the GitHub project has nothing to do with the beta version on steam. I think the claim is that It's not the same application or code.

Confusingly, however, it appears that the stable version on steam IS the beta version hosted here with the stability issues discussed above.

Based on being directed to the other beta application on steam, It sounds like there may not be an intention to fix the issues in this application's code base? Would this be correct?

Moreover, would it be correct to say that the other beta application on steam is not open source?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants