Call host to launch Chrome unelevated if host sends supportsLaunchUnelevatedProcessRequest flag. #676
Conversation
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { | ||
| return new Promise<number>((resolve, reject) => { | ||
| this._session.sendRequest("launchUnelevated", { |
There was a problem hiding this comment.
We are wrapping sendRequest in a promise in a similar way here: https://github.com/Microsoft/vscode-chrome-debug-core/blob/47782a37b6df5efd3a50bf1813019c0068a984fe/src/transformers/fallbackToClientPathTransformer.ts#L36
Would it make sense to extract the async sendRequest logic someplace else?
There was a problem hiding this comment.
@digeff The two usages are not quite the same, they are very different in the parameters they each have respectively. Having a common utility function will carry the different parameters as well, it almost like repeating this code. So extracting a common logic does not improve much, but reduces readability.
@roblourens if the operation does not succeed it will reject, and the call will get an exception thrown then telemetry handler will capture that. Otherwise, there is C# telemetry implemented in the host for each PZ extension request.
There was a problem hiding this comment.
@changsi-an would we know from the telemetry that the error happened because of Chrome launch failure when running as Administrator?
There was a problem hiding this comment.
@dhanvikapila Yes, the admin flag is right on the same DA event (clientRequest/launch). The C# error is on a separate event.
src/chromeDebugAdapter.ts
Outdated
| logger.log(`Parsed output file and got Chrome PID ${pidStr}`); | ||
| this._chromePID = parseInt(pidStr, 10); | ||
| if (this._doesHostSupportLaunchUnelevatedProcessRequest) { | ||
| chromePid = await this.spawnChromeUnelevatedWithPineZorro(chromePath, chromeArgs); |
There was a problem hiding this comment.
Would this be equivalent to use this._chromePID here instead?
src/chromeDebugAdapter.ts
Outdated
| 'meteor://💻app/*': '${webRoot}/*' | ||
| }; | ||
|
|
||
| interface IExtendedInitializeRequestArgumetns extends DebugProtocol.InitializeRequestArguments{ |
src/chromeDebugAdapter.ts
Outdated
| return null | ||
| } | ||
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { |
There was a problem hiding this comment.
Maybe don't name this specifically for pinezorro, just "WithClient"
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { | ||
| return new Promise<number>((resolve, reject) => { | ||
| this._session.sendRequest("launchUnelevated", { |
host sends supportsLaunchUnelevatedProcessRequest flag.
|
PTAL |
|
Can we add tests for this change? |
|
@changsi-an LMK when this is ready to merge. I'm ok with or without tests. |
|
@roblourens Please help merge it now. I will submit a PR for unit test later. |
host sends supportsLaunchUnelevatedProcessRequest flag.
Now PZ host is capable of supporting a
launchUnelevatedrequest from DA. If supportsLaunchUnelevatedProcessRequest flag is set throughinitializerequest, DA will choose to use host's ability to launch Chrome unelevated (when needed).spawnChromeUnelevatedWithWindowsScriptHost()is extracted old code.spawnChromeUnelevatedWithPineZorro()is the new logic to ask host to launch Chrome unelevated.