Don't try to send grants for not realized windows #236
Conversation
gui-agent/vmside.c
Outdated
| SKIP_NONMANAGED_WINDOW; | ||
|
|
||
| wd = list_lookup(windows_list, window)->data; |
There was a problem hiding this comment.
Since you do list_lookup here anyway, you can open-code (modified) SKIP_NONMANAGED_WINDOW here to avoid traversing the list twice.
There was a problem hiding this comment.
Yeah, saw that. But after having started refactoring this (there are multiple, slightly different variants in that code), I postponed it to first get this out. Will fix it.
There was a problem hiding this comment.
Moved into the "vmside: cleanup window lookup" commit in #239
|
Interesting, so it seems we found this issue before already, just hasn't realized its full scope: 8b53d36 |
Oh, yeah, interesting. This reminded me to have another look at the event ordering/generation. I'm not sure if the details of my description is accurate. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025071515-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests10 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 10 fixed
Unstable testsPerformance TestsPerformance degradation:7 performance degradations
Remaining performance tests:65 tests
|
|
(none of the openQA failures is related to this PR) |
So yeah, why the fix works is actually more tricky.
This is simply false. There's no damage event generated here (and that actually makes sense, since it copies the pixel data from the old pixmap). We rely here very much on implementation details of the X server. In particular we require that a damage event is generated after the windows is realized. Note that the map event is generated before realization. Additionally we require that no damage event is delivered between the map notification the window being realized. Based on experimentation and code reading that seems to hold (what exactly triggers the first damage event after map depends on background settings and cursor position). This matches also that we so far haven't had problems even though this bug has been present for a long time. After understanding this I initially planned to implement the fix differently to not rely so much on X server details. But this is not as trivial as I thought. The clean solution would be that the video driver notifies the agent after window realization and only then the agent sends the grants. Not a huge change, but not trivial either. So I'm considering using the current fix given that we know that it hasn't broken for a long time and X server is pretty stable. What do you think @marmarek?
Yeah, although this was before grant refs. There we had offsets and access to the pixmap memory, so access should have worked. The bug then was that we missed that composite had switched the pixmap and we still had mapped the old memory on the gui daemon side. |
Makes sense, it's pretty unlikely this logic will change at the Xorg side at this point. But update the commit message to be more accurate. |
gcc complains about our usage of strncpy. Under Linux zero padding without termination is actually ok. The path comes from trusted input. Silent trunctaion is still not nice. So clean this up.
We rely on composite redirect mode of the X server to get per window pixmaps. Those are setup/teared-down in compRealizeWindow/ compUnrealizeWindow (via compCheckRedirect). So not realized windows don't have a per window pixmap. Sending grant refs for them was always broken since we didn't send the offset into the screen pixmap in those cases. But with the recent change to not allocate grant refs for the screen pixmap this leads to a noticable error message. So don't try to send grant refs for not realized windows. See also added inline comment and QubesOS#236
HW42
force-pushed
the
simon/send-refs-fix
branch
from
6a6d733 to
0fc7bb1
Compare
|
Sending custom events from the video driver to the agent is actually easier than I remembered (probably confused it with what we are doing between agent and input driver). So ended up implementing the nicer fix. Only downside is that the extension handling needs quite a bit of boilerplate code, particularly on the Xlib side. This PR now depends on #239 (changed on top of the cleanup branch) |
We rely on composite redirect mode of the X server to get per window pixmaps. Those are setup/teared-down in compRealizeWindow/ compUnrealizeWindow (via compCheckRedirect).
So not realized windows don't have a per window pixmap. Sending grant refs for them was always broken since we didn't send the offset into the screen pixmap in those cases. But with the recent change to not allocate grant refs for the screen pixmap this leads to a noticable error message.
So don't try to send grant refs for not realized windows. This means that the configure before mapping will not contiain grant refs. But when we map the window we will get a damage event because of the new pixmap compRealizeWindow has allocated and send them then. So this should be fine.