Skip to content

wayland/idle_notify: re-subscribe isIdle when the notification is replaced - #1097

Open
voglster wants to merge 1 commit into
quickshell-mirror:masterfrom
voglster:fix/idle-monitor-notification-aba
Open

voglster wants to merge 1 commit into
quickshell-mirror:masterfrom
voglster:fix/idle-monitor-notification-aba

Conversation

@voglster

@voglster voglster commented Sep 7, 2026

Copy link
Copy Markdown

Fixes #938.

IdleMonitor goes permanently silent after a runtime write to timeout. The
notification is recreated correctly — the interesting part is that nobody is
listening to it.

Cause

updateNotification frees the old notification and allocates the replacement
within the same call, so the allocator routinely returns the block just freed:

quickshell.wayland.idle_notify: Destroyed IdleNotification(0x7fda3454e210)
quickshell.wayland.idle_notify: Created   IdleNotification(0x7fda3454e210) with timeout: 20000

bNotification is only ever written once, at scope exit, and the new pointer
compares equal to the old one. QObjectBindableProperty::setValue returns
early on an equal value without calling notify(), so the bIsIdle binding
installed in onPostReload — which reads bNotification and then reaches
through it to that notification's own bIsIdle — is never re-evaluated and
never subscribes to the replacement. When the new notification receives
idled, nothing propagates.

The old notification = nullptr assigned the local variable, not the property,
so the property never observed an intermediate state.

This is why toggling enabled recovers a wedged monitor: that path is two
updateNotification calls with a real nullptr written to the property in
between.

Note the old code was only correct when the allocator happened to hand back a
different address. The fix removes the dependency on allocator behaviour rather
than relying on it.

Fix

Clear bNotification before freeing the old notification, so the property
transitions A -> nullptr -> B and both writes notify. It also drops the
binding's subscription to the old notification while it is still alive.

Tests

src/wayland/idle_notify/test/monitor.cpp adds two cases. A zero timeout is
specified to notify as soon as the seat is inactive, so neither has to wait out
a real idle period.

unfixed:  PASS freshMonitorReportsIdle
          FAIL liveTimeoutChangeKeepsIdleSubscription
fixed:    PASS both

freshMonitorReportsIdle is deliberately a real assertion rather than a guard,
so a regression in the construction path fails loudly instead of masking the
second test. Both skip when the compositor lacks ext-idle-notify-v1.

Full suite: 10/10 passing in 0.65s.

The bug is allocator-dependent, and so is the test

Built with -DASAN=ON, the unfixed code passes both tests. ASAN's allocator
quarantines freed memory instead of handing it straight back, so the replacement
lands on a different address, bNotification genuinely changes, and the binding
re-subscribes. That is independent confirmation of the mechanism — remove the
address reuse and the bug disappears without touching updateNotification.

It also means this regression test cannot catch this bug under ASAN. No
black-box test can: without address reuse the old code is correct. Worth knowing
before concluding the test is ineffective if you run the suite sanitised.

ASAN reports nothing on either version, so this is a stale-value bug rather than
a memory-safety one. Clearing the property before the delete is defensive
ordering, not a use-after-free fix.

Two more things worth flagging, both happy to change:

  • This is the first automated test under src/wayland and the first use of
    QTRY_* in the tree. It needs a live compositor, so it will skip in CI as
    currently configured — it is a regression test for developers rather than CI
    coverage.
  • Because the control is now an assertion, a compositor that supports the
    protocol but mishandles zero timeouts will fail rather than skip.

Thanks for Quickshell — this is my first contribution here, so please tell me if
I've got the conventions wrong.

I came at this from the other end: my screen stopped blanking on Omarchy and I
went digging for why. It turned out to be this rather than anything in Omarchy's
shell, which is how I ended up in idle_notify.

Full disclosure on process: I used Claude Code to investigate this and write the
patch and tests. I directed the work, reviewed the diff line by line, and had it
walk me through the parts I didn't know — I can explain the reasoning behind
every line, and responsibility for it sits with me, which I understand is what
CONTRIBUTING asks for. Flagging it because I'd rather you know than wonder.

On style: I've run clang-format, followed the per-module qs_test pattern from
src/core/test, and matched the commit subject convention. But I write Python
day to day, so if the C++ idiom is off anywhere, say so and I'll fix it.

…laced

Writing timeout on a live IdleMonitor made it go permanently silent. The
notification was recreated correctly, but the replacement is routinely
allocated at the address the old one was just freed from, and bNotification
is a bindable property, so assigning an equal pointer notified nothing.
isIdle binds through bNotification to the notification's own isIdle, so it
was never re-evaluated and kept the value it last read. Toggling enabled
recovered it only because that is two updates with a real nullptr between
them.

Clear bNotification before freeing the old notification so both transitions
are observable, which also drops the subscription to the old notification
before it goes away.

The regression test writes timeout on a live monitor and waits for the new
notification to report idle. A zero timeout is specified to notify as soon
as the seat is inactive, so the test does not have to wait out a real idle
period. Both tests skip when no compositor supporting ext-idle-notify-v1 is
present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IdleMonitor goes permanently silent after a runtime write to timeout

1 participant