Tuta Mail · tutanota #10881 · Linux click-to-update notification GC race · Francesco

01 · The problem

The dismisser was discarded the moment showOneShot returned

Tuta Mail desktop ships an Electron auto-updater that posts an OS notification when a new build is available. The notification has a click handler that triggers quitAndInstall. On Linux this handler intermittently stopped firing: the notification appeared, the user clicked, nothing happened. Switching to Settings → Desktop → Update worked as a manual fallback, but the notification path was broken. The bug had been live long enough for the core maintainer to comment that it was "slightly infuriating".

The original showOneShot

// src/applications/common/desktop/notifications/DesktopNotifier.ts
async showOneShot(props: { title: string; body?: string; icon?: NativeImage; onClick?: () => unknown }): Promise<void> {
    const params = { title: props.title, body: props.body, icon: ..., group: "oneshot" }
    const factory = await this.notificationFactory.getAsync()
    if (!factory.isSupported()) throw new Error("Notifications are not supported")

    const onClick = props.onClick ?? noOp
    await this.initPromise.promise

    factory.makeNotification(params, onClick);  // ← return value (Dismisser) DISCARDED
}
      

The factory returns a Dismisser: a closure that retains a reference to the underlying electron.Notification. showOneShot threw that closure away. As soon as the method returned, the only reference to the Notification object went out of scope. V8's mark-and-sweep was then free to collect it before the user could click.

The macOS Cocoa bridge and the Windows Toast binding both retain the JS wrapper through their native objects, so the JS-side collection had no observable effect on those platforms. GTK on Linux behaves differently: the click round-trip from the OS back to the JS handler relies on the JS wrapper still being alive when the click event lands. Once V8 collected the wrapper, the click event had nowhere to go and was silently dropped. The bug surfaced as "sometimes works, sometimes not" because the outcome depended on GC timing relative to the user click.

The asymmetry that made the root cause obvious was right there in the same file. showCountedUserNotification, which handles badge-counted notifications for mail arrivals, already retained its dismisser in notificationDismissersPerUser. Badge-counted clicks never failed. One-shot clicks failed on Linux. Two functions in the same file, one retained the closure, the other discarded it.

02 · The fix

Mirror the retention pattern from the function next door

The fix is structural: keep the dismisser alive in a Set on the DesktopNotifier instance until the user interacts with the notification, then release it. Wrap the user-supplied onClick so the click handler also tears down the OS notification through the retained closure. No public API change. No behavior change on macOS or Windows. The retention path on Linux is the only thing that moves.

Diff

 export class DesktopNotifier {
     private readonly initPromise: DeferredObject<void> = defer()
     private readonly notificationDismissersPerUser: Map<string, Dismisser[]> = new Map()
+    // Retains dismissers for one-shot notifications until the user clicks
+    // one. Without this the JS-side electron.Notification can be collected
+    // before the click event fires, which on Linux/GTK intermittently kills
+    // the click handler (issue #10844).
+    private readonly oneShotDismissers: Set<Dismisser> = new Set()

     async showOneShot(props: { ... }): Promise<void> {
         const params = { ... }
         const factory = await this.notificationFactory.getAsync()
         if (!factory.isSupported()) throw new Error("Notifications are not supported")

-        const onClick = props.onClick ?? noOp
+        const userOnClick = props.onClick ?? noOp

         await this.initPromise.promise

-        factory.makeNotification(params, onClick)
+        // Retain the dismisser so the underlying electron.Notification cannot
+        // be collected before the click event fires. When the click lands we
+        // run the user handler, then dismiss the OS notification through the
+        // retained closure and release the reference so the set stays bounded.
+        let dismisser: Dismisser | undefined
+        const onClick = () => {
+            try {
+                userOnClick()
+            } finally {
+                if (dismisser !== undefined) {
+                    this.oneShotDismissers.delete(dismisser)
+                    const toDismiss = dismisser
+                    dismisser = undefined
+                    toDismiss()
+                }
+            }
+        }
+        dismisser = factory.makeNotification(params, onClick)
+        this.oneShotDismissers.add(dismisser)
     }
 }
      

try { userOnClick() } finally { cleanup } guards against a throwing user handler leaking the retention. The temporary toDismiss local is there so that even if the dismisser itself were to re-enter the click path (it does not, but defense in depth), the oneShotDismissers Set is no longer holding the closure when toDismiss() fires.

Two regression tests, GREEN-RED-GREEN locally

Pre-PR torture run with the existing @tutao/otest + testdouble harness, mirrored into a standalone tsx runner that bypasses the wasm/Rust build prerequisites on Windows. The new tests fail against unpatched master with "Unsatisfied verification on test double: close() wanted 1 time, no invocations" and pass with the fix.

showOneShot retains the notification and closes it when the user clicks. After a simulated click, the user-supplied handler fired AND close() was called once via the retained dismisser. Before the fix close() was never called because the dismisser was discarded.
showOneShot retains dismissers independently for concurrent notifications. Three one-shots in flight at once, click each, each handler fires, each notification is dismissed exactly once.

The four existing DesktopNotifierTest cases (start-gating, grouped notification badge, clearUserNotifications scoping, unsupported-factory short-circuit) continue to pass unmodified. Lint clean. Prettier clean.

03 · The process

From maintainer comment to PR in one session

The ethical gate read all four comments on #10844 before any code was touched. The two desktop-area maintainers (ganthern, charlag) had already triaged the bug and confirmed it. No community contributor was in flight with a fix. Path clear, scope bounded, fix surgical.

Done

Picked the issue where a core maintainer admitted personal annoyance

Issue #10844 had ganthern offering the Settings workaround, charlag (Willow, top Tuta reviewer) replying "For me it is inconsistent, sometimes it works, sometimes not, slightly infuriating", and the reporter confirming a version-regression hypothesis. A bug a core maintainer is personally annoyed by carries strong merge incentive once a clean fix lands. Bounded Electron Notification scope. Linux-specific, but the fix is platform-agnostic.

Done

Root cause via static analysis · GC race identified

Traced the flow from ElectronUpdater.notifyAndInstall through DesktopNotifier.showOneShot down to ElectronNotificationFactory.makeNotification. The factory returns a Dismisser closure that retains the electron.Notification reference. showOneShot discarded it. The asymmetry with the working showCountedUserNotification path in the same file made it concrete.

Done

Pre-PR Torture L1 to L5 · failing test first, then fix

Two regression tests added before the fix. Confirmed RED locally (mock close() never invoked). Applied the retention fix in 27 lines. Re-ran the same harness, GREEN 7/7 (2 new + 5 mirrored pre-existing). Lint clean. Prettier reformatted both files. PR description self-reviewed for the em-dash anti-pattern.

Done

PR opened against master with charlag context cited

The PR body opens with the maintainer's own "sometimes works, sometimes not, slightly infuriating" quote so the reviewer recognizes their own characterization. The fix walks through the GC race, references the existing retention pattern in showCountedUserNotification as the design precedent, and documents the bounded-leak trade-off explicitly. No CLA gate on tutao/tutanota for external contributors (verified against recent merged PRs by qureshi96 and hrb-hub).

Goal

First clean merge with Tuta · relationship anchor for a German E2EE shop

tutao GmbH is a 30-person team in Hannover building E2EE mail and calendar at tuta.com. A clean merge here on a desktop-area Electron fix opens the door to follow-up PRs in the same area (notification stack, updater, IPC) and to a referenceable conversation with the team about my work on the K-Perception stack (E2EE Y.js / IndexedDB local-first), which is exactly the kind of cryptography-aware client engineering Tuta needs more of.

04 · How to verify

Reproduce every claim on this page

# 1. Pull the branch from the fork
git clone https://github.com/999purple999/tutanota --branch fix-oneshot-notification-click-linux

# 2. Inspect the change, fully contained in two files
git diff master -- src/applications/common/desktop/notifications/DesktopNotifier.ts
git diff master -- test/tests/desktop/notifications/DesktopNotifierTest.ts

# 3. Static checks on the patched files
npx eslint src/applications/common/desktop/notifications/DesktopNotifier.ts test/tests/desktop/notifications/DesktopNotifierTest.ts
npx prettier --check src/applications/common/desktop/notifications/DesktopNotifier.ts test/tests/desktop/notifications/DesktopNotifierTest.ts

# 4. Unit tests via the official harness (requires Rust + wasm-pack)
npm install
cd test
node --enable-source-maps test -f DesktopNotifier

# 5. Or via a standalone tsx runner on Windows (no Rust toolchain needed)
#    - import DesktopNotifier + testdouble + LazyLoaded directly
#    - mock NotificationFactory with createdNotifications array (see DesktopNotifierTest setup)
#    - assert close() invoked once on click after retention fix
#    - 7/7 PASS post-fix · 2/7 FAIL pre-fix (the two new #10844 cases)

# 6. Manual verification on a Linux desktop build
#    - Build the desktop client locally per BUILDING.md
#    - Trigger the update flow against a local update server (see ElectronUpdater.ts header)
#    - Click the resulting "Click to update" notification
#    - BEFORE the patch: the click intermittently does nothing (GC dependent)
#    - AFTER the patch: the click always triggers quitAndInstall
    