Skip to content
This repository has been archived by the owner on Mar 7, 2023. It is now read-only.

Null check on currentDevice #32

Closed
wants to merge 6 commits into from

Conversation

reidmweber
Copy link
Contributor

We've been using this in an electron app running in Windows. We were getting a crash about 5-10 minutes into launching the app. We've only been able to reproduce it on one PC. Somehow NotifyRemoved in detection.cpp is getting called with a null device and causing a null ptr exception. The PC has a touchscreen that isn't working so it may be that the touchscreen device is continually connecting and disconnecting to create this scenario.
In any case, this change has fixed the crash for us.

Copy link
Owner

@MadLittleMods MadLittleMods left a comment

Choose a reason for hiding this comment

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

We probably also need to add these changes to the mac and linux files as well.

@@ -107,10 +107,14 @@ void NotifyAsync(uv_work_t* req) {
void NotifyFinished(uv_work_t* req) {
if (isRunning) {
if(isAdded) {
NotifyAdded(currentDevice);
if (currentDevice != NULL) {
Copy link
Owner

Choose a reason for hiding this comment

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

currentDevice !== null

}
else {
NotifyRemoved(currentDevice);
if (currentDevice != NULL) {
Copy link
Owner

Choose a reason for hiding this comment

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

currentDevice !== null

@@ -1,6 +1,6 @@
{
"name": "usb-detection",
"version": "1.4.0",
"version": "1.4.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not bump the version in the PR. We can start a release once this gets into master.

@reidmweber
Copy link
Contributor Author

I made those changes to the other platforms. It would be a smaller change if we just put the check in detection.cpp. VS Code cleaned up a bunch of spaces at the end of lines. Let me know if you want me to back those out.

@reidmweber
Copy link
Contributor Author

I think this is ready to go. I have tested it on Mac and Windows. I don't have a linux env to test it on.

@MadLittleMods
Copy link
Owner

@reidmweber Sorry for the delay. My timeline is to review this again over the upcoming weekend. Keep with the pings if it slips.

@MadLittleMods
Copy link
Owner

MadLittleMods commented Mar 14, 2017

@reidmweber

I have merged the changes via these two PRs

Thanks for the contribution 😀

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.

2 participants