Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix: do not crash when using a closed fs event watcher #70

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 28, 2018

Resolves electron/electron#14844.

This PR backports nodejs/node#20985 to 3-0-x.

The original fix went out in v10.5.0, so we don't need to backport it anywhere else since we'll cut 4-0-x with at least Node v10.10.0.

/cc @pfrazee

Verified

This commit was signed with the committer’s verified signature. The key has expired.
weihanglo Weihang Lo
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Looks great!

@codebytere codebytere merged commit fcaf11a into electron-node-v10.2.0-3.0.x Sep 28, 2018
@codebytere codebytere deleted the fix-fs-issue branch September 28, 2018 17:39
@bpasero
Copy link
Contributor

bpasero commented Mar 21, 2019

@codebytere hey, you might want to look at microsoft/vscode#70566 (comment) where a user in VSCode sees that crash in the picture. Could it be that checking for null is not good because it could be undefined too? Any reason for this change and divergence from the node.js original implementation?

@bpasero
Copy link
Contributor

bpasero commented Mar 21, 2019

Ah ok, looks like this came in as a backport. Maybe Electron should follow up and check if a more recent patch is available that avoids this issue.

@bpasero
Copy link
Contributor

bpasero commented Mar 22, 2019

I am actually clueless how the crash in microsoft/vscode#70566 (comment) can happen. Looking at the code it seems like this._handle is either null or has its value ❓ .

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants