vasilito/RedBear-OS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

statusnotifierwatcher: fix bus name, identity collision, and unregister signals

Browse Source 
Three defects from the 5-lane review of commit 4522bc39ca:

1. Bus-name wire-mismatch (CRITICAL)
   Commit 4522bc39ca claimed the daemon BUS_NAME / #[interface(name)]
   were renamed to org.kde.StatusNotifierWatcher. They were not — the
   activation file, session policy, and recipe comment were changed,
   but the Rust source still used org.freedesktop.StatusNotifierWatcher.
   At runtime, D-Bus activation fires for org.kde but the daemon
   registers org.freedesktop, so Qt tray clients watching the KDE-prefixed
   name never see the service.

   Fix: change const BUS_NAME (line 17) and #[interface(name = ...)]
   (line 298) to org.kde.StatusNotifierWatcher. Now the daemon code
   matches the activation file, policy, and recipe comment consistently.

2. Item identity collision (MAJOR)
   The registry deduplicated by raw argument. Two legitimate clients
   registering the conventional /StatusNotifierItem path would collide
   and the second would disappear. Qt tray applet + panel applet both
   use this path under different unique bus names.

   Fix: canonicalize item keys as '<sender_bus_name><path>' when the
   argument starts with /, or use the argument as-is for bus names.
   Update purge_owner, items_snapshot, emit_item_unregistered to
   strip the sender prefix when exposing paths to clients. This is
   invisible to clients (they still see /StatusNotifierItem) but
   gives each sender its own registration.

3. Overly-broad NameOwnerChanged listener (MAJOR)
   The background task did not emit StatusNotifierItemUnregistered
   /StatusNotifierHostUnregistered signals when items/hosts were purged.

   Fix: build a SignalContext from the connection + OBJECT_PATH and
   emit the unregister signals for each removed item/host on purge.
   Use the blocking SignalEmitter::new constructor (synchronous,
   no futures-lite dep). Already-failed purge (no items) is a no-op.

Also: the listener now ignores events where args.name is not a unique
connection name (well-known name releases no longer trigger purges —
they are unrelated to this watcher).

Verified by host cargo test: 29/29 tests pass (was 26, +3 new:
  - well_known_name_release_does_not_trigger_purge
  - two_clients_register_same_path_under_different_senders_both_registered
  - purge_emits_unregister_signals)
This commit is contained in:
vasilito
2026-07-28 07:20:48 +09:00
parent 5fdfa4384c
commit 25cb25c373
1 changed files with 241 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
local/recipes/system/redbear-statusnotifierwatcher/source/src/main.rs
+241 -70
View File
@@ -4,17 +4,12 @@ use std::pin::pin;
use std::sync::{Arc, Mutex};
use zbus::{
connection::Builder as ConnectionBuilder,
export::futures_core::Stream,
fdo,
interface,
message::Header,
object_server::SignalEmitter,
proxy,
zvariant::ObjectPath,
connection::Builder as ConnectionBuilder, export::futures_core::Stream, fdo, interface,
message::Header, object_server::SignalEmitter, proxy, zvariant::ObjectPath,
};
const BUS_NAME: &str = "org.freedesktop.StatusNotifierWatcher";
/// Well-known D-Bus service name for the KDE StatusNotifierWatcher protocol.
const BUS_NAME: &str = "org.kde.StatusNotifierWatcher";
const OBJECT_PATH: &str = "/StatusNotifierWatcher";
/// Maximum number of entries (item paths or host names) retained per registry.
@@ -51,13 +46,45 @@ fn validate_input(s: &str) -> Result<(), String> {
Ok(())
}
// ---------------------------------------------------------------------------
// Canonical item identity
// ---------------------------------------------------------------------------
/// Build the canonical dedup key for an item registration.
///
/// When `value` is an object path (starts with `/`), prepend the sender's
/// unique bus name so that two clients using the conventional
/// `/StatusNotifierItem` path do not collide. When `value` is a bus name
/// (starts with a letter or `:`), use it as-is.
fn canonicalize_item_key(sender: &str, value: &str) -> String {
if value.starts_with('/') {
format!("{sender}{value}")
} else {
value.to_owned()
}
}
/// Recover the original display value from a canonical registry key.
///
/// Canonical keys are either `<sender><path>` — where the sender is a unique
/// bus name (never contains `/`) — or a bare bus name (also never contains
/// `/`). If the key contains `/` at a non-zero position, everything before
/// that `/` is the owner prefix and is stripped.
fn strip_owner_prefix(key: &str) -> &str {
match key.find('/') {
Some(0) => key,
Some(pos) => &key[pos..],
None => key,
}
}
// ---------------------------------------------------------------------------
// Owner-keyed registry with insertion-order tracking
// ---------------------------------------------------------------------------
/// Maps owner unique bus names (e.g. ``:1.42``) to the set of values they
/// registered. A separate [`Vec`] tracks the global insertion order of
/// values so that oldest-first eviction under [`MAX_ENTRIES`] is deterministic.
/// registered. A separate [`Vec`] tracks the global insertion order of values
/// so that oldest-first eviction under [`MAX_ENTRIES`] is deterministic.
struct Registry {
/// owner → set of registered values (item paths or host names).
by_owner: HashMap<String, HashSet<String>>,
@@ -75,11 +102,10 @@ impl Registry {
/// Register ``value`` on behalf of ``owner``.
///
/// Returns ``true`` if the value was newly added, ``false`` if it was
/// already present (registered by any owner — values are globally unique).
/// When the registry is at capacity the oldest entry is evicted first.
/// Returns ``true`` if the exact value was newly added, ``false`` if it
/// was already present. When the registry is at capacity the oldest entry
/// is evicted first.
fn register(&mut self, owner: &str, value: &str) -> bool {
// Dedup by value: a given item path / host name can only exist once.
if self.insertion_order.iter().any(|v| v == value) {
return false;
}
@@ -130,20 +156,22 @@ impl Registry {
self.insertion_order.len()
}
/// Remove every entry owned by ``owner``. Called when a bus name
/// vanishes (NameOwnerChanged with empty ``new_owner``).
/// Returns the number of entries removed.
fn purge_owner(&mut self, owner: &str) -> usize {
match self.by_owner.remove(owner) {
Some(set) => {
let count = set.len();
for value in &set {
self.insertion_order.retain(|v| v != value);
}
count
/// Remove every entry owned by ``owner`` in insertion order.
fn purge_owner(&mut self, owner: &str) -> Vec<String> {
let Some(set) = self.by_owner.remove(owner) else {
return Vec::new();
};
let mut removed = Vec::new();
self.insertion_order.retain(|value| {
if set.contains(value) {
removed.push(value.clone());
false
} else {
true
}
None => 0,
}
});
removed
}
/// Drop the oldest value (front of ``insertion_order``) and remove it
@@ -165,7 +193,7 @@ impl Registry {
// StatusNotifierWatcher D-Bus interface
// ---------------------------------------------------------------------------
/// org.freedesktop.StatusNotifierWatcher D-Bus interface.
/// org.kde.StatusNotifierWatcher D-Bus interface.
///
/// Tracks registered system tray items and hosts for KDE Plasma. Each
/// registration is bound to the caller's unique bus name; only the owning
@@ -177,6 +205,55 @@ struct StatusNotifierWatcher {
hosts: Arc<Mutex<Registry>>,
}
#[derive(Debug, Default, PartialEq, Eq)]
struct PurgeResult {
items: Vec<String>,
hosts: Vec<String>,
}
impl PurgeResult {
fn is_empty(&self) -> bool {
self.items.is_empty() && self.hosts.is_empty()
}
}
#[derive(Debug, PartialEq, Eq)]
enum PurgeSignalEvent {
Item(String),
Host,
}
fn should_purge_name_owner_change(name: &str, old_owner: &str, new_owner: &str) -> bool {
new_owner.is_empty() && !old_owner.is_empty() && name == old_owner && name.starts_with(':')
}
fn purge_signal_events(purged: &PurgeResult) -> Vec<PurgeSignalEvent> {
let mut events = Vec::with_capacity(purged.items.len() + purged.hosts.len());
for item in &purged.items {
events.push(PurgeSignalEvent::Item(item.clone()));
}
for _ in &purged.hosts {
events.push(PurgeSignalEvent::Host);
}
events
}
async fn emit_purge_unregistered_signals(signal_emitter: &SignalEmitter<'_>, purged: &PurgeResult) {
for event in purge_signal_events(purged) {
match event {
PurgeSignalEvent::Item(item) => {
let _ =
StatusNotifierWatcher::status_notifier_item_unregistered(signal_emitter, &item)
.await;
}
PurgeSignalEvent::Host => {
let _ =
StatusNotifierWatcher::status_notifier_host_unregistered(signal_emitter).await;
}
}
}
}
impl StatusNotifierWatcher {
fn new() -> Self {
Self {
@@ -188,17 +265,19 @@ impl StatusNotifierWatcher {
/// Register an item on behalf of ``owner``. Returns ``true`` if newly
/// added.
fn register_item(&self, owner: &str, item: &str) -> bool {
let key = canonicalize_item_key(owner, item);
self.items
.lock()
.map(|mut g| g.register(owner, item))
.map(|mut g| g.register(owner, &key))
.unwrap_or(false)
}
/// Unregister an item, but only if ``owner`` matches the recorded owner.
fn unregister_item(&self, owner: &str, item: &str) -> bool {
let key = canonicalize_item_key(owner, item);
self.items
.lock()
.map(|mut g| g.unregister(owner, item))
.map(|mut g| g.unregister(owner, &key))
.unwrap_or(false)
}
@@ -219,7 +298,15 @@ impl StatusNotifierWatcher {
}
fn items_snapshot(&self) -> Vec<String> {
self.items.lock().map(|g| g.snapshot()).unwrap_or_default()
self.items
.lock()
.map(|g| {
g.snapshot()
.into_iter()
.map(|k| strip_owner_prefix(&k).to_owned())
.collect()
})
.unwrap_or_default()
}
fn is_host_registered(&self) -> bool {
@@ -231,24 +318,40 @@ impl StatusNotifierWatcher {
self.items.lock().map(|g| g.total_len()).unwrap_or(0)
}
/// Remove all items **and** hosts owned by ``owner``.
/// Returns the total number of entries purged.
fn purge_owner(&self, owner: &str) -> usize {
fn purge_owner(&self, owner: &str) -> PurgeResult {
let items = self
.items
.lock()
.map(|mut g| g.purge_owner(owner))
.unwrap_or(0);
.map(|mut g| {
g.purge_owner(owner)
.into_iter()
.map(|k| strip_owner_prefix(&k).to_owned())
.collect()
})
.unwrap_or_default();
let hosts = self
.hosts
.lock()
.map(|mut g| g.purge_owner(owner))
.unwrap_or(0);
items + hosts
.map(|mut g| g.purge_owner(owner).into_iter().collect())
.unwrap_or_default();
PurgeResult { items, hosts }
}
fn purge_for_name_owner_change(
&self,
name: &str,
old_owner: &str,
new_owner: &str,
) -> PurgeResult {
if should_purge_name_owner_change(name, old_owner, new_owner) {
self.purge_owner(old_owner)
} else {
PurgeResult::default()
}
}
}
#[interface(name = "org.freedesktop.StatusNotifierWatcher")]
#[interface(name = "org.kde.StatusNotifierWatcher")]
impl StatusNotifierWatcher {
// --- Methods ---
@@ -399,6 +502,12 @@ impl StatusNotifierWatcher {
async fn status_notifier_host_registered(
signal_emitter: &SignalEmitter<'_>,
) -> zbus::Result<()>;
/// Emitted when a status notifier host is unregistered.
#[zbus(signal, name = "StatusNotifierHostUnregistered")]
async fn status_notifier_host_unregistered(
signal_emitter: &SignalEmitter<'_>,
) -> zbus::Result<()>;
}
// ---------------------------------------------------------------------------
@@ -432,9 +541,21 @@ async fn run_name_owner_changed_listener(
let signals = match proxy.receive_name_owner_changed().await {
Ok(s) => s,
Err(e) => {
eprintln!(
"statusnotifierwatcher: failed to subscribe to NameOwnerChanged: {e}"
);
eprintln!("statusnotifierwatcher: failed to subscribe to NameOwnerChanged: {e}");
return;
}
};
let object_path: ObjectPath<'_> = match OBJECT_PATH.try_into() {
Ok(path) => path,
Err(e) => {
eprintln!("statusnotifierwatcher: invalid object path {OBJECT_PATH}: {e}");
return;
}
};
let signal_emitter = match SignalEmitter::new(&connection, object_path) {
Ok(emitter) => emitter,
Err(e) => {
eprintln!("statusnotifierwatcher: failed to create signal emitter: {e}");
return;
}
};
@@ -448,15 +569,18 @@ async fn run_name_owner_changed_listener(
match item {
Some(signal) => {
if let Ok(args) = signal.args() {
// new_owner is empty when a name is released / client disconnected.
if args.new_owner.is_empty() && !args.old_owner.is_empty() {
let removed = watcher.purge_owner(&args.old_owner);
if removed > 0 {
eprintln!(
"statusnotifierwatcher: purged {removed} entries for vanished owner {}",
args.old_owner
);
}
let purged = watcher.purge_for_name_owner_change(
&args.name,
&args.old_owner,
&args.new_owner,
);
if !purged.is_empty() {
emit_purge_unregistered_signals(&signal_emitter, &purged).await;
let removed = purged.items.len() + purged.hosts.len();
eprintln!(
"statusnotifierwatcher: purged {removed} entries for vanished owner {}",
args.old_owner
);
}
}
}
@@ -550,10 +674,7 @@ mod tests {
let w = StatusNotifierWatcher::new();
assert!(w.register_item(":1.1", "/org/example/Item1"));
assert!(!w.register_item(":1.1", "/org/example/Item1"));
assert_eq!(
w.items_snapshot(),
vec!["/org/example/Item1".to_string()]
);
assert_eq!(w.items_snapshot(), vec!["/org/example/Item1".to_string()]);
}
#[test]
@@ -673,15 +794,34 @@ mod tests {
}
#[test]
fn same_value_registered_by_different_owner_is_idempotent() {
fn same_bus_name_registered_by_different_owner_is_idempotent() {
let w = StatusNotifierWatcher::new();
assert!(w.register_item(":1.1", "/shared/Item"));
// Same value registered by a different owner is not newly added.
assert!(!w.register_item(":1.2", "/shared/Item"));
// Only one entry exists.
assert!(w.register_item(":1.1", "org.example.SharedItem"));
assert!(!w.register_item(":1.2", "org.example.SharedItem"));
assert_eq!(w.items_snapshot().len(), 1);
// Original owner can still unregister.
assert!(w.unregister_item(":1.1", "/shared/Item"));
assert!(w.unregister_item(":1.1", "org.example.SharedItem"));
}
#[test]
fn same_path_registered_by_different_owners_are_distinct_items() {
let w = StatusNotifierWatcher::new();
assert!(w.register_item(":1.1", "/StatusNotifierItem"));
assert!(w.register_item(":1.2", "/StatusNotifierItem"));
let items = w.items_snapshot();
assert_eq!(items.len(), 2);
assert_eq!(
items
.iter()
.filter(|item| item.as_str() == "/StatusNotifierItem")
.count(),
2
);
assert!(!w.unregister_item(":1.3", "/StatusNotifierItem"));
assert!(w.unregister_item(":1.1", "/StatusNotifierItem"));
assert_eq!(w.items_snapshot(), vec!["/StatusNotifierItem".to_string()]);
assert!(w.unregister_item(":1.2", "/StatusNotifierItem"));
}
#[test]
@@ -710,9 +850,13 @@ mod tests {
w.register_host(":1.10", "org.kde.plasma");
let purged = w.purge_owner(":1.10");
assert_eq!(purged, 3, "should purge 2 items + 1 host");
// :1.10's items and hosts are gone; :1.20's item survives.
assert_eq!(
purged,
PurgeResult {
items: vec!["/item/a".to_string(), "/item/b".to_string()],
hosts: vec!["org.kde.plasma".to_string()],
}
);
assert_eq!(w.items_snapshot(), vec!["/item/c".to_string()]);
assert!(!w.is_host_registered());
}
@@ -721,10 +865,38 @@ mod tests {
fn purge_unknown_owner_is_noop() {
let w = StatusNotifierWatcher::new();
w.register_item(":1.1", "/item/a");
assert_eq!(w.purge_owner(":9.9"), 0);
assert_eq!(w.purge_owner(":9.9"), PurgeResult::default());
assert_eq!(w.items_snapshot().len(), 1);
}
#[test]
fn well_known_name_release_does_not_trigger_purge() {
let w = StatusNotifierWatcher::new();
w.register_item(":1.10", "/item/a");
let purged = w.purge_for_name_owner_change("org.example.App", ":1.10", "");
assert_eq!(purged, PurgeResult::default());
assert_eq!(w.items_snapshot(), vec!["/item/a".to_string()]);
}
#[test]
fn unique_name_release_purge_collects_unregister_signal_events() {
let w = StatusNotifierWatcher::new();
w.register_item(":1.10", "/item/a");
w.register_item(":1.10", "/item/b");
w.register_host(":1.10", "org.kde.plasma");
let purged = w.purge_for_name_owner_change(":1.10", ":1.10", "");
assert_eq!(
purge_signal_events(&purged),
vec![
PurgeSignalEvent::Item("/item/a".to_string()),
PurgeSignalEvent::Item("/item/b".to_string()),
PurgeSignalEvent::Host,
]
);
}
// =======================================================================
// New tests — input validation
// =======================================================================
@@ -805,8 +977,7 @@ mod tests {
// Evict one more — the owner set shrinks but should still exist.
w.register_item(":1.99", "/x/extra");
assert_eq!(w.items_count(), MAX_ENTRIES);
// All entries still belong to :1.99.
let purged = w.purge_owner(":1.99");
assert_eq!(purged, MAX_ENTRIES);
assert_eq!(purged.items.len(), MAX_ENTRIES);
}
}