-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: CreateFile ERROR_FILE_NOT_FOUND from crashpad handler #116252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this could possibly improve. To give some background, the first startup will get a process.env
from here:
vscode/src/vs/code/electron-main/main.ts
Lines 202 to 217 in 2baf63a
private patchEnvironment(environmentService: IEnvironmentMainService): IProcessEnvironment { | |
const instanceEnvironment: IProcessEnvironment = { | |
VSCODE_IPC_HOOK: environmentService.mainIPCHandle | |
}; | |
['VSCODE_NLS_CONFIG', 'VSCODE_PORTABLE'].forEach(key => { | |
const value = process.env[key]; | |
if (typeof value === 'string') { | |
instanceEnvironment[key] = value; | |
} | |
}); | |
Object.assign(process.env, instanceEnvironment); | |
return instanceEnvironment; | |
} |
This will end up as initialUserEnv
into each window as you can see here:
configuration.userEnv = { ...this.initialUserEnv, ...options.userEnv }; |
The userEnv
will overwrite anything and thus probably leading to CHROME_CRASHPAD_PIPE_NAME
getting the bad value for the window.
Now, instead of patching the environment in the launch main service, I would rather suggest to fix it centrally where we define the environment for the window itself, here:
vscode/src/vs/code/electron-main/window.ts
Line 693 in c023260
load(config: INativeWindowConfiguration, { isReload, disableExtensions }: { isReload?: boolean, disableExtensions?: boolean } = Object.create(null)): void { |
We already have code to change the userEnv
. I would suggest to overwrite CHROME_CRASHPAD_PIPE_NAME
with the value from process
somewhere there so that we make sure no other code path of opening a window would hit the same issue.
5d3a82f
to
a96b7e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks better to me. Not sure why we would need to check this:
if (process.env['CHROME_CRASHPAD_PIPE_NAME'] !== config.userEnv['CHROME_CRASHPAD_PIPE_NAME']) {
I think we could always assign the value over, no?
0a7b457
to
9e2e11e
Compare
Yup we can, was trying to avoid |
Fixes #115874
Fixes #112216