Implement Explorer.exe monitoring feature. #945

ghost · 2024-11-11T14:28:52Z

Description:
Implement Explorer.exe monitoring feature using TaskbarCreated event.

What it does?
If we detect that Explorer.exe has restarted we will call ReopenTaskbars.

Why?
This will ensure that when Explorer is restarted, RetroBar is also re-initialized.
This will fix stuff like wrong desktop size etc that would happen if we did not re-initialize RetroBar.

Tests performed:
Lots and lots of testing, on Windows 11 systems.

What issues this should fix?
@dremin PR cairoshell/ManagedShell#113 fixed all of these:
#942
#938
#927
#648
#476
#457
so this PR is just a follow up now, this will fix some bugs like the desktop is not resized properly after Explorer is restarted etc.

ghost · 2024-11-11T14:31:46Z

Btw:
The diff looks wierd (fancy GitHub feature or user error? lol),
but the only things that were added\changed were ExplorerMonitorStart() and RestartRetroBar().
There is also new variables for these new functions (on top of the file).

xoascf · 2024-11-11T19:29:06Z

Btw: The diff looks wierd (fancy GitHub feature or user error? lol), but the only things that were added\changed were ExplorerMonitorStart() and RestartRetroBar(). There is also new variables for these new functions (on top of the file).

I think I can fix the diff to show only your changes here (you may be using CRLF instead of LF).

Fixes crash-restart-loop issues: #942 #938 #927 #648 #476 #457 Co-Authored-By: MKKNinetyTwo <[email protected]>

ghost · 2024-11-11T19:32:21Z

Btw: The diff looks wierd (fancy GitHub feature or user error? lol), but the only things that were added\changed were ExplorerMonitorStart() and RestartRetroBar(). There is also new variables for these new functions (on top of the file).

I think I can fix the diff to show only your changes here (you may be using CRLF instead of LF).

Thanks :) Please do!

dremin · 2024-11-11T20:15:30Z

I'm not sure this is quite right--there could be several explorer.exe processes, how do we know we are looking at the current user's shell process and not, say, a file explorer window process?

ghost · 2024-11-11T20:25:04Z

I'm not sure this is quite right--there could be several explorer.exe processes, how do we know we are looking at the current user's shell process and not, say, a file explorer window process?

True.

What if we check that the process exe is located at "windows/explorer.exe"?

The explorer processes are stored on array so we could do foreach on them maybe?

(But this would not cover custom shells, but maybe expand to customs later if even needed)

ghost · 2024-11-11T20:34:12Z

Or get the current shell exe path and name from registry, and compare process paths to that? Then explorer.exe can be located on any path and using any exe name

ghost · 2024-11-11T20:54:04Z

Hmm also the explorer.exe shell that runs desktop etc usually has longests start time, so this could narrow it down even more. And the desktop shell usually does not have a parent process so that can be used as a filter too.

-Added ability to detect the main shell process so we dont accidentally target File Explorers or such -Added suport for custom shells -Some other minor tweaks

ghost · 2024-11-11T22:58:21Z

@dremin I updated the pull request files. now it detects the proper shell\explorer process. There is also some other minor tweaks and a support for custom shells. Let me know what you think.

RetroBar/Utilities/WindowManager.cs

dremin · 2024-11-12T04:19:08Z

I'd like to try avoiding crashing Explorer altogether if possible. This commit prevents the crash for me: cairoshell/ManagedShell@33dc8a3

dremin · 2024-11-12T05:03:06Z

Even with the Explorer crash fixed, we may still want something like what is in this PR so that we can restore the AppBar state and workarea after Explorer restarts. Without the Explorer crash, we can simplify things by listening for the TaskbarCreated message instead of having to detect a PID.

ghost · 2024-11-12T05:11:24Z

Your fix for the crash is much better, thanks for taking a look at this. :)

Should I close this PR now that the issue is solved?

You can take/modify anything on the code of this PR if you want to play around with the TaskbarCreated, or I can try to modify it to use TaskbarCreated if you want? How should we proceed?

EDIT:
I will try to implement the TaskbarCreated, will make updates when done.

ghost · 2024-11-12T12:34:01Z

@dremin I implemented the TaskbarCreated event monitor, and also rewrote the description etc of this pull request.

I used a form to create the monitor, it is more efficient than the other thread version I made.
Let me know what you think.

Also your ManagedShell PR did fix the crashes 100%, it is good now. :)

ghost · 2024-11-12T16:59:45Z

These were the last commits from me for now. Waiting for review. Feel free to change\remove anything on the code. :)

dremin · 2024-11-14T06:42:00Z

Nice! A few comments:

For the purposes of the monitor window, a Form is pretty heavy. Instead we should create a simple NativeWindow, there are a few examples of this in the ManagedShell code if needed.
The message for TaskbarCreated will never change, so as an optimization RegisterWindowMessage should only be called once rather than as part of the WndProc.
You can import all of the pinvokes from ManagedShell.Interop, it should also have any constants needed.
I don't think we need to restart RetroBar completely, instead calling WindowManager.ReopenTaskbars() should be sufficient.

ghost · 2024-11-14T07:31:41Z

Nice! A few comments:

For the purposes of the monitor window, a Form is pretty heavy. Instead we should create a simple NativeWindow, there are a few examples of this in the ManagedShell code if needed.

The message for TaskbarCreated will never change, so as an optimization RegisterWindowMessage should only be called once rather than as part of the WndProc.

You can import all of the pinvokes from ManagedShell.Interop, it should also have any constants needed.

I don't think we need to restart RetroBar completely, instead calling WindowManager.ReopenTaskbars() should be sufficient.

Done.

I tried with both restarting and just reopening taskbars, here are the results:

For some reason if i dont restart completely, the desktop is not resized.

I left the ReopenTaskbars code as commented-out to the code, maybe I use it wrong or something?

ghost · 2024-11-14T08:11:56Z

ok i used ReopenTaskbars wrong, I needed to pass the WindowManager as a reference... I will make a commit in few minutes.

ghost · 2024-11-15T02:45:50Z

Done

dremin

🎉

xoascf · 2024-11-16T01:17:19Z

When explorer.exe crashed on Windows 11 at logon, it created two taskbars on the same screen (there is only one enabled) while using the same .exe (a single instance, but two taskbar windows), I don't recall this happening before this PR was merged. Could this change be related, @dremin?

dremin · 2024-11-16T01:38:37Z

When explorer.exe crashed on Windows 11 at logon, it created two taskbars on the same screen (there is only one enabled) while using the same .exe (a single instance, but two taskbar windows), I don't recall this happening before this PR was merged. Could this change be related, @dremin?

Hm, it's definitely possible. How are you creating the crash at login with the current build?

dremin · 2024-11-16T03:36:16Z

When explorer.exe crashed on Windows 11 at logon, it created two taskbars on the same screen (there is only one enabled) while using the same .exe (a single instance, but two taskbar windows), I don't recall this happening before this PR was merged. Could this change be related, @dremin?

Hm, it's definitely possible. How are you creating the crash at login with the current build?

@xoascf I noticed a potential race. Could you see if it is fixed with the consistent-singleton-init branch?

ghost · 2024-11-16T04:57:35Z

When explorer.exe crashed on Windows 11 at logon, it created two taskbars on the same screen (there is only one enabled) while using the same .exe (a single instance, but two taskbar windows), I don't recall this happening before this PR was merged. Could this change be related, @dremin?

Would it be possible to get the data from windows event viewer>windows logs>application>crash?

Interested in how it is crashing on logon.

On my system retrobar has a delay on startup, few seconds, it is never launch before explorer.exe. retrobar is set to autostart on registry hkcu/run (default behaviour when autostart is enabled on retrobar).

Do you have it set to start on hkcu/run, or somewhere else? (I am trying to reproduce the crash).

ghost · 2024-11-16T06:35:10Z

I managed to trigger the 2 taskbars, and yes it seems to be a race condition:

To test that your problem is the same, and caused by this change, comment out this and test again, it should not trigger 2 taskbars then:

WindowManager.cs
line 29
_explorerMonitor.ExplorerMonitorStart(this);

i used a custom explorer.exe crash to trigger it: (use only for debugging)
c++

#include <windows.h>
#include <tlhelp32.h>
#include <iostream>

DWORD GetExplorerPID() {
    DWORD pid = 0;
    HANDLE snap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
    if (snap != INVALID_HANDLE_VALUE) {
        PROCESSENTRY32 entry = { sizeof(PROCESSENTRY32) };
        if (Process32First(snap, &entry)) {
            do {
                if (_wcsicmp(entry.szExeFile, L"explorer.exe") == 0) {
                    pid = entry.th32ProcessID;
                    break;
                }
            } while (Process32Next(snap, &entry));
        }
        CloseHandle(snap);
    }
    return pid;
}

int main() {
    // Get the PID of explorer.exe
    DWORD pid = GetExplorerPID();
    if (pid == 0) {
        std::cerr << "Explorer process not found!" << std::endl;
        return 1;
    }

    // Open the Explorer process
    HANDLE process = OpenProcess(PROCESS_ALL_ACCESS, FALSE, pid);
    if (!process) {
        std::cerr << "Failed to open Explorer process!" << std::endl;
        return 1;
    }

    // Allocate memory in the target process
    LPVOID remoteMemory = VirtualAllocEx(process, NULL, 1024, MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE);
    if (!remoteMemory) {
        std::cerr << "Failed to allocate memory in Explorer process!" << std::endl;
        CloseHandle(process);
        return 1;
    }

    // Write payload to remote process
    const char* payload = "MessageBoxA(NULL, \"Injected!\", \"Debug\", MB_OK);";
    SIZE_T written;
    if (!WriteProcessMemory(process, remoteMemory, payload, strlen(payload) + 1, &written)) {
        std::cerr << "Failed to write to Explorer process memory!" << std::endl;
        VirtualFreeEx(process, remoteMemory, 0, MEM_RELEASE);
        CloseHandle(process);
        return 1;
    }

    // Create a remote thread to execute the payload
    HANDLE thread = CreateRemoteThread(process, NULL, 0, (LPTHREAD_START_ROUTINE)remoteMemory, NULL, 0, NULL);
    if (!thread) {
        std::cerr << "Failed to create remote thread in Explorer process!" << std::endl;
        VirtualFreeEx(process, remoteMemory, 0, MEM_RELEASE);
        CloseHandle(process);
        return 1;
    }

    // Wait for the remote thread to finish and clean up
    WaitForSingleObject(thread, INFINITE);
    VirtualFreeEx(process, remoteMemory, 0, MEM_RELEASE);
    CloseHandle(thread);
    CloseHandle(process);

    std::cout << "Code injected successfully!" << std::endl;
    return 0;
}

ghost · 2024-11-16T06:45:27Z

and the delay in launching programs listed under the HKCU\Software\Microsoft\Windows\CurrentVersion\Run registry key occurs because these entries are executed after explorer.exe initializes. On Windows 11, this behavior ensures the shell environment is fully loaded before user-specific applications are started, leading to a smoother logon experience.

To launch RetroBar immediately at logon without delay:

Use a Scheduled Task (Immediate Logon Execution)
You can create a Scheduled Task configured to run at logon.
This method allows precise control over the execution timing of your application.

Open the Task Scheduler (taskschd.msc).
Click Create Task in the right-hand pane.

In the General tab:
Name your task to "Run RetroBar"
Select Run only when user is logged on.
Check Run with highest privileges

In the Triggers tab:
Click New.
Set Begin the task to At logon.
Select the specific user or leave it as "Any user."

In the Actions tab:
Click New.
Set Action to Start a program.
Browse to RetroBar.exe

In the Conditions tab:
Uncheck Start the task only if the computer is on AC power.

In the Settings tab:

Check Allow task to be run on demand.
When you log in, the task runs immediately, bypassing the delay of HKCU Run.

Then do the same for the explorer.exe crasher and there you go, 2 taskbars (not on every logon for some reason?)

dremin · 2024-11-16T06:52:46Z

@MKKNinetyTwo does it still happen if you move _explorerMonitor.ExplorerMonitorStart(this); after openTaskbars();?

ghost · 2024-11-16T06:56:11Z

@MKKNinetyTwo does it still happen if you move _explorerMonitor.ExplorerMonitorStart(this); after openTaskbars();?

no, it does not happen.

simplest way to simulate the race is to replace the
_explorerMonitor.ExplorerMonitorStart(this);
with directly calling
ReopenTaskbars();

more accurate way to simulate is the explorer crasher.

ghost · 2024-11-16T07:17:54Z

@MKKNinetyTwo does it still happen if you move _explorerMonitor.ExplorerMonitorStart(this); after openTaskbars();?

And yeah, now the code basically says:
Closetaskbars
Opentaskbars
Opentaskbars

If explorermonitorstart is moved after opentaskbars, then it is:
Opentaskbars
Closetaskbars
Opentaskbars

So move that 1 line to make the logic correct and fix the bug.

ghost · 2024-11-16T07:26:43Z

And what is the trigger for the bug:

If crash happens just at the right moment, just after when line 29 has executed, and before opentaskbars is called, we get 2 taskbars. So it is a race between reopentasbars called by explorermonitor and windowmanager opentaskbars().

dremin · 2024-11-16T07:29:02Z

It should now be fixed by #951.

ghost · 2024-11-16T08:07:06Z

It should now be fixed by #951.

I can confirm that this commit fixed the bug, atleast I cant trigger it anymore even with debugging tricks no matter how hard I try.

@xoascf I hope it is fixed for you now too :)

xoascf · 2024-11-19T01:03:42Z

Thanks, I happened to reproduce the crash twice, didn't have time to test and report back, that and explorer.exe fixed itself after a reboot. If this had been released earlier without the fix, it would have caused a lot of headaches! 😬

xoascf · 2024-11-20T05:59:57Z

It should now be fixed by #951.

Somehow I managed to trigger it again (this time on Windows 10) after disabling Game Mode on a Samsung Smart TV (which stretched the resolution from 2560x1440 at 100% to 4K at 150%). I'm currently using the latest commit: 97392db.

dremin · 2024-11-20T06:02:12Z

It should now be fixed by #951.

Somehow I managed to trigger it again (this time on Windows 10) after disabling Game Mode on a Samsung Smart TV (which stretched the resolution from 2560x1440 at 100% to 4K at 150%). I'm currently using the latest commit: 97392db.

Does this consistently reproduce that way? Does it happen before this PR?

xoascf · 2024-11-20T06:22:36Z

Does this consistently reproduce that way? Does it happen before this PR?

No, it only happens just ~~before~~ after this PR.
Yeah, English can be confusing. Especially if you're staying out late and your mind is racing ("después" means after, and "antes" means before).

Screenshots

Before merging PR #945 (commit 3be4280):

Just after merging PR #945 (commit f52ab9c):

dremin · 2024-11-20T06:31:17Z

@xoascf On my system, changing the monitor scale triggers the TaskbarCreated message, but it doesn't give me the double taskbars. If you change your monitor scale only, does that trigger the issue?

xoascf · 2024-11-20T06:36:16Z

@dremin Changing the monitor scale does not trigger the double taskbars (at least for commit f52ab9c).

dremin · 2024-11-20T06:39:58Z

@xoascf Okay cool, so I think this is just another race with NotifyDisplayChange this time. I pushed a branch fix-reopen-race that adds a lock. Could you please see if it is fixed in that branch?

xoascf · 2024-11-20T06:46:51Z

@xoascf Okay cool, so I think this is just another race with NotifyDisplayChange this time. I pushed a branch fix-reopen-race that adds a lock. Could you please see if it is fixed in that branch?

Still the same issue.

Maybe irrelevant now, but just a heads up

By the way, just after re-enabling Game Mode (after having set it to Portrait mode before), the taskbar shrinks to half of the screen (and positions in half for all edges) for the commit prior to this PR (3be4280).

After trying commit 1733883 of branch fix-reopen-race that does not happen.

dremin · 2024-11-20T06:58:40Z

@xoascf Could you please enable debug logging, then reproduce the double taskbar issue, and provide the logs?

xoascf · 2024-11-20T07:16:59Z

@xoascf Could you please enable debug logging, then reproduce the double taskbar issue, and provide the logs?

@dremin, here is the log file when reproducing that issue for commit 1733883: 2024-11-20_020735924.log

dremin · 2024-11-20T07:36:26Z

@xoascf Could you please enable debug logging, then reproduce the double taskbar issue, and provide the logs?

@dremin, here is the log file when reproducing that issue for commit 1733883: 2024-11-20_020735924.log

@xoascf Thanks! I just pushed another commit to the fix-reopen-race branch. Could you please try it?

xoascf · 2024-11-20T07:41:33Z

@xoascf Could you please enable debug logging, then reproduce the double taskbar issue, and provide the logs?

@dremin, here is the log file when reproducing that issue for commit 1733883: 2024-11-20_020735924.log

@xoascf Thanks! I just pushed another commit to the fix-reopen-race branch. Could you please try it?

@dremin That worked! ~~But now the taskbar shrinks to half the screen when I re-enable Game Mode (as I showed before).~~ Oh wait, it takes a little while for it to reposition to its normal size but it does.

ghost mentioned this pull request Nov 11, 2024

RetroBar causes infinite restart loop on Explorer.exe #942

Closed

Implement ExplorerMonitor

2ae9af7

Fixes crash-restart-loop issues: #942 #938 #927 #648 #476 #457 Co-Authored-By: MKKNinetyTwo <[email protected]>

xoascf force-pushed the master branch from 7ce282d to 2ae9af7 Compare November 11, 2024 19:33

Update ExplorerMonitor

e97d00e

-Added ability to detect the main shell process so we dont accidentally target File Explorers or such -Added suport for custom shells -Some other minor tweaks

xoascf reviewed Nov 12, 2024

View reviewed changes

RetroBar/Utilities/WindowManager.cs Outdated Show resolved Hide resolved

dremin mentioned this pull request Nov 12, 2024

Forward some Explorer messages via PostMessage cairoshell/ManagedShell#113

Merged

MKKNinetyTwo added 2 commits November 12, 2024 10:42

Replace "InteropServices" with "ManagedShell.Interop".

32013df

Use TaskbarCreated event to monitor Explorer restarts.

0f0c0a8

MKKNinetyTwo added 3 commits November 12, 2024 14:54

Remove ExplorerMonitor from the Alt+Tab list.

3e99e7f

Move ExplorerMonitor to ExplorerMonitor.cs and simplify code.

503fcb5

Move ExplorerMonitor to ExplorerMonitor.cs and simplify code.

6d6547a

Replace Form with NativeWindow.

6d98ce5

MKKNinetyTwo added 2 commits November 14, 2024 10:14

Call ReopenTaskbars instead of complete restart.

c00a39a

Call ReopenTaskbars instead of complete restart.

bc158a5

Minor refactorings.

e5363b2

dremin approved these changes Nov 15, 2024

View reviewed changes

dremin merged commit f52ab9c into dremin:master Nov 15, 2024
3 checks passed

This was referenced Nov 21, 2024

Taskbar size does not update after changing display scaling percentage #151

Open

Restarting Explorer causes windows to no longer respect the taskbar area and overlap it #714

Closed

Implement Explorer.exe monitoring feature. #945

Implement Explorer.exe monitoring feature. #945

Conversation

ghost commented Nov 11, 2024 • edited by ghost Loading

ghost commented Nov 11, 2024 • edited by ghost Loading

xoascf commented Nov 11, 2024

ghost commented Nov 11, 2024

dremin commented Nov 11, 2024

ghost commented Nov 11, 2024

ghost commented Nov 11, 2024

ghost commented Nov 11, 2024

ghost commented Nov 11, 2024

dremin commented Nov 12, 2024

dremin commented Nov 12, 2024

ghost commented Nov 12, 2024 • edited by ghost Loading

ghost commented Nov 12, 2024 • edited by ghost Loading

ghost commented Nov 12, 2024

dremin commented Nov 14, 2024

ghost commented Nov 14, 2024 • edited by ghost Loading

ghost commented Nov 14, 2024

ghost commented Nov 15, 2024

dremin left a comment

Choose a reason for hiding this comment

xoascf commented Nov 16, 2024

dremin commented Nov 16, 2024

dremin commented Nov 16, 2024

ghost commented Nov 16, 2024

ghost commented Nov 16, 2024 • edited by ghost Loading

ghost commented Nov 16, 2024

dremin commented Nov 16, 2024

ghost commented Nov 16, 2024

ghost commented Nov 16, 2024

ghost commented Nov 16, 2024

dremin commented Nov 16, 2024

ghost commented Nov 16, 2024

xoascf commented Nov 19, 2024

xoascf commented Nov 20, 2024

dremin commented Nov 20, 2024

xoascf commented Nov 20, 2024 • edited Loading

dremin commented Nov 20, 2024

xoascf commented Nov 20, 2024

dremin commented Nov 20, 2024

xoascf commented Nov 20, 2024

dremin commented Nov 20, 2024

xoascf commented Nov 20, 2024

dremin commented Nov 20, 2024

xoascf commented Nov 20, 2024 • edited Loading

ghost commented Nov 11, 2024 •

edited by ghost

Loading

ghost commented Nov 11, 2024 •

edited by ghost

Loading

ghost commented Nov 12, 2024 •

edited by ghost

Loading

ghost commented Nov 12, 2024 •

edited by ghost

Loading

ghost commented Nov 14, 2024 •

edited by ghost

Loading

ghost commented Nov 16, 2024 •

edited by ghost

Loading

xoascf commented Nov 20, 2024 •

edited

Loading

xoascf commented Nov 20, 2024 •

edited

Loading