Electron-builder: NSIS uninstaller doesn't kill child processes

[jkawamoto]

• Version: 19.54.0

• Target: Windows x64

NSIS uninstaller checks the app is still running and kills it by nsProcess::KillProcess before deleting the installed files. However, nsProcess::KillProcess seems neither emitting any window-alll-closed and quit events, nor kills child (and grand child) processes. Hence, child processes spawned by the app keep running and block files are deleted.

Using taskkill instead of nsProcess::KillProcess might solve this problem.

I found some related discussions:

• Kill a spawned process, on windows
• before-quit, will-quit and quit not triggered on windows 10
• Quit gracefully when Ctrl-C is pressed in console

[develar]

Nice catch, thanks a lot.

[develar]

Since Electron in any case requires Windows 7, taskkill can be used.

[jkawamoto]

Thank you for the quick response!

[IDrinkMoreWater]

@develar @jkawamoto hi，This change brought a new bug. It happens when autoupdate ；“taskkill /f /t “ ，This will kill the installation process，Because the installation process is the main application of the child processes.

As shown:

pid-2028 is a child processes of pid-3044，pid-2028 is installation process，pid-3044 is main application

[IDrinkMoreWater]

@develar hi，I tested it 20.4.0，It still does not working. if change 'taskkill /f /t /im' to 'taskkill /f /im',it will working.

[jkawamoto]

Without \t option, I think it wouldn’t fix this issue.

I wonder why the installation process is still a child process of the main process even if it was spawned with {detached: true, stdio: ’ignore’}.

[IDrinkMoreWater]

@jkawamoto @develar
set {detached: true, stdio: ’ignore’}，This just means “On Windows, setting options.detached to true makes it possible for the child process to continue running after the parent exits. The child will have its own console window.”,But if you kill the process tree（with ‘’taskkill /t‘’），all the processes will be killed ，because the parent-child relationship between processes will not change；
moreover，‘’taskkill /t‘’ a is higher than "PID ne $pid" of priority,so it will still kill installation process.

As shown:
pid-4816 is a child processes of pid-4046，pid-4816 is installation process，pid-4046 is main application,and "好药店.exe" is APP_EXECUTABLE_FILENAME.
pid-4816, pid-4046, pid-7512（another child process） ，They are all killed

[consense]

Hi @develar ,
I am still seeing the problem of installer apparently being killed during autoupdate with 20.4.1.

Caveat:
2 years or so ago the installer didnt finish a running application on its own yet so I had added the following as custom NSIS include do do this from my end:

!macro customInit
    ; SHUT DOWN APP IF CURRENTLY RUNNING
    ${GetProcessInfo} 0 $0 $1 $2 $3 $4
    ${if} $3 != "${APP_EXECUTABLE_FILENAME}"
        ${nsProcess::FindProcess} "${APP_EXECUTABLE_FILENAME}" $R0
        ${If} $R0 == 0
            ;MessageBox MB_OK "App currently running - going to shutdown to install new version"
            ${nsProcess::CloseProcess} "${APP_EXECUTABLE_FILENAME}" $R0
            Sleep 5000
            ${nsProcess::KillProcess} "${APP_EXECUTABLE_FILENAME}" $R0
            Sleep 3000
        ${EndIf}
        ${nsProcess::Unload}
    ${endIf}
!macroend

this led to the error message

Plugin not found, cannot call ${nsProcess::FindProcess}
Error in macro customInit on macroline 4
Error in script "<stdin>" on line 131 -- aborting creation process
  • exited          command=makensis.exe code=1 pid=1040
  • async task error error=
                       Error: \tmp-electron-builder\nsis\nsis-3.0.1.13\Bin\makensis.exe exited with code 1
                           at ChildProcess.childProcess.once.code (C:\cygwin64\home\sshd\builds\code\screenawar

a few days ago (while having worked fine for close to 2 years).
So I took it out and now see the installer closing the original running install and not installing the new version (no logs being produced).
I.e.: not sure if it is a coincidence (me removing the custom nsis exit-running-app code while this ticket was coming up) or whether the fix 12 hours ago is maybe not complete yet.

Gladly let me know if there is any other info I could provide to shed some light on this.

[develar]

nsProcess Is not bundled now. Thanks for code snippet— it seems you use more longer timeouts.

[develar]

@consense "Close" here means that on kill process windows will be closed. I think, it helps app to exit gracefully. But in our case I think proper solution will be still not use nsProcess plugin, but instead try to ask process to exit gracefully.

possible fix:

1. kill using taskkill without /f (force) and /t (tree kill)
2. wait 2 seconds
3. kill using taskkill with /f and /t.

[develar]

tree kill is not used now — now app is killed softly at first and it should be enough to terminate child process.

[IGassmann]

It seems like the installer doesn't kill grandchildren processes. Is this an expected behavior?

[tzarebczan]

@develar so here is our problem:

We spawn LBRY.exe which spawns child process for our daemon, which is lbrynet-daemon.exe . When lbrynet-daemon.exe is spawned, it creates a shell and spawns the actual lbrynet-daemon.exe, which is now a child of the child (or grandchild if you will). When we run the upgrade (or the installer when the app is running), it kills LBRY.exe and the shell lbrynet-daemon.exe, which leaves the grandchild process running. We are still unsure if the current behavior of killing the shell process not killing the child is correct (need to do more research).

Can you share your thoughts on this? I believe this would be fixed by running taskkill /t, but you've stated before that this might kill the installer itself? Is this still the case? In https://github.com/electron-userland/electron-builder/issues/2516#issuecomment-372009092 you alluded to waiting 2 seconds and then killing with /t - don't think this would help us as LBRY.exe would already be killed, but I'm not 100% sure.

We'd be willing to tip if the builder can support our scenario somehow.

[tzarebczan]

Did a little more digging and this only happens when running the installation manually while our app is running. It seems like the child processes and grandchild processes are killed during an in app upgrade, but we did notice that we get 2 UAC alerts to run the updated installer, once with a --updated --force-run parameter and then a second time with --updated /S . When the 2nd UAC prompt comes up, the first is already installing, so if you wait and then proceed when the app is starting/started, it kills it again and then doesn't restart and leaves a child lbrynet-daemon.exe process running.

Can the task killing process (from in-app upgrade process) be replicated when running the installer manually and choosing the "LBRY is running, click OK to close it" option?

We'll try to dig into this behavior a bit more also.

[ustun]

As @IDrinkMoreWater explained at https://github.com/electron-userland/electron-builder/issues/2516#issuecomment-371688191 the current approach is problematic. The /t switch kills the whole tree, even if the /fi switch is supplied, resulting in the installer being killed.

This should also be related to #4891