Skip to content

Fix/quickshell crashing#3502

Open
cmeissl wants to merge 2 commits intoniri-wm:mainfrom
cmeissl:fix/quickshell_crashing
Open

Fix/quickshell crashing#3502
cmeissl wants to merge 2 commits intoniri-wm:mainfrom
cmeissl:fix/quickshell_crashing

Conversation

@cmeissl
Copy link
Contributor

@cmeissl cmeissl commented Feb 24, 2026

No description provided.

clients might re-create layer shell surfaces when receiving
a close event. give them a chance to detect that the
output has been removed by disabling the global before
sending the close event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this function it doesn't matter where the code is because it's a synchronous function. We don't send or receive anything from clients within this function. So I'd rather leave the code where it was

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both, disable global and layersurface send_close will send events to clients. This commit does change the order. I am not convinced this is necessary, but it seems to prevent some qs ghost Windows. I have to check, but my theory is like stated in the commit message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, they aren't sent but they are buffered to be sent in the ordered Wayland message buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, technically they are buffered and not directly sent here.

My theory is based on the fact that without sending the output destroy before the close the client can not tell the difference.

layer shell close defines:

The closed event is sent by the compositor when the surface will no longer be shown. The output may have been destroyed or the user may have asked for it to be removed. Further changes to the surface will be ignored. The client should destroy the resource after receiving this event, and create a new surface if they so choose.

I can't tell if it intentionally uses the past tense for output destroy, but it would make sense.

@cmeissl
Copy link
Contributor Author

cmeissl commented Feb 25, 2026

@YaLTeR I have some idea on what happens. While neither smithay nor niri intentionally send a nil for output in surface.enter the client might still see a nil value if it has destroyed the proxy client side. wayland-client tracks destroyed proxies and will replace destroyed resources with a null value when processing event arguments.
I would argue that a client should not use a destroyed output in layer shell, but we might also want to make sure to not send events for disabled globals.

this is more a workaround as the output should be destroyed
at this point. it looks like someone might be keeping the output
alive.
@cmeissl cmeissl force-pushed the fix/quickshell_crashing branch from a14d89d to 3c38efe Compare February 25, 2026 19:18
@cmeissl cmeissl marked this pull request as ready for review February 25, 2026 19:18
@cmeissl
Copy link
Contributor Author

cmeissl commented Feb 25, 2026

@YaLTeR finding a solution for preventing sending disabled global instances in event arguments will take some time.
so imo this is worth to consider in the mean time to prevent some crashes.

@YaLTeR
Copy link
Member

YaLTeR commented Feb 25, 2026

Yeah, sure, makes sense

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.

2 participants