Repository navigation
fix: run local notification callbacks on the main thread on macOS - #55681
Conversation
…5629) * fix: run local notification callbacks on the main thread on macOS With a nil queue, NSNotificationCenter runs observer blocks on whichever thread posts the notification, and the block called the JS callback there. A background thread would block on the isolate's v8::Locker held by the main thread, which could hang app.quit(). Keep the callbacks in a main-thread-only map and post notifications from other threads to the main thread. Also make the detached-frame cppgc fixture collect with async GCs so conservative stack scanning (notably ASan fake stacks) cannot keep a stale callback holder alive, and disable retries in cpp-heap.spec.ts. Fixes #55392 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: address clang-tidy findings in notification dispatch Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor: track notification subscriptions in a single map Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Backport to 44-x-y: only the macOS fix and the docs note. 44-x-y has no spec/fixtures/api/cppgc-detached-frame.js or native callback holders specs, so the spec changes were dropped. SystemPreferences is not garbage-collected on 44-x-y, so there is no kind map or ClearNotificationSubscriptions() to update; the Subscription struct holds only the observer and the callback, and the old __block copied_callback is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vf2yMwv2LMkF9YULfTdXCy
There was a problem hiding this comment.
I reviewed this backport and didn't find any bugs. It was adapted by hand and changes which thread notification callbacks run on, and the PR says it hasn't been built or tested yet, so a human look would still be worthwhile.
What was reviewed (from a standalone clone, so I couldn't read the Chromium or Foundation sources):
- Registration order: a notification posted on a background thread before the map entry exists reaches
RunNotificationCallbackthrough a posted task. That task runs afterDoSubscribeNotificationreturns on the main thread, so the lookup finds the entry. - Unsubscribing while a task is waiting makes the lookup miss, so the callback doesn't run. Running a copy of the callback lets it unsubscribe itself safely. All map reads and writes happen on the main thread.
- I didn't build this or run any tests, and the PR adds no spec coverage.
Extended reasoning...
The change replaces the global id-to-observer map in shell/browser/api/electron_api_system_preferences_mac.mm with a main-thread-only subscription map, and sends notifications posted on background threads to the main thread's task runner. It also adds a short threading note to docs/api/system-preferences.md. It touches no IPC, permission, or renderer-reachable security surface, and no CODEOWNERS entry covers these files. I deferred because it is a hand-adapted backport of threading-sensitive code. The author left "built and tested" and "npm test passes" unchecked, and the spec changes were dropped.
|
Release Notes Persisted
|
Requested by John Kleinschmidt · Slack thread
Description of Change
Backport of #55629.
See that PR for details.
As #55629 says, this branch takes only the macOS fix and the docs note. The spec changes were dropped because 44-x-y has no
spec/fixtures/api/cppgc-detached-frame.jsor native callback holders specs.electron_api_system_preferences_mac.mmwas adapted by hand: on 44-x-ySystemPreferencesis not garbage-collected, so there is no kind map and noClearNotificationSubscriptions(). TheSubscriptionstruct holds only the observer and the callback, it replacesGetIdMap()and the old__block copied_callback, and dispatch to the main thread works the same as on main.Checklist
npm testpasses🤖 Generated with Claude Code
https://claude.ai/code/session_01Vf2yMwv2LMkF9YULfTdXCy
Release Notes
Notes: Fixed a possible hang on quit on macOS when using
systemPreferences.subscribeLocalNotification,subscribeNotification, orsubscribeWorkspaceNotification.Generated by Claude Code