Native Exception in winpty

[dbaeumer]

• VSCode Version: 1.14 test pas
• OS Version: Windows

Executed a task in the terminal and got:

[Tyriar]

@dbaeumer what was the state of the terminal at the time? Hidden, uninitialised, visible?

Related: #27534

[dbaeumer]

It was not visible. And I saw it again today when starting VS Code out of dev and it tried to restore a terminal (no task runner). But I have no reliable steps to reproduce.

[vscodebot]

This issue has been closed automatically because it needs more information and has not had recent activity. Please refer to our guidelines for filing issues. Thank you for your contributions.

[dbaeumer]

Reopening the issue since it has all information I have.

[dbaeumer]

I see that now very frequently. Once or twice a day.

[Tyriar]

It's failing here https://github.com/rprichard/winpty/blob/ce9239af5d751195ea6982a41c7d71356ac9201c/src/libwinpty/winpty.cc#L852, meaning either winpty_t or winpty_spawn_config_t wasn't initialized correctly.

I'm guessing it was related to the config as an exception should have been thrown earlier if winpty_t
wasn't initialized correctly https://github.com/Tyriar/node-pty/blob/085f1b180f7cfbb66e6656dd8dc95dbde685351f/src/win/pty.cc#L211

@dbaeumer does your terminal config (shell, args, env, cwd) have any special characteristics? Unicode in one of them for example?

[dbaeumer]

I saw it just failing with the following command line under Windows:

C:\WINDOWS\sysnative\WindowsPowerShell\v1.0\powershell.exe -NoProfile -ExecutionPolicy Bypass Write-Host 'Invoking Pester...'; Invoke-Pester -PesterOption @{IncludeVSCodeMarker=$true}; Invoke-Command { Write-Host 'Completed Test task in task runner.' }

However I see the same exception when open a single terminal inside VS Code and even in Hyper.js from time to time.

[dbaeumer]

Invoking the above command a second time worked. So might be a timing problem.

[Tyriar]

@rprichard can you think of any reason winpty_spawn_config_new might fail and return nullptr?

It seems to be happening sometimes for @dbaeumer and I verify the winpty_t so it much be the config failing https://github.com/Tyriar/node-pty/blob/085f1b180f7cfbb66e6656dd8dc95dbde685351f/src/win/pty.cc#L211

[rprichard]

@Tyriar

can you think of any reason winpty_spawn_config_new might fail and return nullptr?

AFAICT, out-of-memory is the only way it would return nullptr.

I'm guessing it was related to the config as an exception should have been thrown earlier if winpty_t
wasn't initialized correctly https://github.com/Tyriar/node-pty/blob/085f1b180f7cfbb66e6656dd8dc95dbde685351f/src/win/pty.cc#L211

I had noticed earlier that the call sites to Nan::ThrowError in pty.cc are inconsistent. Of the 7 calls, 3 of them return the value returned by Nan::ThrowError, 1 of them discards the return value but somewhat assumes that the function returns (i.e. it's followed by goto cleanup), and 3 of them assume the function doesn't return (implying setjmp or C++ exceptions).

I'm unfamiliar with the nodejs nan module, but my guess is that Nan::ThrowError does return.
I'm guessing it records the exception object somewhere so that a JavaScript object is thrown once the native function completes. ThrowError's return type is void, FWIW, but that doesn't really answer the question:

inline void ThrowError(v8::Local<v8::Value> error) {
    v8::Isolate::GetCurrent()->ThrowException(error);
}

I commented on this previously; maybe you didn't see it? It's here: #28593 (comment). "Aside: the control flow with Nan::ThrowError in node-pty is suspicious looking".

[dbaeumer]

Got it today with Hyper.js. Here the screen shot.

[Tyriar]

@rprichard yeah I haven't touched that stuff since I forked it https://github.com/chjj/pty.js/blob/master/src/win/pty.cc, can have a look at that in microsoft/node-pty#102. We've actually been seeing more out of memory crashes on Windows than other platforms recently so maybe that's related to this.

[Tyriar]

This is likely related to winpty, tracking in #45693