-
Notifications
You must be signed in to change notification settings - Fork 69
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
Don't send messages to destroyed webContents #11
base: master
Are you sure you want to change the base?
Conversation
This commit: 1. Prevents sending messages to destroyed webContents (which result in a crash) 2. Allows registering multiple webContents for push notifications We need this because we are listening for notifcations in a specific webContents. If it crashes we create a new one and call setup again, without this change the next push notification that came in would cause a crash.
cc @MatthieuLemoine I tried to make the minimal change to make this work, but I think I'd like to do some refactoring later on to make it s.t. each call to setup is isolated from one another, what do you think? |
That seems to be crashing the app ref: electron/electron#11797
Hit a weird electron crash, so I removed the call to |
webContents.send(NOTIFICATION_SERVICE_ERROR, e.message); | ||
// Forward error to the renderer processes | ||
send(NOTIFICATION_SERVICE_ERROR, e.message); | ||
starting = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
starting
is never set to false if no error is thrown. Is it intended ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - in that case started
should be true, so it would return around line 44 anyway.
Hey it seems like the crash I was experiencing might be unrelated to this project, doing some more digging now... |
Okay - the other crash I was getting was electron/electron#11797 I think this change is still needed to prevent the main process from crashing when the webContents that's setup to receive notifications crashes... |
This commit:
We need this because we are listening for notifcations in a specific
webContents. If it crashes we create a new one and call setup again,
without this change the next push notification that came in would cause
a crash.