Fix duplicated and crashing DBus tray icon - #21980
Conversation
MrJul
added
bug
os-linux
backport-candidate-12.1.x
|
Thank you for your contribution. Please follow our AI Guidelines and trim down the PR description and code comments. |
|
|
You can test this PR using the following package version. |
|
@cla-avalonia agree |
The StatusNotifierItem object was exported before the well-known name was requested, and the name was released and re-taken with a new instance id whenever the watcher came back. A host that scans the bus then finds the object on a connection owning no such name and registers it under the unique connection name, so RegisterStatusNotifierItem adds a second entry for the same process. The object is now exported only while the name is owned, and the name is kept for the lifetime of the icon. WatchAsync and CreateTrayIcon are async void: the OperationCanceledException from disposal escaped onto the dispatcher and terminated the process, as would any failing DBus call during registration. Both are handled now.
bb72bf5 to
aec2e66
Compare
|
You can test this PR using the following package version. |
| if (!_sysTrayServiceNameOwned) | ||
| { | ||
| // Set before awaiting, so that a concurrent hide queues its ReleaseName instead of skipping it. | ||
| _sysTrayServiceNameOwned = true; |
There was a problem hiding this comment.
_sysTrayServiceNameOwned is set before ownership is acquired. if invoke a failing RequestNameAsync: the exception is logged by the outer catch, but this flag remains true. Every subsequent CreateTrayIcon() therefore skips name acquisition and attempts to register using a name the connection does not own.
Could the in-flight request be tracked separately, with _sysTrayServiceNameOwned set only after successful acquisition?
There was a problem hiding this comment.
Good catch, fixed in fdf755a. The flag is gone; the request task itself is now the state, which distinguishes in-flight, acquired and failed:
_sysTrayServiceNameRequest ??= _connection.RequestNameAsync(...)thenawaitit — the happy path is unchanged.- the catch clears the field unless the task ran to completion, so a refused name is asked for again on the next attempt, while a failure in
RegisterStatusNotifierItemafterwards leaves ownership alone. ReleaseTrayServiceNametakes the task, clears the field and awaits it before callingReleaseName— that is what the early flag was there for (a hide racing the request), and it no longer releases a name that was never acquired.
The flag was set before awaiting RequestName, so a refused acquisition left it true: every later CreateTrayIcon skipped the request and registered the item under a name the connection does not own. Keeping the request task tells the three states apart - in flight, acquired, failed - so a failure is retried, and a hide racing the request now waits for it before releasing the name instead of releasing one that is not owned yet.
|
You can test this PR using the following package version. |
Its entry conditions were only checked before the first await, so a hide or a dispose while the bus answered RequestName still ran into the export: the release queued on that same task runs after this continuation, leaving the object exported under a name no longer owned - the state the duplicate item grows from. The conditions are re-read after each await, and the request is compared by reference so a stale continuation gives way to a newer one. Releasing the name is now awaited before requesting it again: the bus keeps a name registered until the release is acknowledged, and since the name is reused for the lifetime of the icon, a show right after a hide would otherwise fail outright and leave no icon until the next toggle.
|
Pushed db0484e, closing two more races I found while re-reading the registration path:
Also narrowed the catch that drops a refused request: it now only clears the request this call made, and only if it never completed. |
|
@bolikcraft please remind your ai agent to reduce the unnecessary comments. direct it to write docs in ASD-STE100 Simplified Technical English. |
The comments explained the same rule more than one time and used long sentences. Each comment is now one or two short sentences in ASD-STE100 style. The code does not change.
|
Ok, fixed in 5cd6efe: fewer comments, ASD-STE100 style. |
|
You can test this PR using the following package version. |
|
@jsuarezruiz the flag issue is fixed in fdf755a — the name request task is now the state. Two more races are closed in db0484e, and the comments are shortened in 5cd6efe. All checks pass. Please look at it again when you have time. |
|
|
||
| // CreateTrayIcon exports this object when the connection owns the name. | ||
| _statusNotifierItemDbusObj = new StatusNotifierItemDbusObj(_connection, dbusMenuPath); | ||
| _connection.AddMethodHandler(_statusNotifierItemDbusObj); |
There was a problem hiding this comment.
Confirmed this is the root cause of the duplicate. On main, the object can be inspected through the connection immediately after construction, even before the name is owned. On this branch, it isn’t exposed until the name is acquired. Good catch.
| _statusNotifierItemDbusObj.SetIcon(_icon); | ||
| // Export the object only while the connection owns the name. If not, a host that scans | ||
| // the bus adds a second item. | ||
| _connection.AddMethodHandler(_statusNotifierItemDbusObj); |
There was a problem hiding this comment.
If SetIsVisible toggles quickly (for exampke, false/true/false/true in one turn), two CreateTrayIcon calls end up running at once, both wait on the same release/request, and both reach AddMethodHandler. The second one throws "A method handler is already registered for '/StatusNotifierItem'" and logs "Unable to register the system tray icon."
It is just an error in the log, but it points to CreateTrayIcon not being safe to re enter. On main this never happened because it removed the handler before re adding it.
We could improve with a small guard so re entry skips the add instead of throwing.
6 participants
What does the pull request do?
Fixes two independent defects in
DBusTrayIconImpl.Duplicate icon (#21978). The
/StatusNotifierItemobject was exported before any well-known name was requested, and the name was released and re-taken with a new instance id whenever the watcher reappeared. A host that scans the bus for items (gnome-shell-extension-appindicator does this on every start) finds the object on a connection owning noorg.kde.StatusNotifierItem-*name and keys the item by the unique connection name, while theRegisterStatusNotifierItemcall that follows is keyed by the well-known name — two entries, two icons. The object is now exported only while the name is owned, and the name keeps its instance id for the lifetime of the icon. It is deliberately not released when the watcher goes away, so a restarting host always sees object and name together; hiding still releases it, which is the only part of hiding a host can observe.Crash on disposal (#21979).
WatchAsyncisasync voidand awaits a cancellable delay.Disposecancels the token and only then sets_isDisposed, but the continuation is posted to the dispatcher and runs afterDisposereturned, so neither existing exception filter matches and theOperationCanceledExceptionis rethrown on the dispatcher, aborting the process (exit code 134). It is now caught explicitly.CreateTrayIconisasync voidtoo and had no handler at all, so a DBus call failing mid-registration ended the same way; its body is guarded now.No public API changes.
What is the updated/expected behavior with this PR?
One tray icon per application, surviving any number of host restarts; disposal and application shutdown complete silently.
SetIsVisible(false)still removes the icon, andSetIsVisible(true)brings back the same item instead of a new one.Testing
Manually verified on GNOME Shell 48.5 with
appindicatorsupport@rgcjonas.gmail.com, against both an unpatched (12.1.0) and a patched build of the same test application, pollingorg.kde.StatusNotifierWatcher.RegisteredStatusNotifierItems: disabling and re-enabling the extension repeatedly, replacing the icon throughTrayIcon.SetIcons, shutting the application down, and hide/show cycles (including one racingReleaseNameagainstRequestName). Unpatched runs show the duplicate entry and the 134 exit; patched runs keep exactly one entry under a stable name and exit 0. Menu, tooltip, icon andActivatestill work after a hide/show cycle. Not covered by unit tests: the behaviour only shows against a live session bus and a StatusNotifier host.Checklist
Breaking changes
None.
Fixed issues
Fixes #21978
Fixes #21979
Related to #13130 (same registration path, but that report is about the item being lost when the host restarts without the watcher name going away — not addressed here).