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

Launch Chrome in unelevated state on Windows platform by using `wmic call create`. #619

Merged
merged 1 commit into from Mar 20, 2018

Conversation

Projects
None yet
2 participants
@changsi-an
Copy link
Member

changsi-an commented Mar 16, 2018

This change is needed because when VS is started in elevated mode, DA will be started in elevated mode too. Any processes launched by DA, in our case Chrome, will be started in elevated mode as well. Chrome is known to have issues with elevated mode.

When DA received shouldLaunchChromeUnelevated launch parameter, it will call cscript.exe to run a customized Windows Script Host program (launchUnelevated.js) to launch chrome process in unelevated mode.

launchUnelevated.js will dump the process id information to a temporary file negotiated with DA, so after the WSH process is finished, DA will poll the content of the file and find out the PID of the launched Chrome process.

@@ -38,7 +38,8 @@ const watchedSources = [
];

const scripts = [
'src/terminateProcess.sh'
'src/terminateProcess.sh',

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

Is "teminatePRocess.sh" still needed?

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

Yes as far as I know. It's mac and Linux only

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

I can't find src/terminateProcess.sh in the code base. How does it get injected for mac and Linux cases?

@changsi-an changsi-an force-pushed the changsi-an:master branch from 03aaf7f to f41884a Mar 16, 2018

@@ -38,7 +38,8 @@ const watchedSources = [
];

const scripts = [
'src/terminateProcess.sh'
'src/terminateProcess.sh',

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

Yes as far as I know. It's mac and Linux only

@@ -38,7 +38,8 @@ const watchedSources = [
];

const scripts = [
'src/terminateProcess.sh'
'src/terminateProcess.sh',
'src/launchUnelevated.js'

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

I think this will have to be added to the csproj for signing

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

Looking at the current state of the csproj, when it uses wildcard of ..\out\src\**\* and $(OutDir)\**\*.js, it should include this file too.

@@ -222,7 +225,7 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
// Disconnect before killing Chrome, because running "taskkill" when it's paused sometimes doesn't kill it
super.disconnect(args);

if (this._chromeProc && !hadTerminated) {
if ( (this._chromeProc || (!this._chromeProc && this._chromePID)) && !hadTerminated) {

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

Shouldn't happen with the current code, but since this can go through when this._chromeProc is null, add a null check below where we call .kill on it.

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

Good catch.

Done.


telemetry.telemetry.reportEvent('error', telemetryProperties);

return null;

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

Is this supposed to just keep going and try to attach if it can't find the Chrome pid? Or should it throw and fail the launch?

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

Yes, after evaluated two approaches, we reached to a conclusion that we should let the debugging session go as much as it can. Not being able to find process id doesn't necessary mean Chrome is not launched nor non-debuggable. One known side-effect is merely that Chrome can't be closed at the end of the session. Due to current multi-debugger-client design of Chrome, that is not an issue any more.

@@ -0,0 +1,28 @@
var objShell = new ActiveXObject("shell.application");

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

I'm just going to trust that this is all correct :)

This comment has been minimized.

@roblourens

roblourens Mar 16, 2018

Member

Maybe use let instead of var.

This comment has been minimized.

@changsi-an

changsi-an Mar 16, 2018

Member

Yes, and I tested it as much as I can. It works.

Unfortunately the Windows Script Host (cscript.exe) is nowhere near any modern ECMAScript standard. let is not supported.

@changsi-an changsi-an force-pushed the changsi-an:master branch from f41884a to 5ca41a7 Mar 16, 2018

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 16, 2018

I am in the process of fixing the current build error. Potentially submit another PR to fix the build error first. So please don't merge this PR until the travis build are all green. :)

Guess the build error is already fixed now. So please disregard the above. :)

@changsi-an changsi-an force-pushed the changsi-an:master branch 2 times, most recently from 7ad7e0b to 5ca41a7 Mar 20, 2018

@roblourens roblourens merged commit aa48fc2 into Microsoft:master Mar 20, 2018

2 checks passed

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

@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018

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