Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: try to remove the temporary user data directory after the proces… #7761

Merged
merged 3 commits into from Nov 15, 2021
Merged

fix: try to remove the temporary user data directory after the proces… #7761

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c6dc5d
fix: try to remove the temporary user data directory after the proces…
whimboo Nov 10, 2021
5769870
Merge branch 'main' into kill_temporary_folder
OrKoN Nov 11, 2021
67e5d29
Merge branch 'main' into kill_temporary_folder
OrKoN Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/node/BrowserRunner.ts
View file
Open in desktop
Expand Up @@ -180,13 +180,6 @@ export class BrowserRunner {
}

kill(): void {
// Attempt to remove temporary profile directory to avoid littering.
try {
if (this._isTempUserDataDir) {
removeFolder.sync(this._userDataDir);
}
} catch (error) {}

// If the process failed to launch (for example if the browser executable path
// is invalid), then the process does not get a pid assigned. A call to
// `proc.kill` would error, as the `pid` to-be-killed can not be found.
Expand All @@ -199,6 +192,14 @@ export class BrowserRunner {
);
}
}

// Attempt to remove temporary profile directory to avoid littering.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this.proc.kill('SIGKILL') throws but the process is still killed? Can that happen in practice? That might be the reason for removing the temp dir upfront?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. So I've taken a look at the subprocess.kill() documentation, and I cannot see that an error gets thrown at all. So my question would be do we really need the try/catch and will we ever re-throw the error (that is the part that I totally issed - thanks for noting!).

Shouldn't a check for the return value be performed instead? We could then throw an error at the end of the method, and could still have the user data directory deletion before. If it is false we could still try to remove the user data directory even if not fully successful.

Maybe the error and close events could be used here to better handle the kill status by making the BrowserRunner.kill() method async.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's strange. I could not find why it was like this in the first place. It does not look like it can throw https://github.com/nodejs/node/blob/master/lib/child_process.js#L419

try {
if (this._isTempUserDataDir) {
removeFolder.sync(this._userDataDir);
}
} catch (error) {}

// Cleanup this listener last, as that makes sure the full callback runs. If we
// perform this earlier, then the previous function calls would not happen.
helper.removeEventListeners(this._listeners);
Expand Down