chore: option to restart with sandbox disabled when workbench launch fails #186265
Conversation
| this.updateChromiumSandboxEnablement(false); | ||
| this.lifecycleMainService.relaunch(); | ||
| } else { | ||
| await this.destroyWindow(false, false); |
There was a problem hiding this comment.
| await this.destroyWindow(false, false); | |
| this.lifecycleMainService.quit(); |
There was a problem hiding this comment.
Not sure? This is to reload a crashed window.
| @@ -612,6 +614,40 @@ export class CodeWindow extends Disposable implements ICodeWindow { | |||
| return this.marketplaceHeadersPromise; | |||
| } | |||
|
|
|||
| private async updateChromiumSandboxEnablement(enable: boolean): Promise<void> { | |||
There was a problem hiding this comment.
this duplicated from
vscode/src/vs/code/electron-main/app.ts
Line 1316 in ac34ea3
There was a problem hiding this comment.
Yes that would be better. How about a reusable method in src/vs/platform/environment/node/argv.ts?
| @@ -612,6 +614,40 @@ export class CodeWindow extends Disposable implements ICodeWindow { | |||
| return this.marketplaceHeadersPromise; | |||
| } | |||
|
|
|||
| private async updateChromiumSandboxEnablement(enable: boolean): Promise<void> { | |||
There was a problem hiding this comment.
Yes that would be better. How about a reusable method in src/vs/platform/environment/node/argv.ts?
| @@ -707,6 +743,23 @@ export class CodeWindow extends Disposable implements ICodeWindow { | |||
| let message: string; | |||
| if (!details) { | |||
| message = localize('appGone', "The window terminated unexpectedly"); | |||
| } else if (details.reason === 'launch-failed' && details.exitCode === 18) { | |||
There was a problem hiding this comment.
Should we only do this on Windows? Or will this also happen on Linux and macOS?
| const { response } = await this.dialogMainService.showMessageBox({ | ||
| type: 'warning', | ||
| buttons: [ | ||
| localize({ key: 'restart', comment: ['&& denotes a mnemonic'] }, "&&Restart") |
There was a problem hiding this comment.
We should also have a "Learn more" button to explain the impact further.
| }, this._win); | ||
| if (response === 0) { | ||
| this.updateChromiumSandboxEnablement(false); | ||
| this.lifecycleMainService.relaunch(); |
There was a problem hiding this comment.
I think we should still destroy the window to ensure an orderly shutdown. I would suggest to have another parameter for destroyWindow to trigger a relaunch instead of a reload of the window.
Better error flow for #184687 (comment)