vrcx-team / VRCX

Friendship management tool for VRChat
MIT License
975 stars 183 forks source link

[Bug] Auto Launch not closing apps after closing VRchat #577

Closed JanieUwU closed 1 year ago

JanieUwU commented 1 year ago

Describe the bug Auto launch setting to auto close does not shut down the program.

To Reproduce Steps to reproduce the behavior:

  1. Put app shortcut into startup folder.
  2. Launch VRC, app auto launches with game.
  3. Close VRC, program stays open.

Expected behavior Upon closing the game, the apps that was launched with it would also be closed.

What version you are running Version VRCX 2023.06.15

Natsumi-sama commented 1 year ago

is it some apps or all apps? does it run as admin?

JanieUwU commented 1 year ago

Seems to just be one app. I was using it for a custom made program for moderation of certain in game communities so it's not signed at all, the app should not be running in admin either though.

JanieUwU commented 1 year ago

Not sure if it matters but it was built with Electron, using electon-builder's Squirrel.Windows packer?

JanieUwU commented 1 year ago

Just checked the app, it is not running as admin

JanieUwU commented 1 year ago

How does VRCX know what window to close? It could be that our app's process has different names listed for it's running processes? The shortened processes are just Moderator Toolkit while the window's name is {Community Name] Moderator Toolkit. Maybe that could be causing the issue? Screenshot 2023-06-15 201057

GroovyTeacup commented 1 year ago

I just tested it with a random electron app and it seemed to work, so I'm not sure if electron is an issue.

When the launcher starts a shortcut, it stores the Process instance of whatever got spawned by Process.Start. When things need to close, it uses the PID of the process it started to close that process and any child processes. Not the window names or handles directly.

If the app, say, starts up a with one process when launching, starts a bunch of child processes, and then kills the parent process without closing the children, VRCX will lose it because it's assumed the process has closed by itself or the user has closed it.

Do you get a UAC prompt when starting the app? AFAIK, UAC prompts will cause this issue as well, as after you accept the prompt, the original process is closed and then re-opened under another PID, also leading to VRCX losing track of the process.

DorCoMaNdO commented 1 year ago

If the app, say, starts up a with one process when launching, starts a bunch of child processes, and then kills the parent process without closing the children, VRCX will lose it because it's assumed the process has closed by itself or the user has closed it.

This can be partially addressed by periodically polling every child process for it's own child processes (and then their own child processes) and storing all those PIDs in a list to terminate, you can find child processes using WMI, see this stackoverflow answer, see this doc for query syntax.

As for UAC, the above wouldn't really work since it launches the child process and self terminates as soon as it's dismissed, so you could instead pass a launch parameter which would be retained once the process is launched by UAC (on by default but should have an option to turn off per program set to launch by VRCX, as some poorly coded applications may fail to launch correctly with a parameter they don't expect, or if they take it as a path string) and use WMI again to search for processes that might have it, use query Select ProcessID, CommandLine From Win32_Process WHERE CommandLine != NULL, you can't query for a substring so do that in code, note that most (technically all, besides those that are altered post-launch, usually done by anti-cheats) results will start with the path to application (within quotes if it contains spaces), followed by the arguments, though this shouldn't matter if the launch argument you insert is unique enough for string.Contains, I expect "-launched-by-VRCX" to be fine.

If you do make use of both of these solutions, you'll probably want to combine them to reduce queries as they can be quite slow, using Select ParentProcessID, ProcessID, CommandLine From Win32_Process and doing the parent PID check (and repeating it once you have child PIDs) and the command line check from one set of results.

GroovyTeacup commented 1 year ago

If the app, say, starts up a with one process when launching, starts a bunch of child processes, and then kills the parent process without closing the children, VRCX will lose it because it's assumed the process has closed by itself or the user has closed it.

This can be partially addressed by periodically polling every child process for it's own child processes (and then their own child processes) and storing all those PIDs in a list to terminate, you can find child processes using WMI, see this stackoverflow answer, see this doc for query syntax.

As for UAC, the above wouldn't really work since it launches the child process and self terminates as soon as it's dismissed, so you could instead pass a launch parameter which would be retained once the process is launched by UAC (on by default but should have an option to turn off per program set to launch by VRCX, as some poorly coded applications may fail to launch correctly with a parameter they don't expect, or if they take it as a path string) and use WMI again to search for processes that might have it, use query Select ProcessID, CommandLine From Win32_Process WHERE CommandLine != NULL, you can't query for a substring so do that in code, note that most (technically all, besides those that are altered post-launch, usually done by anti-cheats) results will start with the path to application (within quotes if it contains spaces), followed by the arguments, though this shouldn't matter if the launch argument you insert is unique enough for string.Contains, I expect "-launched-by-VRCX" to be fine.

If you do make use of both of these solutions, you'll probably want to combine them to reduce queries as they can be quite slow, using Select ParentProcessID, ProcessID, CommandLine From Win32_Process and doing the parent PID check (and repeating it once you have child PIDs) and the command line check from one set of results.

So,

Ma'am this is a Wendy's

JanieUwU commented 1 year ago

Do you get a UAC prompt when starting the app? AFAIK, UAC prompts will cause this issue as well, as after you accept the prompt, the original process is closed and then re-opened under another PID, also leading to VRCX losing track of the process.

No UAC prompt by windows to start the app. Not sure exactly why it will not close then. Might be that the process VRCX is trying to kill does not kill the app and the app simply restarts that process. Tired killing some of the pocesses by the app in task manager, not all closed the application but some did freak it out.

VrcX does not seem to cause any of the issues though so it just seems to fail to close/attempt to close the application at all.

GroovyTeacup commented 1 year ago

Do you get a UAC prompt when starting the app? AFAIK, UAC prompts will cause this issue as well, as after you accept the prompt, the original process is closed and then re-opened under another PID, also leading to VRCX losing track of the process.

No UAC prompt by windows to start the app. Not sure exactly why it will not close then. Might be that the process VRCX is trying to kill does not kill the app and the app simply restarts that process. Tired killing some of the pocesses by the app in task manager, not all closed the application but some did freak it out.

VrcX does not seem to cause any of the issues though so it just seems to fail to close/attempt to close the application at all.

That's possible! I'm not super familiar with the process structure of electron apps, and it may differ per versions and such. As a test of whether VRCX is tracking anything, you could open up your task manager's 'Details' tab, find the group of processes from "ModeratorToolkit.exe" or whatever it may be named, and then see if any those processes close even briefly when VRChat closes.

If not, I can add some logging to the applauncher in the next nightly and that should reveal whether its losing the process, erroring out, or something else.

DorCoMaNdO commented 1 year ago
  • Constant recursive polling of launched child processes

Unless a process frequently spawns new child processes it'll only be recursive once or twice, I'd poll once every second for the first 5 seconds (to account for initialization, especially with chromium-based applications) and then once a minute should be plenty.

  • Storing polled children in a list linked to the parent

Doesn't need to be linked to the parent, you don't care for the hierarchy, just that it needs to be closed when the game is closed.

  • WMI to get child processes

Lack of better options natively in C#, when I called the queries "slow" it was ~20ms slow, which is slow for code execution but with 60s polling would be negligible.

  • Toggles for every shortcut to turn off a single, optional launch parameter that may or may not break programs and no one will have any idea what it does

Again this was just a suggestion, and only to try and follow applications past a UAC prompt, this could also be off by default and the toggle could be named something like "alternate process tracking" with a small description to mention that it might help with programs that have a UAC prompt on launch. Also for the vast majority of programs an unrecognized launch parameter wouldn't break anything, but I have seen instances of poor code where it has, that it is the only reason I brought it up.

Natsumi-sama commented 1 year ago

I think this is way too far out of scope for a simple app launcher, but you're welcome to PR this yourself.

DorCoMaNdO commented 1 year ago

I think this is way too far out of scope for a simple app launcher, but you're welcome to PR this yourself.

578 Reined it in, no WMI, no launch parameter monitoring, reused Win API functions that were already in use by the feature to improve child process monitoring.

JanieUwU commented 1 year ago

This PR does indeed fix the issue with the app I have in question <3 Thank you