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
[Bug]: Electron crashes with libnotify 0.8.3 in portal environment when closing notification #40461
Comments
libnotify 0.8.x, before 0.8.3 is affected by another bug: https://gitlab.gnome.org/GNOME/libnotify/-/issues/34. But libnotify 0.8.3 has fixed its own bug, as I guess it's time for Electron to fix this unexpected Workaround for app dev: Don't use |
Reported to electron upstream as electron/electron#40461.
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment! |
I'm sure this bug still exists in electron 28.2.1 and 29.0.0-beta.6. |
just tested it in 28.2.3, there it is still an issue |
You can also work around this crash by forcing libnotify to use dbus instead of the portal with the env var You need to add this to your flatpak build file: finish-args:
- --talk-name=org.freedesktop.Notifications
- --env=NOTIFY_IGNORE_PORTAL=1 example from my project: flathub/chat.delta.desktop@cf25cd0 |
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes electron#40461.
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461.
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: taoky <me@taoky.moe>
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: taoky <me@taoky.moe>
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: taoky <me@taoky.moe>
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: taoky <me@taoky.moe>
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: taoky <me@taoky.moe>
Callers of Notification::Dismiss() assume that the notification instance is not deleted after the call, but this was not the case for LibnotifyNotification: - Destroy() would get `this` deleted. - notify_notification_close() in portal environment triggers LibnotifyNotification::OnNotificationClosed(), and finally calls Destroy() This patch removes all Destroy() in Dismiss(), and adds a boolean to tell whether notify_notification_close() is running, to avoid crash under portal environment. Fixes #40461. Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: taoky <me@taoky.moe>
Hi @taoky I'm using electron
What's strange, I checked the Senry reports and I see the blow stack trace (
Sentry report!OS Version: Windows 10.0.22621 (3296) Report Version: 104Crashed Thread: 9112 Application Specific Information: Thread 9112 Crashed: Thread 6964 name: LoaderLockSampler Thread 22016 name: BrokerEvent Thread 17492 name: ThreadPoolServiceThread Thread 23100 name: ThreadPoolForegroundWorker Thread 4336 name: ThreadPoolBackgroundWorker Thread 17816 name: ThreadPoolForegroundWorker Thread 17912 name: Chrome_IOThread Thread 18328 name: MemoryInfra Thread 21708 Thread 13592 Thread 9308 Thread 17272 Thread 18824 Thread 6140 Thread 18052 Thread 23316 Thread 15200 Thread 21400 name: ThreadPoolSingleThreadCOMSTASharedForeground0 Thread 11924 Thread 10584 Thread 18816 Thread 19804 name: ThreadPoolForegroundWorker Thread 22756 name: ThreadPoolForegroundWorker Thread 14248 name: CompositorTileWorker1 Thread 23084 name: Chrome_InProcGpuThread Thread 9240 name: Chrome_ChildIOThread Thread 22344 name: VizCompositorThread Thread 9608 name: VideoCaptureThread Thread 4220 Thread 17676 name: ThreadPoolSingleThreadCOMSTASharedForegroundBlocking1 Thread 18088 name: ThreadPoolSingleThreadForegroundBlocking2 Thread 22496 name: ThreadPoolForegroundWorker Thread 1856 name: ThreadPoolSingleThreadSharedForeground3 Thread 20984 name: ThreadPoolForegroundWorker Thread 12728 name: ThreadPoolForegroundWorker Thread 16608 name: CacheThread_BlockFile Thread 17968 name: DManip Delegate Thread Thread 17436 Thread 15132 name: ThreadPoolBackgroundWorker Thread 18508 name: ThreadPoolSingleThreadSharedBackgroundBlocking4 Thread 16240 Thread 21040 Is the above stack trace related to #40461 and upgrading electron to Thanks in advance! |
Hi @taoky You're right, it's another issue! I'm able to reproduce the issue on my machine (unfortunately, couldn't create an election Fiddle app that reproduces the issue to open a new issue on GitHub to electron team).
but basically, the scenario to reproduce the crash on my app is as below:
I found a workaround to avoid that crash by creating the notification inside
|
Preflight Checklist
Electron Version
25.0.0, 27.0.3 and 28.0.0-beta.2
What operating system are you using?
Other Linux
Operating System Version
uname -a
:Linux hostname 6.5.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Tue, 10 Oct 2023 21:10:21 +0000 x86_64 GNU/Linux
What arch are you using?
x64
Last Known Working Electron version
Not sure
Expected Behavior
Electron does not crash when running
notification.close()
.Actual Behavior
Electron crashes with
notification.close()
when in portal (flatpak/snap, orNOTIFY_FORCE_PORTAL=1
is set).Testcase Gist URL
No response
Additional Information
An MRE modified from electron-quick-start: https://github.com/taoky/electron-libnotify-portal-crash-mre.
Screencast:
Kooha-2023-11-07-04-23-16.mp4
My analysis:
LibnotifyNotification::Show()
binds "closed" glib signal toLibnotifyNotification::OnNotificationClosed
electron/shell/browser/notifications/linux/libnotify_notification.cc
Lines 91 to 94 in 7999ea3
LibnotifyNotification::Dismiss()
is called which runslibnotify_loader_.notify_notification_close()
:electron/shell/browser/notifications/linux/libnotify_notification.cc
Line 167 in 7999ea3
notify_notification_close()
, as libnotify 0.8.x adds supports for portal environment, it will callremove_portal_notification()
instead of using gdbus call (https://gitlab.gnome.org/GNOME/libnotify/-/blob/0.8.3/libnotify/notification.c#L1761), then it callsclose_notification()
(https://gitlab.gnome.org/GNOME/libnotify/-/blob/0.8.3/libnotify/notification.c#L825), and nightmare happens: itg_signal_emit (notification, signals[SIGNAL_CLOSED], 0);
(https://gitlab.gnome.org/GNOME/libnotify/-/blob/0.8.3/libnotify/notification.c#L707).LibnotifyNotification::OnNotificationClosed()
happily callsNotificationDismissed()
, which finally callsDestroy()
, and thenpresenter()->RemoveNotification(this)
, andthis
is thendelete
d inside.Notification::Close()
in electron_api_notification.cc has no idea aboutnotification_
being deleted, it continues, and crashes:https://github.com/electron/electron/blob/7999ea39e2ee9702e64e4125e8e89c314e9f468f/shell/browser/api/electron_api_notification.cc#L217C6-L227
The text was updated successfully, but these errors were encountered: