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

Call host to launch Chrome unelevated if host sends supportsLaunchUnelevatedProcessRequest flag. #676

Merged
merged 1 commit into from Jun 4, 2018

Conversation

Projects
None yet
5 participants
@changsi-an
Member

changsi-an commented Jun 1, 2018

host sends supportsLaunchUnelevatedProcessRequest flag.

Now PZ host is capable of supporting a launchUnelevated request from DA. If supportsLaunchUnelevatedProcessRequest flag is set through initialize request, 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.

@changsi-an

This comment has been minimized.

Member

changsi-an commented Jun 1, 2018

@changsi-an changsi-an changed the title from Call host to launch Chrome unelevated if to Call host to launch Chrome unelevated if host sends supportsLaunchUnelevatedProcessRequest flag. Jun 1, 2018

private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> {
return new Promise<number>((resolve, reject) => {
this._session.sendRequest("launchUnelevated", {

This comment has been minimized.

@digeff

digeff Jun 1, 2018

Contributor

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?

This comment has been minimized.

@roblourens

roblourens Jun 1, 2018

Member

Does it need telemetry?

This comment has been minimized.

@changsi-an

changsi-an Jun 1, 2018

Member

@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.

This comment has been minimized.

@dhanvikapila

dhanvikapila Jun 1, 2018

Member

@changsi-an would we know from the telemetry that the error happened because of Chrome launch failure when running as Administrator?

This comment has been minimized.

@changsi-an

changsi-an Jun 4, 2018

Member

@dhanvikapila Yes, the admin flag is right on the same DA event (clientRequest/launch). The C# error is on a separate event.

logger.log(`Parsed output file and got Chrome PID ${pidStr}`);
this._chromePID = parseInt(pidStr, 10);
if (this._doesHostSupportLaunchUnelevatedProcessRequest) {
chromePid = await this.spawnChromeUnelevatedWithPineZorro(chromePath, chromeArgs);

This comment has been minimized.

@digeff

digeff Jun 1, 2018

Contributor

Would this be equivalent to use this._chromePID here instead?

This comment has been minimized.

@changsi-an
@@ -28,15 +28,20 @@ const DefaultWebSourceMapPathOverrides: ISourceMapPathOverrides = {
'meteor://💻app/*': '${webRoot}/*'
};
interface IExtendedInitializeRequestArgumetns extends DebugProtocol.InitializeRequestArguments{

This comment has been minimized.

@roblourens

This comment has been minimized.

@changsi-an
return null
}
private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> {

This comment has been minimized.

@roblourens

roblourens Jun 1, 2018

Member

Maybe don't name this specifically for pinezorro, just "WithClient"

This comment has been minimized.

@changsi-an
private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> {
return new Promise<number>((resolve, reject) => {
this._session.sendRequest("launchUnelevated", {

This comment has been minimized.

@roblourens

roblourens Jun 1, 2018

Member

Does it need telemetry?

Call host to launch Chrome unelevated if
host sends supportsLaunchUnelevatedProcessRequest flag.
@changsi-an

This comment has been minimized.

Member

changsi-an commented Jun 1, 2018

PTAL

@rakatyal

This comment has been minimized.

Member

rakatyal commented Jun 1, 2018

Can we add tests for this change?

@roblourens

This comment has been minimized.

Member

roblourens commented Jun 4, 2018

@changsi-an LMK when this is ready to merge. I'm ok with or without tests.

@changsi-an

This comment has been minimized.

Member

changsi-an commented Jun 4, 2018

@roblourens Please help merge it now. I will submit a PR for unit test later.

@roblourens roblourens merged commit f26a832 into Microsoft:master Jun 4, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@roblourens roblourens added this to the June 2018 milestone Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment