debug: try to wait for a debugger before running extensions

[connor4312]

Fixes #106698
Fixes #99222

See discussion here: #106698 (comment). This is the same problem we were facing on the web previously, so I implemented it in a way which will solve both.

Most of the changes here are piping data around and adding it to environments. I added a separate debugExpected from just the isExtensionDevelopmentDebug, since isExtensionDevelopmentDebug is true whenever a debugging is enabled at all, which includes running ./scripts/code-cli -- using that as the "wait" signal would cause code-cli to take an extra 5 seconds to run extensions.

Tagging Joh for review seems like you've been touching much of this code most recently according to gitlens.

[connor4312]

CI failure is an existing issue in master

[bpasero]

I could not really figure out the purpose of the new arguments. One thing to keep in mind is that we should check if the properties need to be unset or changed when the window reloads, similar to this code:

vscode/src/vs/code/electron-main/window.ts

Lines 751 to 759 in a7caa97

	// Some configuration things get inherited if the window is being reloaded and we are
	// in extension development mode. These options are all development related.
	if (this.isExtensionDevelopmentHost && cli) {
	configuration.verbose = cli.verbose;
	configuration['inspect-extensions'] = cli['inspect-extensions'];
	configuration['inspect-brk-extensions'] = cli['inspect-brk-extensions'];
	configuration.debugId = cli.debugId;
	configuration['extensions-dir'] = cli['extensions-dir'];
	}

[alexdima]

I have reviewed the changes in the extension host area.

I would make extHost.protocol.ts -> IEnvironment.debugExpected an optional argument and only set it for the web worker extension host. I believe that is the one that needs this special treatment, since the other extension hosts are nodejs based and can be launched with --inspect-brk and do not need this mechanism.

[connor4312]

Thanks for the reviews!

It looks like in node-debug2 uses --inspect-brk-extensions, but in js-debug at one point (probably while initially getting extension host dev working) that was changed for --inspect-extensions -- fixing that should fix the debugging in web scenario, and then I can remove the unnecessary generalization here.

[connor4312]

One thing to keep in mind is that we should check if the properties need to be unset or changed when the window reloads

thanks for the callout -- it looks like debugRenderer is still in the configuration in that case, and we will not need to unset it as users cannot detach from the renderer without ending the debug session.

[alexdima]

👍 The changes in extHostExtensionService.ts and webWorkerExtensionHost.ts look good to me.

[bpasero]

One thing to consider is to rename debugRenderer to extensionDebugRenderer if this is only for extension debugging. That way we keep the properties grouped together that belong to extension debug.

[connor4312]

this is only for extension debugging

It's also used for debugging webviews. But, VS Code does not know whether the debugger will attach to webviews, extensions or both, it's only asked to expose the renderer.

[bpasero]

@connor4312 I see. I feel renderer is a term that Electron/Chrome introduces. Maybe we find a better more generic name or is this really only specific to Electron renderers?

[connor4312]

It's specific to electron renderers. In web we can never debug the 'renderer', as a tab cannot start another tab in debug mode.

In theory if running serverful web locally we could actually launch a debugged browser instance on the desktop, but this is probably not worth doing...