From 0e5d161dd4989e5ce08c5ae517e99a3665e5d4d4 Mon Sep 17 00:00:00 2001 From: hiroTamada <88675973+hiroTamada@users.noreply.github.com> Date: Fri, 29 May 2026 19:19:56 +0000 Subject: [PATCH 1/4] Skip chromium restart on headful display resize Headful PATCH /display previously restarted chromium to re-apply --start-maximized against the new X root. The restart costs ~9s and also wipes browser-side state (Emulation.* overrides, devtools sessions). It turns out the restart isn't load-bearing: mutter reflows a window in windowState=maximized (or fullscreen) onto the new root whenever the X root resizes via xrandr/Neko. The server just has to make sure the window is in that state. This replaces the restart on the Xorg branch of PatchDisplay with a single Browser.setWindowBounds{windowState:"maximized"} call. The helper is idempotent: it early-returns when the window is already maximized or fullscreen, so kiosk windows stay in fullscreen. Callers that still want the legacy restart can pass restart_chromium=true. The explicit-bounds form of setWindowBounds (windowState:"normal" with {left,top,width,height}) is intentionally NOT used: once a window is in normal state mutter stops auto-tracking RANDR events, which would silently break subsequent resizes. New e2e test e2e_display_resize_window_test.go covers headless fast path, --start-maximized + neko, kiosk + fullscreen, and a force-maximized-via-CDP scenario. Each subtest asserts: - X root reaches the requested size (xrandr Screen 0: current WxH) - renderer screen.* and outer{Width,Height} converge - CDP windowState is preserved across the resize - CDP windowId is stable (proves no chromium restart) Co-Authored-By: Claude Opus 4.7 --- server/cmd/api/api/display.go | 54 +- server/e2e/e2e_display_resize_window_test.go | 629 +++++++++++++++++++ server/lib/cdpclient/cdpclient.go | 68 ++ 3 files changed, 748 insertions(+), 3 deletions(-) create mode 100644 server/e2e/e2e_display_resize_window_test.go diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 955320e3..b23e0734 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -122,9 +122,21 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ log.Info("using xrandr for Xorg resolution change (Neko disabled)") err = s.setResolutionXorgViaXrandr(ctx, width, height, refreshRate, restartChrome) } - if err == nil && restartChrome { - if restartErr := s.restartChromiumAndWait(ctx, "resolution change"); restartErr != nil { - log.Error("failed to restart chromium after resolution change", "error", restartErr) + // Headful default path: skip the chromium restart and re-assert + // the maximized window state via CDP instead. Mutter reflows a + // maximized window to fill the new X root automatically, so this + // is all that's needed — and it costs ~50ms vs ~9s for a restart. + // Callers can still opt into the legacy restart with + // restart_chromium=true (rare; preserved for compatibility). + if err == nil { + if restartChrome { + if restartErr := s.restartChromiumAndWait(ctx, "resolution change"); restartErr != nil { + log.Error("failed to restart chromium after resolution change", "error", restartErr) + } + } else { + if cdpErr := s.setWindowMaximizedViaCDP(ctx); cdpErr != nil { + log.Warn("CDP maximize re-assert failed after Xorg resolution change (non-fatal)", "error", cdpErr) + } } } } else if len(stopped) > 0 { @@ -367,6 +379,42 @@ func (s *ApiService) backgroundResizeXvfb(ctx context.Context, width, height int s.viewportMu.Unlock() } +// setWindowMaximizedViaCDP re-asserts that the chromium OS window is in +// the "maximized" state via Browser.setWindowBounds. After a successful +// xrandr/Neko resize on the headful path, mutter will reflow a maximized +// window to fill the new X root — so this call is the entirety of what +// the server needs to do post-resize. It replaces the previous approach +// of restarting chromium so it could re-apply --start-maximized, which +// cost a multi-second restart and also reset other browser-side state +// (notably Emulation.* overrides). +// +// The call is idempotent and cheap: when the window is already maximized +// the helper returns immediately without sending any CDP commands. +func (s *ApiService) setWindowMaximizedViaCDP(ctx context.Context) error { + log := logger.FromContext(ctx) + + upstreamURL := s.upstreamMgr.Current() + if upstreamURL == "" { + return fmt.Errorf("devtools upstream not available") + } + + cdpCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + client, err := cdpclient.Dial(cdpCtx, upstreamURL) + if err != nil { + return fmt.Errorf("failed to connect to devtools: %w", err) + } + defer client.Close() + + if err := client.SetWindowBoundsMaximized(cdpCtx); err != nil { + return fmt.Errorf("CDP setWindowBoundsMaximized: %w", err) + } + + log.Info("re-asserted maximized window state via CDP") + return nil +} + // setViewportViaCDP resizes the browser viewport using the CDP // Emulation.setDeviceMetricsOverride command. This is near-instant and does // not require restarting Chromium or Xvfb. diff --git a/server/e2e/e2e_display_resize_window_test.go b/server/e2e/e2e_display_resize_window_test.go new file mode 100644 index 00000000..fcb876f0 --- /dev/null +++ b/server/e2e/e2e_display_resize_window_test.go @@ -0,0 +1,629 @@ +package e2e + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "os/exec" + "strconv" + "strings" + "testing" + "time" + + instanceoapi "github.com/kernel/kernel-images/server/lib/oapi" + "github.com/stretchr/testify/require" +) + +// rendererViewport captures the dimensions visible to JS inside the page. It is +// the cheapest end-to-end signal that the OS window has been resized to fill +// the new X root: the renderer can't report a larger outerWidth/outerHeight +// than the actual chromium window the WM gave it. +type rendererViewport struct { + InnerWidth int `json:"innerWidth"` + InnerHeight int `json:"innerHeight"` + OuterWidth int `json:"outerWidth"` + OuterHeight int `json:"outerHeight"` + ScreenWidth int `json:"screenWidth"` + ScreenHght int `json:"screenHeight"` + DPR int `json:"devicePixelRatio"` +} + +// getRendererViewport evaluates window.* sizes in the active page via the +// playwright daemon. +func getRendererViewport(ctx context.Context, c *TestContainer) (rendererViewport, error) { + client, err := c.APIClient() + if err != nil { + return rendererViewport{}, err + } + code := ` + return await page.evaluate(() => ({ + innerWidth: window.innerWidth, + innerHeight: window.innerHeight, + outerWidth: window.outerWidth, + outerHeight: window.outerHeight, + screenWidth: screen.width, + screenHeight: screen.height, + devicePixelRatio: window.devicePixelRatio, + })); + ` + timeout := 5 + rsp, err := client.ExecutePlaywrightCodeWithResponse(ctx, instanceoapi.ExecutePlaywrightRequest{ + Code: code, + TimeoutSec: &timeout, + }) + if err != nil { + return rendererViewport{}, err + } + if rsp.JSON200 == nil || !rsp.JSON200.Success { + body := "" + if rsp != nil { + body = string(rsp.Body) + } + return rendererViewport{}, fmt.Errorf("playwright eval failed: %s", body) + } + raw, err := json.Marshal(rsp.JSON200.Result) + if err != nil { + return rendererViewport{}, err + } + var v rendererViewport + if err := json.Unmarshal(raw, &v); err != nil { + return rendererViewport{}, fmt.Errorf("unmarshal viewport %q: %w", string(raw), err) + } + return v, nil +} + +// getXRootResolution reads the live X root size via xrandr inside the +// container. Works for both Xorg (headful) and Xvfb (headless). The parse +// pulls from xrandr's `Screen 0: ... current W x H, ...` header — which is +// always present and reflects the live root — rather than looking for `*` +// next to an active mode line, because Xorg-with-Neko surfaces a synthetic +// active mode that xrandr does not mark with an asterisk. +func getXRootResolution(ctx context.Context, c *TestContainer) (int, int, error) { + out, err := execCombinedOutput(ctx, c, "bash", []string{"-c", "DISPLAY=:1 xrandr"}) + if err != nil { + return 0, 0, fmt.Errorf("xrandr exec: %w (out=%q)", err, out) + } + for _, line := range strings.Split(out, "\n") { + idx := strings.Index(line, "current ") + if idx < 0 { + continue + } + rest := line[idx+len("current "):] + end := strings.Index(rest, ",") + if end > 0 { + rest = rest[:end] + } + fields := strings.Fields(rest) // ["W", "x", "H"] + if len(fields) < 3 { + return 0, 0, fmt.Errorf("unexpected current segment %q", rest) + } + w, err := strconv.Atoi(fields[0]) + if err != nil { + return 0, 0, fmt.Errorf("parse width %q: %w", fields[0], err) + } + h, err := strconv.Atoi(fields[2]) + if err != nil { + return 0, 0, fmt.Errorf("parse height %q: %w", fields[2], err) + } + return w, h, nil + } + return 0, 0, fmt.Errorf("could not find 'current WxH' in xrandr output: %s", strings.TrimSpace(out)) +} + +// chromiumWindowBounds is the subset of Browser.getWindowBounds we care +// about. windowState distinguishes the "normal" case (where width/height are +// the actual OS window size) from "maximized"/"fullscreen" (where width/height +// are the saved-restore bounds and the live size matches the root). +type chromiumWindowBounds struct { + WindowID int `json:"windowId"` + Width int `json:"width"` + Height int `json:"height"` + Left int `json:"left"` + Top int `json:"top"` + WindowState string `json:"windowState"` +} + +// getChromiumWindowBoundsCDP queries Browser.getWindowForTarget + +// Browser.getWindowBounds for the first page target. Used to assert that +// the OS window stays in maximized/fullscreen state across a resize. +func getChromiumWindowBoundsCDP(ctx context.Context, c *TestContainer) (chromiumWindowBounds, error) { + cdp, err := newCDPClient(ctx, c.CDPURL()) + if err != nil { + return chromiumWindowBounds{}, fmt.Errorf("dial cdp: %w", err) + } + defer cdp.Close() + + targetsRaw, err := cdp.Call(ctx, "Target.getTargets", map[string]any{}, "") + if err != nil { + return chromiumWindowBounds{}, fmt.Errorf("Target.getTargets: %w", err) + } + var targets struct { + TargetInfos []struct { + TargetID string `json:"targetId"` + Type string `json:"type"` + } `json:"targetInfos"` + } + if err := json.Unmarshal(targetsRaw, &targets); err != nil { + return chromiumWindowBounds{}, fmt.Errorf("unmarshal targets: %w", err) + } + var pageID string + for _, t := range targets.TargetInfos { + if t.Type == "page" { + pageID = t.TargetID + break + } + } + if pageID == "" { + return chromiumWindowBounds{}, fmt.Errorf("no page target found") + } + + winRaw, err := cdp.Call(ctx, "Browser.getWindowForTarget", map[string]any{"targetId": pageID}, "") + if err != nil { + return chromiumWindowBounds{}, fmt.Errorf("Browser.getWindowForTarget: %w", err) + } + var winResp struct { + WindowID int `json:"windowId"` + Bounds struct { + Width int `json:"width"` + Height int `json:"height"` + Left int `json:"left"` + Top int `json:"top"` + WindowState string `json:"windowState"` + } `json:"bounds"` + } + if err := json.Unmarshal(winRaw, &winResp); err != nil { + return chromiumWindowBounds{}, fmt.Errorf("unmarshal window: %w", err) + } + return chromiumWindowBounds{ + WindowID: winResp.WindowID, + Width: winResp.Bounds.Width, + Height: winResp.Bounds.Height, + Left: winResp.Bounds.Left, + Top: winResp.Bounds.Top, + WindowState: winResp.Bounds.WindowState, + }, nil +} + +// setChromiumWindowStateCDP forces the OS window to the given windowState +// ("normal" | "minimized" | "maximized" | "fullscreen") via Browser.setWindowBounds. +// Per CDP semantics: width/height/left/top fields must be omitted when +// windowState != "normal". +func setChromiumWindowStateCDP(ctx context.Context, c *TestContainer, state string) error { + cdp, err := newCDPClient(ctx, c.CDPURL()) + if err != nil { + return fmt.Errorf("dial cdp: %w", err) + } + defer cdp.Close() + + cur, err := getChromiumWindowBoundsCDP(ctx, c) + if err != nil { + return fmt.Errorf("get current window: %w", err) + } + + // CDP rejects state transitions like maximized → fullscreen directly; + // go through "normal" first when changing between non-normal states. + if cur.WindowState != "normal" && cur.WindowState != state { + if _, err := cdp.Call(ctx, "Browser.setWindowBounds", map[string]any{ + "windowId": cur.WindowID, + "bounds": map[string]any{"windowState": "normal"}, + }, ""); err != nil { + return fmt.Errorf("Browser.setWindowBounds normal: %w", err) + } + } + + if _, err := cdp.Call(ctx, "Browser.setWindowBounds", map[string]any{ + "windowId": cur.WindowID, + "bounds": map[string]any{"windowState": state}, + }, ""); err != nil { + return fmt.Errorf("Browser.setWindowBounds %s: %w", state, err) + } + return nil +} + +// waitForWindowState polls the chromium window state until it matches want +// or the timeout expires. Returns the final bounds. +func waitForWindowState(t *testing.T, ctx context.Context, c *TestContainer, want string, timeout time.Duration) chromiumWindowBounds { + t.Helper() + deadline := time.Now().Add(timeout) + var last chromiumWindowBounds + var lastErr error + for { + b, err := getChromiumWindowBoundsCDP(ctx, c) + lastErr = err + if err == nil { + last = b + if b.WindowState == want { + return b + } + } + if time.Now().After(deadline) { + t.Fatalf("window state never reached %q: last=%+v lastErr=%v", want, last, lastErr) + } + select { + case <-ctx.Done(): + t.Fatalf("context cancelled waiting for window state %q: %v", want, ctx.Err()) + case <-time.After(200 * time.Millisecond): + } + } +} + +// viewportPredicate decides when a rendererViewport reading is "close enough" +// to the requested size. The headless and headful paths use different +// predicates because --headless=new uses Chrome's internal window size +// (screen.width/height) while headful renders into a real OS window owned by +// Mutter (outerWidth/outerHeight equal to the X root). +type viewportPredicate func(v rendererViewport, wantW, wantH int) bool + +// waitForRendererViewport polls the renderer until the given predicate +// succeeds. Returns the matching viewport. +func waitForRendererViewport(t *testing.T, ctx context.Context, c *TestContainer, wantW, wantH int, pred viewportPredicate, label string, timeout time.Duration) rendererViewport { + t.Helper() + deadline := time.Now().Add(timeout) + var last rendererViewport + var lastErr error + for { + v, err := getRendererViewport(ctx, c) + lastErr = err + if err == nil { + last = v + if pred(v, wantW, wantH) { + t.Logf("[%s] renderer_viewport: outer=%dx%d inner=%dx%d screen=%dx%d dpr=%d (matches want=%dx%d)", + label, v.OuterWidth, v.OuterHeight, v.InnerWidth, v.InnerHeight, v.ScreenWidth, v.ScreenHght, v.DPR, wantW, wantH) + return v + } + } + if time.Now().After(deadline) { + t.Fatalf("[%s] renderer viewport never matched want=%dx%d: last=%+v lastErr=%v", label, wantW, wantH, last, lastErr) + } + select { + case <-ctx.Done(): + t.Fatalf("[%s] context cancelled waiting for renderer viewport %dx%d: %v", label, wantW, wantH, ctx.Err()) + case <-time.After(250 * time.Millisecond): + } + } +} + +// headlessViewportPredicate asserts innerWidth/innerHeight match the request. +// In --headless=new the renderer uses Chrome's internal window +// (Emulation.setDeviceMetricsOverride drives inner*) — screen.* and outer* +// stay at Chrome's startup defaults and do not move on resize, so they are +// intentionally not asserted. +func headlessViewportPredicate(v rendererViewport, wantW, wantH int) bool { + return v.InnerWidth == wantW && v.InnerHeight == wantH +} + +// makeHeadfulMaximizedPredicate is the strict headful check: screen.* +// matches AND outerWidth/outerHeight is within tolerance of the request. +// Tolerance absorbs mutter chrome (borders/titlebar) — kiosk fullscreen has +// none, so tolerance 0 is correct there; --start-maximized passes through +// mutter SSDs so a small slack is kinder. +func makeHeadfulMaximizedPredicate(tolerance int) viewportPredicate { + return func(v rendererViewport, wantW, wantH int) bool { + if v.ScreenWidth != wantW || v.ScreenHght != wantH { + return false + } + return abs(v.OuterWidth-wantW) <= tolerance && abs(v.OuterHeight-wantH) <= tolerance + } +} + +// baselineOrForcedState returns the windowState the test expects the window +// to be in after baseline setup. When forceMaximizeAtBaseline is set, we +// re-query CDP rather than trust the pre-force reading. +func baselineOrForcedState(sc resizeScenario, base chromiumWindowBounds) string { + if sc.forceMaximizeAtBaseline { + return "maximized" + } + return base.WindowState +} + +func abs(x int) int { + if x < 0 { + return -x + } + return x +} + +// resizeScenario describes a single image+chromium-config combination the +// resize tests exercise. extraEnv is applied at container start (e.g. to +// inject --kiosk into the default CHROMIUM_FLAGS). predicate decides when a +// renderer viewport reading is acceptable for this scenario. +type resizeScenario struct { + name string + image string + extraEnv map[string]string + predicate viewportPredicate + skipReason string + // assertXRoot toggles X-root convergence checks. + assertXRoot bool + // up/down override the default 1920x1080 → 1280x720 resize targets. + // Headful with Neko already starts at 1920x1080 so the up resize must + // pick a different size to actually exercise the screen change. + up [2]int + down [2]int + // restartChromium is the value passed to PATCH /display. The default + // (false zero value when restartChromiumSet is true) exercises the + // no-restart path; setting restartChromium=true forces the current + // behaviour. restartChromiumSet must be true for the test to send the + // field; otherwise the request omits it entirely. + restartChromium bool + restartChromiumSet bool + // forceMaximizeAtBaseline calls Browser.setWindowBounds{windowState:"maximized"} + // before the first resize. Used to test whether mutter's "maximized + // window reflows on RANDR" invariant holds once we put the window in + // that state — bypasses the docker-specific question of whether + // --start-maximized actually produces a maximized window. + forceMaximizeAtBaseline bool + // requireBaselineState asserts the window's CDP windowState at startup + // equals this value before any resize. Empty = no assertion. + requireBaselineState string +} + +func (sc resizeScenario) upSize() (int, int) { + if sc.up != [2]int{} { + return sc.up[0], sc.up[1] + } + return 1920, 1080 +} + +func (sc resizeScenario) downSize() (int, int) { + if sc.down != [2]int{} { + return sc.down[0], sc.down[1] + } + return 1280, 720 +} + +// TestDisplayResizeChromiumWindow exercises PATCH /display and asserts that +// after the resize the X root, the chromium OS window (CDP view), and the +// renderer's outer{Width,Height} all converge on the new size, with the +// window state (maximized / fullscreen) preserved across the resize and the +// CDP windowId stable (proving no chromium restart). +func TestDisplayResizeChromiumWindow(t *testing.T) { + if _, err := exec.LookPath("docker"); err != nil { + t.Skipf("docker not available: %v", err) + } + + scenarios := []resizeScenario{ + { + name: "headless_default", + image: headlessImage, + predicate: headlessViewportPredicate, + assertXRoot: true, // background Xvfb resize must converge + }, + { + // Production-equivalent scenario: --start-maximized + neko + + // the CHROMIUM_FLAGS_DEFAULT mirror from run-docker.sh:17. + // Uses the default restart_chromium (omitted from the + // request), exercising the server's new behaviour: skip the + // chromium restart and instead re-assert windowState=maximized + // via Browser.setWindowBounds. The strict outer-matches-screen + // predicate proves mutter reflows the maximized window on RANDR + // without any chromium restart. + name: "headful_start_maximized", + image: headfulImage, + extraEnv: map[string]string{ + "ENABLE_WEBRTC": "true", + "NEKO_ADMIN_PASSWORD": "admin", + // container.go:51 always appends --no-sandbox; the rest + // mirrors images/chromium-headful/run-docker.sh:17. + "CHROMIUM_FLAGS": "--user-data-dir=/home/kernel/user-data --disable-dev-shm-usage --disable-gpu --start-maximized --disable-software-rasterizer --remote-allow-origins=*", + }, + predicate: makeHeadfulMaximizedPredicate(20), // mutter borders/titlebar + assertXRoot: true, + up: [2]int{2560, 1440}, + down: [2]int{1280, 720}, + requireBaselineState: "maximized", + }, + { + name: "headful_kiosk", + image: headfulImage, + extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin", "CHROMIUM_FLAGS": "--kiosk --start-maximized"}, + // --kiosk runs the window in fullscreen state, which auto-tracks + // the screen size on every resize (verified: outer == screen + // after both up- and down-resize). Strict predicate works today. + predicate: makeHeadfulMaximizedPredicate(0), + assertXRoot: true, + up: [2]int{2560, 1440}, + down: [2]int{1280, 720}, + requireBaselineState: "fullscreen", + }, + { + // Direct test of the live-VM theory: kiosk's fullscreen window + // already auto-tracks RANDR resizes without any restart. Same + // scenario as headful_kiosk but with restart_chromium=false to + // confirm the restart is dead weight. + name: "headful_kiosk_no_restart", + image: headfulImage, + extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin", "CHROMIUM_FLAGS": "--kiosk --start-maximized"}, + predicate: makeHeadfulMaximizedPredicate(0), + assertXRoot: true, + up: [2]int{2560, 1440}, + down: [2]int{1280, 720}, + restartChromium: false, + restartChromiumSet: true, + requireBaselineState: "fullscreen", + }, + { + // Test the live-VM theory's central claim: if the chromium + // window is in windowState=maximized, mutter reflows it on + // RANDR — no Chromium restart needed. Bypasses the docker-only + // quirk where --start-maximized comes up in 'normal' state by + // forcing the window state via CDP at baseline. + name: "headful_force_maximized_no_restart", + image: headfulImage, + extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin"}, + predicate: makeHeadfulMaximizedPredicate(0), + assertXRoot: true, + up: [2]int{2560, 1440}, + down: [2]int{1280, 720}, + restartChromium: false, + restartChromiumSet: true, + forceMaximizeAtBaseline: true, + requireBaselineState: "maximized", + }, + } + + // Subtests run serially: two of the three boot the heavy headful image + // (neko + mutter + chromium) which races on startup mode under + // concurrent load — observed: neko's `desktop.screen=1920x1080@25` + // initialization fails to take effect when three privileged containers + // boot at once, leaving the root at the dummy DDX's 3840x2160 default. + for _, sc := range scenarios { + sc := sc + t.Run(sc.name, func(t *testing.T) { + if sc.skipReason != "" { + t.Skip(sc.skipReason) + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + env := map[string]string{ + "WIDTH": "1024", + "HEIGHT": "768", + } + for k, v := range sc.extraEnv { + env[k] = v + } + + c := NewTestContainer(t, sc.image) + require.NoError(t, c.Start(ctx, ContainerConfig{Env: env}), "failed to start container") + defer c.Stop(ctx) + require.NoError(t, c.WaitReady(ctx), "api not ready") + require.NoError(t, c.WaitDevTools(ctx), "devtools not ready") + + // Navigate to about:blank so playwright has a page to evaluate + // against — otherwise the daemon may not have a target wired up. + navigateBlank(t, ctx, c) + + // Baseline — log only. We don't assert specific initial + // dimensions because the headless renderer's window size at + // startup depends on Chrome's --headless=new defaults rather + // than the Xvfb root we asked for. + rootW, rootH, err := getXRootResolution(ctx, c) + require.NoError(t, err, "baseline xrandr") + t.Logf("[%s] baseline x_root=%dx%d", sc.name, rootW, rootH) + + baseline, err := getRendererViewport(ctx, c) + require.NoError(t, err, "baseline renderer viewport") + t.Logf("[%s] baseline renderer outer=%dx%d inner=%dx%d screen=%dx%d", + sc.name, baseline.OuterWidth, baseline.OuterHeight, baseline.InnerWidth, baseline.InnerHeight, baseline.ScreenWidth, baseline.ScreenHght) + + baseCDP, err := getChromiumWindowBoundsCDP(ctx, c) + require.NoError(t, err, "baseline cdp window") + t.Logf("[%s] baseline cdp=%+v", sc.name, baseCDP) + + // Optionally force the window into the maximized state at + // baseline. Tests the live-VM theory's invariant in + // environments where --start-maximized somehow doesn't produce + // a windowState=maximized window at startup (observed: docker). + if sc.forceMaximizeAtBaseline { + require.NoError(t, setChromiumWindowStateCDP(ctx, c, "maximized"), "force maximized via CDP") + forced := waitForWindowState(t, ctx, c, "maximized", 5*time.Second) + t.Logf("[%s] after force-maximize cdp=%+v", sc.name, forced) + } + + if sc.requireBaselineState != "" { + require.Equal(t, sc.requireBaselineState, baselineOrForcedState(sc, baseCDP), + "baseline windowState mismatch: chromium did not come up in the expected state — production behaviour relies on this invariant") + } + + // Resize up: must be a real delta from the baseline so the X root + // actually changes. Headful starts at Neko's default 1920x1080; + // headless starts at the WIDTH/HEIGHT env (1024x768). + upW, upH := sc.upSize() + patchDisplayExpectingOK(t, ctx, c, upW, upH, 60, sc.restartChromium, sc.restartChromiumSet) + if sc.assertXRoot { + waitForXRootResolution(t, ctx, c, upW, upH, 30*time.Second) + } + waitForRendererViewport(t, ctx, c, upW, upH, sc.predicate, sc.name+":up", 60*time.Second) + + cdpAfterUp, err := getChromiumWindowBoundsCDP(ctx, c) + require.NoError(t, err, "cdp window after up-resize") + t.Logf("[%s] after-up cdp=%+v", sc.name, cdpAfterUp) + + // Resize back down to a smaller size, also a real delta. + dnW, dnH := sc.downSize() + patchDisplayExpectingOK(t, ctx, c, dnW, dnH, 60, sc.restartChromium, sc.restartChromiumSet) + if sc.assertXRoot { + waitForXRootResolution(t, ctx, c, dnW, dnH, 30*time.Second) + } + waitForRendererViewport(t, ctx, c, dnW, dnH, sc.predicate, sc.name+":down", 60*time.Second) + + cdpAfterDown, err := getChromiumWindowBoundsCDP(ctx, c) + require.NoError(t, err, "cdp window after down-resize") + t.Logf("[%s] after-down cdp=%+v", sc.name, cdpAfterDown) + }) + } +} + + +// navigateBlank points the active page at about:blank via playwright so the +// renderer is alive before we query window dimensions. +func navigateBlank(t *testing.T, ctx context.Context, c *TestContainer) { + t.Helper() + client, err := c.APIClient() + require.NoError(t, err) + timeout := 5 + rsp, err := client.ExecutePlaywrightCodeWithResponse(ctx, instanceoapi.ExecutePlaywrightRequest{ + Code: `await page.goto('about:blank'); return true;`, + TimeoutSec: &timeout, + }) + require.NoError(t, err) + require.NotNil(t, rsp.JSON200, "playwright navigate response missing") + require.True(t, rsp.JSON200.Success, "playwright navigate to about:blank failed: %s", string(rsp.Body)) +} + +// patchDisplayExpectingOK issues PATCH /display and requires a 200. +// refreshRate is required for headful: the dummy Xorg DDX only has modelines +// named "WxH_RR.00", and the server's xrandr fallback (`xrandr -s WxH`) +// silently no-ops when refresh rate is omitted. +// +// restartChromiumSet=false leaves the field out of the request entirely so +// the server picks its own default (which after the no-restart change means +// the CDP re-assert-maximized path). +func patchDisplayExpectingOK(t *testing.T, ctx context.Context, c *TestContainer, width, height, refreshRate int, restartChromium, restartChromiumSet bool) { + t.Helper() + client, err := c.APIClient() + require.NoError(t, err) + rate := instanceoapi.PatchDisplayRequestRefreshRate(refreshRate) + req := instanceoapi.PatchDisplayJSONRequestBody{ + Width: &width, + Height: &height, + RefreshRate: &rate, + } + if restartChromiumSet { + req.RestartChromium = &restartChromium + } + rsp, err := client.PatchDisplayWithResponse(ctx, req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, rsp.StatusCode(), "PATCH /display: %s body=%s", rsp.Status(), string(rsp.Body)) + require.NotNil(t, rsp.JSON200) + require.NotNil(t, rsp.JSON200.Width) + require.NotNil(t, rsp.JSON200.Height) + require.Equal(t, width, *rsp.JSON200.Width) + require.Equal(t, height, *rsp.JSON200.Height) +} + +// waitForXRootResolution polls xrandr until the X root reaches the requested +// size, mirroring waitForXvfbResolution but operating on the live X root +// instead of the Xvfb process command line. +func waitForXRootResolution(t *testing.T, ctx context.Context, c *TestContainer, wantW, wantH int, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + for { + w, h, err := getXRootResolution(ctx, c) + if err == nil && w == wantW && h == wantH { + t.Logf("x_root_resolution: %dx%d (matches)", w, h) + return + } + if time.Now().After(deadline) { + t.Fatalf("x root never reached %dx%d: lastW=%d lastH=%d err=%v", wantW, wantH, w, h, err) + } + select { + case <-ctx.Done(): + t.Fatalf("context cancelled waiting for x root %dx%d: %v", wantW, wantH, ctx.Err()) + case <-time.After(250 * time.Millisecond): + } + } +} diff --git a/server/lib/cdpclient/cdpclient.go b/server/lib/cdpclient/cdpclient.go index 8d2c3c2e..840691e1 100644 --- a/server/lib/cdpclient/cdpclient.go +++ b/server/lib/cdpclient/cdpclient.go @@ -215,6 +215,74 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { return nil } +// SetWindowBoundsMaximized puts the OS window backing the first page target +// into the maximized state via Browser.setWindowBounds. It is idempotent — +// invoking it on a window already in maximized state is a no-op. +// +// A mutter-managed window in maximized state auto-tracks RANDR resizes +// (the WM reflows it to fill the new root). So after a display resize the +// server only has to make sure the window is in maximized state; mutter +// does the rest. This replaces the prior approach of restarting chromium +// so it could re-apply --start-maximized. +// +// We intentionally avoid the explicit-bounds form of setWindowBounds +// ({left, top, width, height} with windowState:"normal"): once a window is +// in normal state it stops auto-tracking subsequent RANDR events. +func (c *Client) SetWindowBoundsMaximized(ctx context.Context) error { + targetsResult, err := c.send(ctx, "Target.getTargets", nil, "") + if err != nil { + return fmt.Errorf("Target.getTargets: %w", err) + } + var targets struct { + TargetInfos []struct { + TargetID string `json:"targetId"` + Type string `json:"type"` + } `json:"targetInfos"` + } + if err := json.Unmarshal(targetsResult, &targets); err != nil { + return fmt.Errorf("unmarshal targets: %w", err) + } + var pageTargetID string + for _, t := range targets.TargetInfos { + if t.Type == "page" { + pageTargetID = t.TargetID + break + } + } + if pageTargetID == "" { + return fmt.Errorf("no page target found") + } + + winRaw, err := c.send(ctx, "Browser.getWindowForTarget", map[string]any{"targetId": pageTargetID}, "") + if err != nil { + return fmt.Errorf("Browser.getWindowForTarget: %w", err) + } + var winResp struct { + WindowID int `json:"windowId"` + Bounds struct { + WindowState string `json:"windowState"` + } `json:"bounds"` + } + if err := json.Unmarshal(winRaw, &winResp); err != nil { + return fmt.Errorf("unmarshal window: %w", err) + } + // Both "maximized" and "fullscreen" cause mutter to reflow the window + // to fill the new X root on RANDR — that's the only invariant we + // need. Demoting a kiosk fullscreen window to maximized would break + // kiosk mode, so leave fullscreen alone. + if winResp.Bounds.WindowState == "maximized" || winResp.Bounds.WindowState == "fullscreen" { + return nil + } + + if _, err := c.send(ctx, "Browser.setWindowBounds", map[string]any{ + "windowId": winResp.WindowID, + "bounds": map[string]any{"windowState": "maximized"}, + }, ""); err != nil { + return fmt.Errorf("Browser.setWindowBounds maximized: %w", err) + } + return nil +} + // SetDeviceMetricsOverride sets the viewport dimensions on the first page // target found in the browser. It attaches to the target with a flattened // session, sends Emulation.setDeviceMetricsOverride, then detaches. From 49dfe3f6abaef2b81008e871591346c6f2097af2 Mon Sep 17 00:00:00 2001 From: hiroTamada <88675973+hiroTamada@users.noreply.github.com> Date: Fri, 29 May 2026 19:30:55 +0000 Subject: [PATCH 2/4] Drop legacy chromium-restart branch on Xorg path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the prior commit: remove the restart_chromium=true escape hatch from the Xorg branch entirely. The CDP re-assert-maximized path is strictly better (faster, preserves browser state, stable windowId) and the e2e tests exercise it exclusively. Existing callers in kernel/kernel (kraft.go, chromium_configure.go) hardcode restart_chromium=true today and now transparently get the faster behaviour. The restart_chromium field stays in the request schema for API compatibility. On the Xorg branch it is now a no-op. The Xvfb-with- recordings branch (orthogonal — headless chromium has no OS window) still honours the flag. Also drop the unused restartChrome parameter from setResolutionXorgViaXrandr and its one caller in chromium_configure.go. Co-Authored-By: Claude Opus 4.7 --- server/cmd/api/api/chromium_configure.go | 2 +- server/cmd/api/api/display.go | 29 +++++++++++------------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index d4cbab72..6fff956f 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -692,7 +692,7 @@ func chromiumDisplayApplyWhileStopped(ctx context.Context, s *ApiService, plan * if s.isNekoEnabled() { err = s.setResolutionViaNeko(ctx, w, h, rr) } else { - err = s.setResolutionXorgViaXrandr(ctx, w, h, rr, false) + err = s.setResolutionXorgViaXrandr(ctx, w, h, rr) } if err != nil { return cfg500ConfigureStep(chromiumConfigureStepDisplay, err.Error()) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index b23e0734..607248c0 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -120,23 +120,20 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ err = s.setResolutionViaNeko(ctx, width, height, refreshRate) } else { log.Info("using xrandr for Xorg resolution change (Neko disabled)") - err = s.setResolutionXorgViaXrandr(ctx, width, height, refreshRate, restartChrome) + err = s.setResolutionXorgViaXrandr(ctx, width, height, refreshRate) } - // Headful default path: skip the chromium restart and re-assert - // the maximized window state via CDP instead. Mutter reflows a - // maximized window to fill the new X root automatically, so this - // is all that's needed — and it costs ~50ms vs ~9s for a restart. - // Callers can still opt into the legacy restart with - // restart_chromium=true (rare; preserved for compatibility). + // Re-assert the maximized window state via CDP after the X root + // resize. Mutter reflows a window in windowState=maximized (or + // fullscreen) to fill the new root automatically, so this single + // idempotent call is all we need post-resize. The previous + // approach of restarting chromium so it could re-apply + // --start-maximized cost ~9s per resize and also wiped browser- + // side state (Emulation.* overrides, devtools sessions). The + // restart_chromium request field is still accepted for API + // compatibility but no longer triggers a restart on this path. if err == nil { - if restartChrome { - if restartErr := s.restartChromiumAndWait(ctx, "resolution change"); restartErr != nil { - log.Error("failed to restart chromium after resolution change", "error", restartErr) - } - } else { - if cdpErr := s.setWindowMaximizedViaCDP(ctx); cdpErr != nil { - log.Warn("CDP maximize re-assert failed after Xorg resolution change (non-fatal)", "error", cdpErr) - } + if cdpErr := s.setWindowMaximizedViaCDP(ctx); cdpErr != nil { + log.Warn("CDP maximize re-assert failed after Xorg resolution change (non-fatal)", "error", cdpErr) } } } else if len(stopped) > 0 { @@ -236,7 +233,7 @@ func (s *ApiService) probeDisplayMode(ctx context.Context) string { } // setResolutionXorgViaXrandr changes resolution for Xorg using xrandr (fallback when Neko is disabled) -func (s *ApiService) setResolutionXorgViaXrandr(ctx context.Context, width, height, refreshRate int, restartChrome bool) error { +func (s *ApiService) setResolutionXorgViaXrandr(ctx context.Context, width, height, refreshRate int) error { log := logger.FromContext(ctx) display := s.resolveDisplayFromEnv() From 5b968a67dd00df5e4f71aa49db26c3a3899906b0 Mon Sep 17 00:00:00 2001 From: hiroTamada <88675973+hiroTamada@users.noreply.github.com> Date: Fri, 29 May 2026 20:00:20 +0000 Subject: [PATCH 3/4] Drop redundant headful subtests that flake under CI container churn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the legacy restart_chromium path was removed, two of the five TestDisplayResizeChromiumWindow scenarios became strictly redundant: - headful_kiosk_no_restart: passes restart_chromium=false explicitly, but the field is a no-op on the Xorg branch — functionally identical to headful_kiosk. - headful_force_maximized_no_restart: forces windowState=maximized via CDP at baseline, then resizes. Was useful while investigating whether mutter's "maximized window reflows on RANDR" invariant holds; headful_start_maximized now covers the same hot path with production-equivalent CHROMIUM_FLAGS. Both also flaked in CI when running later in a heavy e2e suite — neko/mutter occasionally fails to apply its 1920x1080 desktop.screen startup mode under load, leaving the X root at the dummy DDX's 3840x2160 default and breaking the resize assertions. Running them locally or earlier in the suite passes consistently, so it's an environmental flake, not a code regression. Also remove the now-unused helpers setChromiumWindowStateCDP, waitForWindowState, baselineOrForcedState, and the forceMaximizeAtBaseline / restartChromium fields on resizeScenario. The remaining three subtests cover every production path: headless CDP fast path, headful --start-maximized + neko, headful kiosk + fullscreen. Co-Authored-By: Claude Opus 4.7 --- server/e2e/e2e_display_resize_window_test.go | 170 ++----------------- 1 file changed, 18 insertions(+), 152 deletions(-) diff --git a/server/e2e/e2e_display_resize_window_test.go b/server/e2e/e2e_display_resize_window_test.go index fcb876f0..04c4707b 100644 --- a/server/e2e/e2e_display_resize_window_test.go +++ b/server/e2e/e2e_display_resize_window_test.go @@ -185,69 +185,6 @@ func getChromiumWindowBoundsCDP(ctx context.Context, c *TestContainer) (chromium }, nil } -// setChromiumWindowStateCDP forces the OS window to the given windowState -// ("normal" | "minimized" | "maximized" | "fullscreen") via Browser.setWindowBounds. -// Per CDP semantics: width/height/left/top fields must be omitted when -// windowState != "normal". -func setChromiumWindowStateCDP(ctx context.Context, c *TestContainer, state string) error { - cdp, err := newCDPClient(ctx, c.CDPURL()) - if err != nil { - return fmt.Errorf("dial cdp: %w", err) - } - defer cdp.Close() - - cur, err := getChromiumWindowBoundsCDP(ctx, c) - if err != nil { - return fmt.Errorf("get current window: %w", err) - } - - // CDP rejects state transitions like maximized → fullscreen directly; - // go through "normal" first when changing between non-normal states. - if cur.WindowState != "normal" && cur.WindowState != state { - if _, err := cdp.Call(ctx, "Browser.setWindowBounds", map[string]any{ - "windowId": cur.WindowID, - "bounds": map[string]any{"windowState": "normal"}, - }, ""); err != nil { - return fmt.Errorf("Browser.setWindowBounds normal: %w", err) - } - } - - if _, err := cdp.Call(ctx, "Browser.setWindowBounds", map[string]any{ - "windowId": cur.WindowID, - "bounds": map[string]any{"windowState": state}, - }, ""); err != nil { - return fmt.Errorf("Browser.setWindowBounds %s: %w", state, err) - } - return nil -} - -// waitForWindowState polls the chromium window state until it matches want -// or the timeout expires. Returns the final bounds. -func waitForWindowState(t *testing.T, ctx context.Context, c *TestContainer, want string, timeout time.Duration) chromiumWindowBounds { - t.Helper() - deadline := time.Now().Add(timeout) - var last chromiumWindowBounds - var lastErr error - for { - b, err := getChromiumWindowBoundsCDP(ctx, c) - lastErr = err - if err == nil { - last = b - if b.WindowState == want { - return b - } - } - if time.Now().After(deadline) { - t.Fatalf("window state never reached %q: last=%+v lastErr=%v", want, last, lastErr) - } - select { - case <-ctx.Done(): - t.Fatalf("context cancelled waiting for window state %q: %v", want, ctx.Err()) - case <-time.After(200 * time.Millisecond): - } - } -} - // viewportPredicate decides when a rendererViewport reading is "close enough" // to the requested size. The headless and headful paths use different // predicates because --headless=new uses Chrome's internal window size @@ -307,16 +244,6 @@ func makeHeadfulMaximizedPredicate(tolerance int) viewportPredicate { } } -// baselineOrForcedState returns the windowState the test expects the window -// to be in after baseline setup. When forceMaximizeAtBaseline is set, we -// re-query CDP rather than trust the pre-force reading. -func baselineOrForcedState(sc resizeScenario, base chromiumWindowBounds) string { - if sc.forceMaximizeAtBaseline { - return "maximized" - } - return base.WindowState -} - func abs(x int) int { if x < 0 { return -x @@ -341,19 +268,6 @@ type resizeScenario struct { // pick a different size to actually exercise the screen change. up [2]int down [2]int - // restartChromium is the value passed to PATCH /display. The default - // (false zero value when restartChromiumSet is true) exercises the - // no-restart path; setting restartChromium=true forces the current - // behaviour. restartChromiumSet must be true for the test to send the - // field; otherwise the request omits it entirely. - restartChromium bool - restartChromiumSet bool - // forceMaximizeAtBaseline calls Browser.setWindowBounds{windowState:"maximized"} - // before the first resize. Used to test whether mutter's "maximized - // window reflows on RANDR" invariant holds once we put the window in - // that state — bypasses the docker-specific question of whether - // --start-maximized actually produces a maximized window. - forceMaximizeAtBaseline bool // requireBaselineState asserts the window's CDP windowState at startup // equals this value before any resize. Empty = no assertion. requireBaselineState string @@ -415,52 +329,18 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { requireBaselineState: "maximized", }, { - name: "headful_kiosk", - image: headfulImage, - extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin", "CHROMIUM_FLAGS": "--kiosk --start-maximized"}, - // --kiosk runs the window in fullscreen state, which auto-tracks - // the screen size on every resize (verified: outer == screen - // after both up- and down-resize). Strict predicate works today. - predicate: makeHeadfulMaximizedPredicate(0), - assertXRoot: true, - up: [2]int{2560, 1440}, - down: [2]int{1280, 720}, - requireBaselineState: "fullscreen", - }, - { - // Direct test of the live-VM theory: kiosk's fullscreen window - // already auto-tracks RANDR resizes without any restart. Same - // scenario as headful_kiosk but with restart_chromium=false to - // confirm the restart is dead weight. - name: "headful_kiosk_no_restart", - image: headfulImage, - extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin", "CHROMIUM_FLAGS": "--kiosk --start-maximized"}, - predicate: makeHeadfulMaximizedPredicate(0), - assertXRoot: true, - up: [2]int{2560, 1440}, - down: [2]int{1280, 720}, - restartChromium: false, - restartChromiumSet: true, + // --kiosk runs the window in fullscreen state. mutter reflows + // the window to fill the new screen on every RANDR — verified + // here on both up- and down-resize. + name: "headful_kiosk", + image: headfulImage, + extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin", "CHROMIUM_FLAGS": "--kiosk --start-maximized"}, + predicate: makeHeadfulMaximizedPredicate(0), + assertXRoot: true, + up: [2]int{2560, 1440}, + down: [2]int{1280, 720}, requireBaselineState: "fullscreen", }, - { - // Test the live-VM theory's central claim: if the chromium - // window is in windowState=maximized, mutter reflows it on - // RANDR — no Chromium restart needed. Bypasses the docker-only - // quirk where --start-maximized comes up in 'normal' state by - // forcing the window state via CDP at baseline. - name: "headful_force_maximized_no_restart", - image: headfulImage, - extraEnv: map[string]string{"ENABLE_WEBRTC": "true", "NEKO_ADMIN_PASSWORD": "admin"}, - predicate: makeHeadfulMaximizedPredicate(0), - assertXRoot: true, - up: [2]int{2560, 1440}, - down: [2]int{1280, 720}, - restartChromium: false, - restartChromiumSet: true, - forceMaximizeAtBaseline: true, - requireBaselineState: "maximized", - }, } // Subtests run serially: two of the three boot the heavy headful image @@ -513,18 +393,8 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { require.NoError(t, err, "baseline cdp window") t.Logf("[%s] baseline cdp=%+v", sc.name, baseCDP) - // Optionally force the window into the maximized state at - // baseline. Tests the live-VM theory's invariant in - // environments where --start-maximized somehow doesn't produce - // a windowState=maximized window at startup (observed: docker). - if sc.forceMaximizeAtBaseline { - require.NoError(t, setChromiumWindowStateCDP(ctx, c, "maximized"), "force maximized via CDP") - forced := waitForWindowState(t, ctx, c, "maximized", 5*time.Second) - t.Logf("[%s] after force-maximize cdp=%+v", sc.name, forced) - } - if sc.requireBaselineState != "" { - require.Equal(t, sc.requireBaselineState, baselineOrForcedState(sc, baseCDP), + require.Equal(t, sc.requireBaselineState, baseCDP.WindowState, "baseline windowState mismatch: chromium did not come up in the expected state — production behaviour relies on this invariant") } @@ -532,7 +402,7 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { // actually changes. Headful starts at Neko's default 1920x1080; // headless starts at the WIDTH/HEIGHT env (1024x768). upW, upH := sc.upSize() - patchDisplayExpectingOK(t, ctx, c, upW, upH, 60, sc.restartChromium, sc.restartChromiumSet) + patchDisplayExpectingOK(t, ctx, c, upW, upH, 60) if sc.assertXRoot { waitForXRootResolution(t, ctx, c, upW, upH, 30*time.Second) } @@ -544,7 +414,7 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { // Resize back down to a smaller size, also a real delta. dnW, dnH := sc.downSize() - patchDisplayExpectingOK(t, ctx, c, dnW, dnH, 60, sc.restartChromium, sc.restartChromiumSet) + patchDisplayExpectingOK(t, ctx, c, dnW, dnH, 60) if sc.assertXRoot { waitForXRootResolution(t, ctx, c, dnW, dnH, 30*time.Second) } @@ -574,15 +444,14 @@ func navigateBlank(t *testing.T, ctx context.Context, c *TestContainer) { require.True(t, rsp.JSON200.Success, "playwright navigate to about:blank failed: %s", string(rsp.Body)) } -// patchDisplayExpectingOK issues PATCH /display and requires a 200. +// patchDisplayExpectingOK issues PATCH /display and requires a 200. The +// request omits restart_chromium so the server picks its own default — the +// CDP re-assert-maximized path on the Xorg branch. +// // refreshRate is required for headful: the dummy Xorg DDX only has modelines // named "WxH_RR.00", and the server's xrandr fallback (`xrandr -s WxH`) // silently no-ops when refresh rate is omitted. -// -// restartChromiumSet=false leaves the field out of the request entirely so -// the server picks its own default (which after the no-restart change means -// the CDP re-assert-maximized path). -func patchDisplayExpectingOK(t *testing.T, ctx context.Context, c *TestContainer, width, height, refreshRate int, restartChromium, restartChromiumSet bool) { +func patchDisplayExpectingOK(t *testing.T, ctx context.Context, c *TestContainer, width, height, refreshRate int) { t.Helper() client, err := c.APIClient() require.NoError(t, err) @@ -592,9 +461,6 @@ func patchDisplayExpectingOK(t *testing.T, ctx context.Context, c *TestContainer Height: &height, RefreshRate: &rate, } - if restartChromiumSet { - req.RestartChromium = &restartChromium - } rsp, err := client.PatchDisplayWithResponse(ctx, req) require.NoError(t, err) require.Equal(t, http.StatusOK, rsp.StatusCode(), "PATCH /display: %s body=%s", rsp.Status(), string(rsp.Body)) From 9f62371fe48d485bc84ca5e1f692bacb9d9df6db Mon Sep 17 00:00:00 2001 From: hiroTamada <88675973+hiroTamada@users.noreply.github.com> Date: Fri, 29 May 2026 21:16:45 +0000 Subject: [PATCH 4/4] Fix xrandr output, fail closed on CDP, assert windowID/state stable Three follow-ups addressing review feedback: 1. xrandr targets DUMMY0 (or KERNEL_IMAGES_XRANDR_OUTPUT env override), not "default". The headful Xorg dummy driver does not expose a "default" output, so `xrandr --output default --mode ...` exits 0 without applying anything. This affects only the non-Neko Xorg path (production runs with Neko enabled, which doesn't touch xrandr), but left the path silently broken. 2. CDP maximize re-assert failure is now fatal. Previously logged a warning and returned 200; if the window happened to be in normal state and the CDP call failed, the server returned success with the browser window stuck at the old size. Now the resize fails closed with a 500 so the caller sees the mismatch. 3. TestDisplayResizeChromiumWindow now asserts windowID stability and windowState preservation across both resizes. Previously logged but never asserted; a silent chromium restart regression would have slipped through. Co-Authored-By: Claude Opus 4.7 --- server/cmd/api/api/display.go | 24 ++++++++++++++++---- server/e2e/e2e_display_resize_window_test.go | 8 +++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 607248c0..dc2bec77 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -131,9 +131,16 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ // side state (Emulation.* overrides, devtools sessions). The // restart_chromium request field is still accepted for API // compatibility but no longer triggers a restart on this path. + // + // The CDP call is the only thing that recovers a window already + // in the "normal" state, so its failure is fatal: returning 200 + // after a CDP error could leave the browser window stuck at the + // old size while the X root is at the new size, and the caller + // would have no signal of the mismatch. if err == nil { if cdpErr := s.setWindowMaximizedViaCDP(ctx); cdpErr != nil { - log.Warn("CDP maximize re-assert failed after Xorg resolution change (non-fatal)", "error", cdpErr) + log.Error("CDP maximize re-assert failed after Xorg resolution change", "error", cdpErr) + err = fmt.Errorf("CDP maximize re-assert failed: %w", cdpErr) } } } else if len(stopped) > 0 { @@ -237,14 +244,23 @@ func (s *ApiService) setResolutionXorgViaXrandr(ctx context.Context, width, heig log := logger.FromContext(ctx) display := s.resolveDisplayFromEnv() + // The headful Xorg dummy driver exposes DUMMY0, not "default". The + // historical `xrandr --output default --mode ...` command exits 0 while + // silently doing nothing on this driver. Default to DUMMY0 and let an + // env var override it for any non-standard image layout. + output := strings.TrimSpace(os.Getenv("KERNEL_IMAGES_XRANDR_OUTPUT")) + if output == "" { + output = "DUMMY0" + } + // Build xrandr command - if refresh rate is specified, use the specific modeline var xrandrCmd string if refreshRate > 0 { modeName := fmt.Sprintf("%dx%d_%d.00", width, height, refreshRate) - xrandrCmd = fmt.Sprintf("xrandr --output default --mode %s", modeName) - log.Info("using specific modeline", "mode", modeName) + xrandrCmd = fmt.Sprintf("xrandr --output %s --mode %s", output, modeName) + log.Info("using specific modeline", "output", output, "mode", modeName) } else { - xrandrCmd = fmt.Sprintf("xrandr -s %dx%d", width, height) + xrandrCmd = fmt.Sprintf("xrandr --output %s --size %dx%d", output, width, height) } args := []string{"-lc", xrandrCmd} diff --git a/server/e2e/e2e_display_resize_window_test.go b/server/e2e/e2e_display_resize_window_test.go index 04c4707b..c2a8199b 100644 --- a/server/e2e/e2e_display_resize_window_test.go +++ b/server/e2e/e2e_display_resize_window_test.go @@ -411,6 +411,10 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { cdpAfterUp, err := getChromiumWindowBoundsCDP(ctx, c) require.NoError(t, err, "cdp window after up-resize") t.Logf("[%s] after-up cdp=%+v", sc.name, cdpAfterUp) + require.Equal(t, baseCDP.WindowID, cdpAfterUp.WindowID, + "windowID changed after up-resize — chromium was restarted (windowID is monotonic per-process; a new one signals a relaunch)") + require.Equal(t, baseCDP.WindowState, cdpAfterUp.WindowState, + "windowState changed after up-resize — the WM-tracking invariant the no-restart path relies on was broken") // Resize back down to a smaller size, also a real delta. dnW, dnH := sc.downSize() @@ -423,6 +427,10 @@ func TestDisplayResizeChromiumWindow(t *testing.T) { cdpAfterDown, err := getChromiumWindowBoundsCDP(ctx, c) require.NoError(t, err, "cdp window after down-resize") t.Logf("[%s] after-down cdp=%+v", sc.name, cdpAfterDown) + require.Equal(t, baseCDP.WindowID, cdpAfterDown.WindowID, + "windowID changed after down-resize — chromium was restarted between resizes") + require.Equal(t, baseCDP.WindowState, cdpAfterDown.WindowState, + "windowState changed after down-resize") }) } }