Electron 16 build TS extension opens command prompts on Windows

[rzhao271]

With the latest exploration build:

1. Open VS Code
2. Open a TS file
3. 🐛 Command prompts open while the TS extension activates

CPU profile: CPU-20211209T161643.347Z.cpuprofile.txt

[rzhao271]

The only spawning I see the TS extension doing is at

vscode/extensions/typescript-language-features/src/tsServer/spawner.ts

Line 155 in fa12665

const process = this._factory.fork(version.tsServerPath, args, kind, configuration, this._versionManager);

tsServerPath points to https://github.com/microsoft/TypeScript/blob/main/lib/tsserver.js. There's also https://github.com/microsoft/TypeScript/blob/ef9fd97e4dbcc0a26fa5bac7007e8bfa594ccccd/src/typingsInstaller/nodeTypingsInstaller.ts#L113, but I don't think that is what is causing the npm install types-registry@latest command prompt to pop up.

The extension logs (including verbose server logs) also don't say anything about types-registry or prefix, two packages being installed when the TS extension is activating.

CC @mjbvz

[mjbvz]

TypeScript spawns a 'typing installer' process for automatic type acquisition. This is spawned with a call to .fork here:

https://github.com/microsoft/TypeScript/blob/8590f0db408c419d25ea767783428b59fe6c9d0b/src/tsserver/nodeServer.ts#L510

This process then called execSync to run npm:

https://github.com/microsoft/TypeScript/blob/db01e84700023a4b728fd6e38878264160c43fbf/src/typingsInstaller/nodeTypingsInstaller.ts#L206

[deepak1556]

Thanks for clarifying the process hierarchy @mjbvz

@rzhao271 the next step here would be to replicate this process tree with a minimal electron app. Basically, we want something like

• Main Process
  • ELECTRON_RUN_AS_NODE forked child process (Extension host)
    • Fork child process (TsServer)
      • Fork child process (Typing Installer)
        • ExecSync npm.cmd

You can extend the test case at https://gist.github.com/deepak1556/eefed2d47fada30c15ab9077d1532d0b which was used for #137481

[rzhao271]

I extended my minimal repro, and I can repro the issue with that process tree after packaging the app. Though, when I launch the unpackaged app from Windows terminal, I notice the title changes to npm version, which is what I made execSync run. Therefore, the unpackaged app also has the problem.

execSync itself causes issues. If I put it in the mock extension host, it still creates npm prefix and npm version command prompts.

Also, to fork the mock TsServer, I noticed that I had to reset execPath to my installed node path, otherwise Electron's node would be used again. Assuming I need to do that execPath reset, I notice another command prompt flash really quickly before the npm command prompts. If we don't have to reset execPath (meaning all children in the process tree use either Electron's node or the standard node), then this issue is irrelevant, but the execSync issue is still there.

[rzhao271]

execSync specifically comes with a windowsHide flag, that by default is set to false, meaning a new window pops up. When windowsHide is set to true, the npm command prompts do not pop up anymore. execSync also sets the shell option to true when it is not specified, before it calls spawnSync: https://github.com/nodejs/node/blob/ad747a8956ac4bdce1b4669310a93815447e6954/lib/child_process.js#L188

I can repro the issue with just the following main.js file:

const { app } = require('electron')
const cp = require('child_process');

function doSpawn () {
  cp.execSync('npm help version');
  cp.spawnSync('npm', ['version'], { shell: true });
}

app.whenReady().then(doSpawn);

[deepak1556]

Ignore testing from main.js, continue to test the same test case from a forked process off main (ELECTRON_RUN_AS_NODE) process as that is were we set the hide console window global flag explicitly. Next step with the reduced test case would be to find why the flag https://github.com/electron/electron/blob/b63c190fe6ac20f9b84e2813ab4c049862dc753a/shell/app/node_main.cc#L213-L214 is not having effect in this scenario.

[rzhao271]

spawnSync has its own file that doesn't seem to go by the usual flow.
https://github.com/nodejs/node/blob/c85e67b600c07c0c37bebd077579f102747062a4/src/spawn_sync.cc#L808

I verified that if I added the following lines below, the consoles stop showing up on the screen:

if (env()->hide_console_windows())
    uv_process_options_.flags |= UV_PROCESS_WINDOWS_HIDE_CONSOLE;

Also, I'm aware that there's already a similar code snippet at https://github.com/nodejs/node/blob/c85e67b600c07c0c37bebd077579f102747062a4/src/process_wrap.cc#L241

[rzhao271]

Actually, it looks like Electron forgot to carry over this patch

https://github.com/electron/electron/blob/13-x-y/patches/node/fix_don_t_create_console_window_when_creating_process.patch

I'll see if that patch fixes it + make a PR if needed.