fix: create XWayland wrappers before initial xprop reads caused by #948#1031
fix: create XWayland wrappers before initial xprop reads caused by #948#1031LFRon wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LFRon The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @LFRon. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideRefactors WXWayland async X11 property reading to be per-request and independent of PropertyNotify, adds robust cancellation and polling, and reorders ShellHandler so XWayland SurfaceWrapper instances are created before optional initial xprop reads—plus ensures late-created wrappers apply mapped state correctly. Sequence diagram for per-request async X11 property reading in WXWaylandsequenceDiagram
participant ShellHandler
participant WXWayland
participant WXWaylandPrivate as WXWaylandPrivate
participant Xcb as XCB
ShellHandler->>WXWayland: readAsyncProperties(windowId, requests, timeoutMs, callback)
WXWayland->>WXWaylandPrivate: store PerWindowProps(windowId, requests)
WXWaylandPrivate->>Xcb: xcb_get_property_unchecked(...)
Xcb-->>WXWaylandPrivate: cookies
WXWaylandPrivate->>WXWaylandPrivate: start timeoutTimer
WXWaylandPrivate->>WXWaylandPrivate: start pollTimer
loop poll
WXWaylandPrivate->>WXWaylandPrivate: xcbPollReplies()
WXWaylandPrivate->>Xcb: xcb_poll_for_reply64(cookie)
Xcb-->>WXWaylandPrivate: reply
WXWaylandPrivate->>WXWaylandPrivate: mark done[i], store results
alt all requests done
WXWaylandPrivate->>WXWaylandPrivate: cleanupAsyncRequest(it, false)
WXWaylandPrivate-->>ShellHandler: callback(windowId, results)
end
end
alt timeout fires
WXWaylandPrivate->>WXWaylandPrivate: xcbAsyncTimeoutForRequest(requestId)
WXWaylandPrivate->>Xcb: xcb_poll_for_reply64 / xcb_discard_reply
WXWaylandPrivate->>WXWaylandPrivate: cleanupAsyncRequest(it, false)
WXWaylandPrivate-->>ShellHandler: callback(windowId, results)
end
opt cancel from windowId
ShellHandler->>WXWayland: cancelAsyncProperties(windowId)
WXWayland->>WXWaylandPrivate: cleanupAsyncRequest(it, true)
WXWaylandPrivate->>Xcb: xcb_discard_reply(sequence)
end
Sequence diagram for XWayland SurfaceWrapper creation and initial property handlingsequenceDiagram
participant WXWaylandSurface as WXWaylandSurface
participant ShellHandler
participant RootContainer as RootSurfaceContainer
participant Wrapper as SurfaceWrapper
participant IMMgr as IMCandidatePanelManager
WXWaylandSurface-->>ShellHandler: onXWaylandSurfaceAdded(surface)
ShellHandler->>ShellHandler: ensureXwaylandWrapper(surface, appId)
ShellHandler->>RootContainer: getSurface(surface)
RootContainer-->>ShellHandler: wrapper
ShellHandler->>ShellHandler: fetchInitialProperties(surface)
ShellHandler->>WXWaylandSurface: xwayland()
ShellHandler->>WXWaylandSurface: handle()->handle()->window_id
ShellHandler->>IMMgr: imCandidatePanelAtom()
ShellHandler->>WXWaylandSurface: xwayland()->readAsyncProperties(..., callback)
WXWaylandSurface-->>ShellHandler: onInitialPropertiesReady(surface, result)
ShellHandler->>IMMgr: isIMCandidatePanel(result, imCandidatePanelAtom)
ShellHandler->>WXWaylandSurface: setProperty(imCandidatePanel)
ShellHandler->>RootContainer: getSurface(surface)
RootContainer-->>ShellHandler: wrapper
ShellHandler->>IMMgr: checkAndApplyIMCandidatePanel(wrapper, surface)
note over Wrapper: Late-wrapper mapped handling
Wrapper->>Wrapper: setHasInitializeContainer(true)
Wrapper->>Wrapper: onMappedChanged()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new per-request
pollTimerrunning every 1ms for each async property read could be quite heavy under load; consider using a shared timer or integratingxcbPollReplies()into an existing event loop tick to avoid a proliferation of high-frequency timers. cleanupAsyncRequestusesdeleteLater()for timers, which assumes an active event loop and object lifetime; if this can be called during teardown (e.g., in destructors), consider using direct deletion or guarding against late timer invocations to avoid leaks or use-after-free issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new per-request `pollTimer` running every 1ms for each async property read could be quite heavy under load; consider using a shared timer or integrating `xcbPollReplies()` into an existing event loop tick to avoid a proliferation of high-frequency timers.
- `cleanupAsyncRequest` uses `deleteLater()` for timers, which assumes an active event loop and object lifetime; if this can be called during teardown (e.g., in destructors), consider using direct deletion or guarding against late timer invocations to avoid leaks or use-after-free issues.
## Individual Comments
### Comment 1
<location path="waylib/src/server/protocols/wxwayland.cpp" line_range="598-604" />
<code_context>
+ for (auto it = asyncProps.begin(); it != asyncProps.end();) {
auto &props = it.value();
bool windowDone = true;
for (int i = 0; i < props.cookies.size(); ++i) {
- if (props.results.contains(props.requests[i].atom))
</code_context>
<issue_to_address>
**issue (bug_risk):** `windowDone` never becomes false when replies are not ready, causing premature completion.
In the new `xcbPollReplies` loop, `windowDone` stays `true` even when `xcb_poll_for_reply64` returns 0 or an error, and `props.done[i]` is set unconditionally. This means a failed or pending poll can still mark the whole window as complete. Only set `props.done[i]` and keep `windowDone` as `true` when `ret == 1`, and set `windowDone = false` when `ret == 0` or on error so incomplete properties don’t prematurely complete the request.
</issue_to_address>
### Comment 2
<location path="waylib/src/server/protocols/wxwayland.cpp" line_range="677-678" />
<code_context>
free(replyPtr);
if (err)
free(err);
+ if (ret == 0 && props.cookies[i].sequence)
+ xcb_discard_reply(conn, props.cookies[i].sequence);
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The `xcb_discard_reply` call is currently unreachable due to being nested inside `if (ret == 1 && replyPtr)`.
Because this logic is inside the `if (ret == 1 && replyPtr)` block in `xcbAsyncTimeoutForRequest`, the `if (ret == 0 && props.cookies[i].sequence)` branch can never run. If the goal is to discard unretrieved replies when `ret == 0`, this check should be moved outside the `ret == 1` block and run whenever `ret == 0` with a valid sequence; otherwise we neither keep polling nor discard, leaving replies in the XCB queue.
</issue_to_address>
### Comment 3
<location path="waylib/src/server/protocols/wxwayland.cpp" line_range="147" />
<code_context>
+ QTimer *pollTimer = nullptr;
};
- QMap<xcb_window_t, PerWindowProps> asyncProps;
+ QMap<quint64, PerWindowProps> asyncProps;
+ quint64 nextAsyncPropRequestId = 1;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the async property infrastructure by reusing window-based keys, centralizing polling and timeouts, and encoding completion in cookies instead of extra state and helper functions.
You can keep the new robustness while simplifying a few key areas. The main opportunities are:
1. **Use `xcb_window_t` as the primary key, and track multiple requests per window inside.**
2. **Use a single shared poll timer instead of per-request `pollTimer`s.**
3. **Drop the parallel `done` vector by encoding completion in `cookies`.**
4. **Simplify cleanup by removing `cleanupAsyncRequest` indirection.**
Below are concrete, incremental changes that keep your current behavior (timeouts, discard of pending replies, etc.) but reduce moving parts.
---
### 1. Go back to `QMap<xcb_window_t, PerWindowProps>`, allow multiple requests per window internally
Instead of `QMap<quint64, PerWindowProps>` and a synthetic `requestId`, you can keep the map keyed by window and add an inner per-request structure if you really need multiple concurrent sessions per window:
```cpp
struct AsyncPropSession
{
QVector<WXWayland::AsyncPropRequest> requests;
QMap<xcb_atom_t, QByteArray> results;
QVector<xcb_get_property_cookie_t> cookies;
std::function<void(xcb_window_t, const QMap<xcb_atom_t, QByteArray> &)> callback;
QElapsedTimer deadline; // or qint64 deadlineMs
};
struct PerWindowProps
{
QVector<AsyncPropSession> sessions;
};
QMap<xcb_window_t, PerWindowProps> asyncProps;
```
Then `cancelAsyncProperties(windowId)` stays a simple `find(windowId)`:
```cpp
void WXWayland::cancelAsyncProperties(xcb_window_t windowId)
{
W_D(WXWayland);
auto it = d->asyncProps.find(windowId);
if (it == d->asyncProps.end())
return;
auto *conn = xcbConnection();
if (conn) {
for (auto &session : it->sessions) {
for (const auto &cookie : session.cookies) {
if (cookie.sequence)
xcb_discard_reply(conn, cookie.sequence);
}
}
}
d->asyncProps.erase(it);
}
```
This keeps multi-session support without global `requestId` bookkeeping or iterator invalidation issues across a `QMap<quint64, …>`.
---
### 2. Replace per-request `pollTimer` with a single shared poll timer
Instead of creating one `pollTimer` per request, keep a single timer in `WXWaylandPrivate` that drives `xcbPollReplies()` as long as there are pending sessions:
```cpp
// In WXWaylandPrivate
QTimer *pollTimer = nullptr;
// init once
WXWaylandPrivate::WXWaylandPrivate()
{
pollTimer = new QTimer(q);
connect(pollTimer, &QTimer::timeout, q, [this]() { xcbPollReplies(); });
pollTimer->setInterval(1); // or a slightly larger value if acceptable
pollTimer->setSingleShot(false);
}
```
Start/stop it based on `asyncProps` emptiness:
```cpp
void WXWayland::readWindowPropertiesAsync(...)
{
W_D(WXWayland);
// ... set up session ...
d->asyncProps[windowId].sessions.append(std::move(session));
if (!d->pollTimer->isActive())
d->pollTimer->start();
}
```
And in `xcbPollReplies()`:
```cpp
void WXWaylandPrivate::xcbPollReplies()
{
if (asyncProps.isEmpty()) {
pollTimer->stop();
return;
}
// ... iterate windows/sessions, read replies ...
if (asyncProps.isEmpty())
pollTimer->stop();
}
```
This removes all per-request `pollTimer` allocation and the need to manage them in `cleanupAsyncRequest`.
---
### 3. Replace `QVector<bool> done` with “cookie cleared” as completion flag
You can drop the `done` vector by treating a cleared cookie (`sequence == 0`) as “done”. This keeps the same semantics but avoids parallel arrays.
When you successfully poll or decide a cookie is finished (including timeout/discard), clear its sequence:
```cpp
// helper
inline bool isCookiePending(const xcb_get_property_cookie_t &cookie)
{
return cookie.sequence != 0;
}
inline void markCookieDone(xcb_get_property_cookie_t &cookie)
{
cookie.sequence = 0;
}
```
Use in `xcbPollReplies()`:
```cpp
for (int i = 0; i < session.cookies.size(); ++i) {
auto &cookie = session.cookies[i];
if (!isCookiePending(cookie))
continue;
void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);
if (ret == 0) {
windowDone = false;
continue;
}
markCookieDone(cookie);
if (ret == 1 && replyPtr) {
auto *reply = static_cast<xcb_get_property_reply_t *>(replyPtr);
if (reply->type != 0 && reply->value_len > 0) {
QByteArray data(static_cast<const char *>(xcb_get_property_value(reply)),
xcb_get_property_value_length(reply));
session.results.insert(session.requests[i].atom, data);
}
free(reply);
} else if (err) {
free(err);
}
}
```
Completion check becomes:
```cpp
bool windowDone = std::all_of(session.cookies.begin(), session.cookies.end(),
[](const auto &c) { return !isCookiePending(c); });
```
Timeout handling uses the same mechanism and calls `xcb_discard_reply` before clearing:
```cpp
for (int i = 0; i < session.cookies.size(); ++i) {
auto &cookie = session.cookies[i];
if (!isCookiePending(cookie))
continue;
void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);
if (ret == 0 && cookie.sequence)
xcb_discard_reply(conn, cookie.sequence);
markCookieDone(cookie);
// ... same result insertion / free logic ...
}
```
This preserves all your new behavior (including discarding unconsumed replies) without the additional `done` vector and its synchronization cost.
---
### 4. Simplify cleanup, avoid `cleanupAsyncRequest` as a separate concept
If you:
- key by `xcb_window_t`,
- use a single shared `pollTimer`,
- encode completion in `cookies`,
then `cleanupAsyncRequest` can be inlined into the point where you decide a session is done:
```cpp
void WXWaylandPrivate::xcbPollReplies()
{
W_Q(WXWayland);
xcb_connection_t *conn = q->xcbConnection();
if (!conn)
return;
for (auto winIt = asyncProps.begin(); winIt != asyncProps.end();) {
auto &perWindow = winIt.value();
for (int s = 0; s < perWindow.sessions.size();) {
auto &session = perWindow.sessions[s];
// ... poll replies, update cookies/results ...
bool sessionDone = std::all_of(session.cookies.begin(), session.cookies.end(),
[](const auto &c) { return !isCookiePending(c); });
if (sessionDone) {
auto callback = std::move(session.callback);
auto windowId = winIt.key();
auto results = session.results;
perWindow.sessions.remove(s);
callback(windowId, results);
} else {
++s;
}
}
if (perWindow.sessions.isEmpty())
winIt = asyncProps.erase(winIt);
else
++winIt;
}
}
```
Timeouts can be handled similarly via a single timer that checks deadlines per session instead of maintaining a `timeoutTimer` per request. If you want to keep a per-request timeout, you can store the expiry time in `AsyncPropSession` instead of spawning another `QTimer`.
---
These changes preserve the functional improvements you added (timeouts, discard of pending replies, more robust async handling) while:
- removing artificial `requestId` indirection,
- centralizing polling into one timer,
- eliminating `QVector<bool> done`,
- and simplifying lifecycle / cleanup logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| bool windowDone = true; | ||
| for (int i = 0; i < props.cookies.size(); ++i) { | ||
| if (props.results.contains(props.requests[i].atom)) | ||
| continue; // already got this one | ||
| if (props.done[i]) | ||
| continue; | ||
|
|
||
| void *replyPtr = nullptr; | ||
| xcb_generic_error_t *err = nullptr; |
There was a problem hiding this comment.
issue (bug_risk): windowDone never becomes false when replies are not ready, causing premature completion.
In the new xcbPollReplies loop, windowDone stays true even when xcb_poll_for_reply64 returns 0 or an error, and props.done[i] is set unconditionally. This means a failed or pending poll can still mark the whole window as complete. Only set props.done[i] and keep windowDone as true when ret == 1, and set windowDone = false when ret == 0 or on error so incomplete properties don’t prematurely complete the request.
| QTimer *pollTimer = nullptr; | ||
| }; | ||
|
|
||
| QMap<xcb_window_t, PerWindowProps> asyncProps; |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the async property infrastructure by reusing window-based keys, centralizing polling and timeouts, and encoding completion in cookies instead of extra state and helper functions.
You can keep the new robustness while simplifying a few key areas. The main opportunities are:
- Use
xcb_window_tas the primary key, and track multiple requests per window inside. - Use a single shared poll timer instead of per-request
pollTimers. - Drop the parallel
donevector by encoding completion incookies. - Simplify cleanup by removing
cleanupAsyncRequestindirection.
Below are concrete, incremental changes that keep your current behavior (timeouts, discard of pending replies, etc.) but reduce moving parts.
1. Go back to QMap<xcb_window_t, PerWindowProps>, allow multiple requests per window internally
Instead of QMap<quint64, PerWindowProps> and a synthetic requestId, you can keep the map keyed by window and add an inner per-request structure if you really need multiple concurrent sessions per window:
struct AsyncPropSession
{
QVector<WXWayland::AsyncPropRequest> requests;
QMap<xcb_atom_t, QByteArray> results;
QVector<xcb_get_property_cookie_t> cookies;
std::function<void(xcb_window_t, const QMap<xcb_atom_t, QByteArray> &)> callback;
QElapsedTimer deadline; // or qint64 deadlineMs
};
struct PerWindowProps
{
QVector<AsyncPropSession> sessions;
};
QMap<xcb_window_t, PerWindowProps> asyncProps;Then cancelAsyncProperties(windowId) stays a simple find(windowId):
void WXWayland::cancelAsyncProperties(xcb_window_t windowId)
{
W_D(WXWayland);
auto it = d->asyncProps.find(windowId);
if (it == d->asyncProps.end())
return;
auto *conn = xcbConnection();
if (conn) {
for (auto &session : it->sessions) {
for (const auto &cookie : session.cookies) {
if (cookie.sequence)
xcb_discard_reply(conn, cookie.sequence);
}
}
}
d->asyncProps.erase(it);
}This keeps multi-session support without global requestId bookkeeping or iterator invalidation issues across a QMap<quint64, …>.
2. Replace per-request pollTimer with a single shared poll timer
Instead of creating one pollTimer per request, keep a single timer in WXWaylandPrivate that drives xcbPollReplies() as long as there are pending sessions:
// In WXWaylandPrivate
QTimer *pollTimer = nullptr;
// init once
WXWaylandPrivate::WXWaylandPrivate()
{
pollTimer = new QTimer(q);
connect(pollTimer, &QTimer::timeout, q, [this]() { xcbPollReplies(); });
pollTimer->setInterval(1); // or a slightly larger value if acceptable
pollTimer->setSingleShot(false);
}Start/stop it based on asyncProps emptiness:
void WXWayland::readWindowPropertiesAsync(...)
{
W_D(WXWayland);
// ... set up session ...
d->asyncProps[windowId].sessions.append(std::move(session));
if (!d->pollTimer->isActive())
d->pollTimer->start();
}And in xcbPollReplies():
void WXWaylandPrivate::xcbPollReplies()
{
if (asyncProps.isEmpty()) {
pollTimer->stop();
return;
}
// ... iterate windows/sessions, read replies ...
if (asyncProps.isEmpty())
pollTimer->stop();
}This removes all per-request pollTimer allocation and the need to manage them in cleanupAsyncRequest.
3. Replace QVector<bool> done with “cookie cleared” as completion flag
You can drop the done vector by treating a cleared cookie (sequence == 0) as “done”. This keeps the same semantics but avoids parallel arrays.
When you successfully poll or decide a cookie is finished (including timeout/discard), clear its sequence:
// helper
inline bool isCookiePending(const xcb_get_property_cookie_t &cookie)
{
return cookie.sequence != 0;
}
inline void markCookieDone(xcb_get_property_cookie_t &cookie)
{
cookie.sequence = 0;
}Use in xcbPollReplies():
for (int i = 0; i < session.cookies.size(); ++i) {
auto &cookie = session.cookies[i];
if (!isCookiePending(cookie))
continue;
void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);
if (ret == 0) {
windowDone = false;
continue;
}
markCookieDone(cookie);
if (ret == 1 && replyPtr) {
auto *reply = static_cast<xcb_get_property_reply_t *>(replyPtr);
if (reply->type != 0 && reply->value_len > 0) {
QByteArray data(static_cast<const char *>(xcb_get_property_value(reply)),
xcb_get_property_value_length(reply));
session.results.insert(session.requests[i].atom, data);
}
free(reply);
} else if (err) {
free(err);
}
}Completion check becomes:
bool windowDone = std::all_of(session.cookies.begin(), session.cookies.end(),
[](const auto &c) { return !isCookiePending(c); });Timeout handling uses the same mechanism and calls xcb_discard_reply before clearing:
for (int i = 0; i < session.cookies.size(); ++i) {
auto &cookie = session.cookies[i];
if (!isCookiePending(cookie))
continue;
void *replyPtr = nullptr;
xcb_generic_error_t *err = nullptr;
int ret = xcb_poll_for_reply64(conn, cookie.sequence, &replyPtr, &err);
if (ret == 0 && cookie.sequence)
xcb_discard_reply(conn, cookie.sequence);
markCookieDone(cookie);
// ... same result insertion / free logic ...
}This preserves all your new behavior (including discarding unconsumed replies) without the additional done vector and its synchronization cost.
4. Simplify cleanup, avoid cleanupAsyncRequest as a separate concept
If you:
- key by
xcb_window_t, - use a single shared
pollTimer, - encode completion in
cookies,
then cleanupAsyncRequest can be inlined into the point where you decide a session is done:
void WXWaylandPrivate::xcbPollReplies()
{
W_Q(WXWayland);
xcb_connection_t *conn = q->xcbConnection();
if (!conn)
return;
for (auto winIt = asyncProps.begin(); winIt != asyncProps.end();) {
auto &perWindow = winIt.value();
for (int s = 0; s < perWindow.sessions.size();) {
auto &session = perWindow.sessions[s];
// ... poll replies, update cookies/results ...
bool sessionDone = std::all_of(session.cookies.begin(), session.cookies.end(),
[](const auto &c) { return !isCookiePending(c); });
if (sessionDone) {
auto callback = std::move(session.callback);
auto windowId = winIt.key();
auto results = session.results;
perWindow.sessions.remove(s);
callback(windowId, results);
} else {
++s;
}
}
if (perWindow.sessions.isEmpty())
winIt = asyncProps.erase(winIt);
else
++winIt;
}
}Timeouts can be handled similarly via a single timer that checks deadlines per session instead of maintaining a timeoutTimer per request. If you want to keep a per-request timeout, you can store the expiry time in AsyncPropSession instead of spawning another QTimer.
These changes preserve the functional improvements you added (timeouts, discard of pending replies, more robust async handling) while:
- removing artificial
requestIdindirection, - centralizing polling into one timer,
- eliminating
QVector<bool> done, - and simplifying lifecycle / cleanup logic.
|
TAG Bot New tag: 0.8.12 |
7365cb5 to
7a05539
Compare
|
TAG Bot New tag: 0.8.13 |
7a05539 to
65e1853
Compare
|
该问题看着已修复, 关闭这个PR |
|
最新提交里又出现了一样的问题,故重新开启该PR |
…nuxdeepin#948 Create and match XWayland SurfaceWrapper instances as soon as the surface is associated, then read optional initial X11 properties afterwards. This keeps mapped state, activation, animations, focus and input setup from depending on a best-effort async xprop read. Make WXWayland async property reads independent per request and complete from XCB replies without waiting for PropertyNotify. Keep cancellation scoped by X window so pending reads are discarded when the surface dissociates. Also handle the late-wrapper case by applying the current mapped state when a mapped surface receives its container after the original mapped signal was missed.
65e1853 to
3e7aa7a
Compare
该PR用于修复 #948 引起的:
Create and match XWayland SurfaceWrapper instances as soon as the surface is associated, then read optional initial X11 properties afterwards. This keeps mapped state, activation, animations, focus and input setup from depending on a best-effort async xprop read.
Make WXWayland async property reads independent per request and complete from XCB replies without waiting for PropertyNotify. Keep cancellation scoped by X window so pending reads are discarded when the surface dissociates.
Also handle the late-wrapper case by applying the current mapped state when a mapped surface receives its container after the original mapped signal was missed.