wayland: ensure configure sequence is acked before mapping a surface [chromium/src : main]
Nick Yamane (Gerrit)
Attention is currently required from: Kramer Ge.
Nick Yamane has uploaded this change for review.
wayland: ensure configure sequence is acked before mapping a surface
In order to avoid "xdg_surface has never been configured" fatal protocol
errors, like in the linked crbug, we must ensure the initial configure
sequence is acked before trying to attach a buffer to that xdg_surface's
wl_surface.
Due to the multi-process async-oriented of Chrome graphics, the current
configure handling may be done both:
1. Instantly, upon xdg_surface.configure event (simple case), and
2. Deferred to be notified to the Wayland compositor
(set_window_geometry + ack_configure requests, amongst others) just
before attaching the buffer corresponding to the new frame.
In the second case, some obscure problems were observed in the wild,
where the configure sequence is not acked before trying to attach the
first buffer (surface mapping), resulting in crashes due to fatal
protocol error. However, as the spec says:
> A client is not required to commit immediately after sending an
> ack_configure request - it may even ack_configure several times before
> its next surface commit.
>
> A client may send multiple ack_configure requests before committing,
> but only the last request sent before a commit indicates which configure
> event the client really is responding to.
This fix proposes to send ack_configure immediately even in scenario (2),
for not-yet-configured surfaces.
[1] https://wayland.app/protocols/xdg-shell#xdg_surface:request:ack_configure
R=fang...@chromium.org
Bug: 1313023
Change-Id: Ia35f789d2e3452260be47b77062dd4715654fff0
---
M ui/ozone/platform/wayland/host/wayland_window.cc
1 file changed, 53 insertions(+), 0 deletions(-)
diff --git a/ui/ozone/platform/wayland/host/wayland_window.cc b/ui/ozone/platform/wayland/host/wayland_window.cc
index 153812a7..4163da2c5 100644
--- a/ui/ozone/platform/wayland/host/wayland_window.cc
+++ b/ui/ozone/platform/wayland/host/wayland_window.cc
@@ -979,6 +979,16 @@
// PlatformWindowDelegate at most once per frame.
if (pending_configures_.size() <= 1)
ApplyPendingBounds();
+ // Ensure this configure sequence is acked, even if the visual update
+ // (as well as an additional set_window_geometry/ack_configure for this
+ // sequence) happens when rendering the next frame. As per the spec,
+ // multiple acks can be sent for a single configure sequence, which in our
+ // case might avoid issues like https://crbug.com/1313023. More details:
+ // https://wayland.app/protocols/xdg-shell#xdg_surface:request:ack_configure
+ if (!IsSurfaceConfigured()) {
+ AckConfigure(serial);
+ connection()->ScheduleFlush();
+ }
}
}
To view, visit change 3647289. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Kramer Ge.
Patch set 3:Commit-Queue +1
1 comment:
Patchset:
kramer, ptal.
To view, visit change 3647289. To unsubscribe, or for help writing mail filters, visit settings.
Kramer Ge (Gerrit)
Attention is currently required from: Nick Yamane.
Patch set 3:Code-Review +1
2 comments:
Patchset:
Makes sense to me. This should always happen in the first configure? Should probably enforce DCHECK_EQ(pending_configures_.size(), 1u);
File ui/ozone/platform/wayland/host/wayland_window.cc:
Patch Set #3, Line 989:
AckConfigure(serial);What happens if the next incoming frame doesn't use this pending_configure.bounds, but the serial ack will be committed anyways b/c it is already sent.
Then a following frame coming that use the pending_configure.bounds will commit the same serial again, I'm not sure if certain wayland compositor will complain. I don't think they will as there's no error enum associated with it but we should watch out for it.
To view, visit change 3647289. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Kramer Ge.
1 comment:
Patchset:
Marking this back as WIP due to new crash reports by hjfreyer@ after testing with this CL applied [1].
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1313023#c34
To view, visit change 3647289. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Nick Yamane abandoned this change.
To view, visit change 3647289. To unsubscribe, or for help writing mail filters, visit settings.