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

Fix chrome session restore promt in Windows #606

Merged
merged 4 commits into from Mar 14, 2018

Conversation

Projects
None yet
3 participants
@aj-r
Copy link
Contributor

aj-r commented Feb 28, 2018

Fixes #416.

My first approach (call continue() before killing chrome) didn't work. Sometimes the issue happens even when the debugger isn't paused.

My solution is to start by killing chrome more gracefully using taskkill /PID ${this._chromePID}. However this won't always kill chrome - if chrome prompts the user upon closing (e.g. via onbeforeunload), then chrome won't close. So in case the original kill command doesn't work, we send a follow-up command with the /F parameter to force kill.

If we end up needing to force kill, then the session restore bubble will sometimes still appear. But at least it happens much less frequently.

Note: I removed the /T parameter. taskkill /T /PID ${this._chromePID} doesn't work - I get this console output:

SUCCESS: Sent termination signal to process with PID 7020, child of PID 17760.
SUCCESS: Sent termination signal to process with PID 16576, child of PID 17760.
ERROR: The process with PID 8320 (child process of PID 17760) could not be terminated.
Reason: This process can only be terminated forcefully (with /F option).
ERROR: The process with PID 1180 (child process of PID 17760) could not be terminated.
Reason: This process can only be terminated forcefully (with /F option).
ERROR: The process with PID 17760 (child process of PID 18168) could not be terminated.
Reason: One or more child processes of this process were still running.

Apparently certain child processes can only be terminated with /F (possibly including the debugger process itself). But removing the /T option works.

logger.log(`Killing Chrome process by pid: ${taskkillCmd}`);
try {
execSync(taskkillCmd);
} catch (e) {
// Can fail if Chrome was already open, and the process with _chromePID is gone.
// Or if it already shut down for some reason.
}
if (!this._hasTerminated) {

This comment has been minimized.

@aj-r

aj-r Feb 28, 2018

Contributor

Is this the right way to check if the process is dead?

AJ Richardson
@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 5, 2018

Are those other child processes just children of the main Chrome process, like Chrome Helper? If so, please check that they are cleaned up properly with this solution. I think they will take care of that themselves.

Is the second taskkill definitely necessary? I am fine with just removing /f /t if that fixes this. If it is, then hasTerminated isn't quite right. It will be set when the target disconnects but also if the client disconnects. And isn't guaranteed to be set by the time that code runs. Checking the exit code on the execSync might be better.

@aj-r

This comment has been minimized.

Copy link
Contributor

aj-r commented Mar 5, 2018

I'll do some more testing to verify that the child processes get cleaned up.

The second task kill is not critical, but without it Chrome won't shut down in certain situations, for example if the web page prompts the user about unsaved changes. I feel like the expected behavior would be for us to kill Chrome anyway, without waiting for the user to answer the prompt.

I'll see if the exit code gives me anything useful.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 6, 2018

but without it Chrome won't shut down in certain situations, for example if the web page prompts the user about unsaved changes

Oh I see. Yeah I agree.

@aj-r

This comment has been minimized.

Copy link
Contributor

aj-r commented Mar 7, 2018

@roblourens I have verified that all child processes are cleaned up, even without the /T parameter. (And yes, the child processes are all Chrome helper processes).

Unfortunately, taskkill returns an exit code of 0, even if the process doesn't shut down. It only knows that the process received the signal; it doesn't know if the process shut down.

I think I'll just remove the if (!this._hasTerminated) { check, and always run taskkill twice. If the first command successfully kills chrome, then the second one will have no effect. The only way around this would be to query the PID to see if it's still running, but I'm not sure how to do that in node.js.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 7, 2018

In that case I think there's a very small chance that the PID would be recycled for a new process in the small slice of time between killing chrome with the first taskkill, and running the second. But I think it's very unlikely that it would happen that quickly.

@aj-r

This comment has been minimized.

Copy link
Contributor

aj-r commented Mar 8, 2018

@roblourens I have updated this PR. However, Travis complains about errors in code that I did not touch. Also, if I do npm run build locally, it succeeds. Any idea what's up?

@aj-r

This comment has been minimized.

Copy link
Contributor

aj-r commented Mar 14, 2018

@roblourens can this PR be merged? Or do you want to see some change first?

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 14, 2018

Sorry, the PR is good, I am just keeping master stable right now as we are working on shipping this in VS.

@digeff @rakatyal I don't know whether to merge this now or wait for your release cycle to be done, to give it more verification time on the vscode side.

It fixes #416, do you see that issue when running in VS? If so, you are probably interested in this change, would appreciate if you try it out.

@rakatyal

This comment has been minimized.

Copy link
Member

rakatyal commented Mar 14, 2018

@roblourens: We do see this issue in VS and would definitely like this to be fixed. We have to do an insertion later today in staging where we can verify if this works inside VS too. We don't ship till 2 more weeks so we have sufficient time to roll back just in case. So you can merge it in if this looks good to you.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 14, 2018

Ok, I'll merge it, let me know how it goes

@roblourens roblourens merged commit db839b7 into Microsoft:master Mar 14, 2018

1 of 2 checks passed

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

@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