diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index 7d995314..ff7f6cb4 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -24,7 +24,13 @@ import ( const userDataProfileDir = "/home/kernel/user-data" const maxStartURLLen = 2048 -const startURLDispatchTimeout = 3 * time.Second + +// startURLNavigateTimeout bounds the start_url navigation. Page.navigate only +// resolves once the navigation commits, so this budget has to cover the origin's +// DNS, connect and first byte; the previous 3s budget expired against slow +// origins and handed the session over still showing the pre-navigation page. It +// does not have to cover subresource loading, which continues after commit. +const startURLNavigateTimeout = 15 * time.Second type chromiumConfigureState struct { displayJSON *string @@ -181,8 +187,19 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu } if spec.needsNav { - if err := chromiumDoNavigate(ctx, s, spec); err != nil { - logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err) + // A failed navigation leaves the session on whatever the browser was + // showing, so log it and keep going rather than discarding a browser over + // a start_url the caller may not control. + navStart := time.Now() + navErrorText, navErr := chromiumDoNavigate(ctx, s, spec) + if navErr == nil && navErrorText != "" { + navErr = errors.New(navErrorText) + } + if navErr != nil { + logger.FromContext(ctx).Warn("start_url dispatch failed", + "error", navErr, + "elapsed", time.Since(navStart).String(), + "timeout", startURLNavigateTimeout.String()) } } @@ -234,12 +251,14 @@ func normalizeStartURL(rawURL string) string { return rawURL } -func chromiumDoNavigate(ctx context.Context, s *ApiService, spec startURLParsed) error { +// chromiumDoNavigate navigates to spec.url and returns Chrome's navigation +// errorText, which is non-empty when an error page committed instead. +func chromiumDoNavigate(ctx context.Context, s *ApiService, spec startURLParsed) (string, error) { upstream := s.upstreamMgr.Current() if upstream == "" { - return fmt.Errorf("devtools upstream not available") + return "", fmt.Errorf("devtools upstream not available") } - navCtx, cancel := context.WithTimeout(ctx, startURLDispatchTimeout) + navCtx, cancel := context.WithTimeout(ctx, startURLNavigateTimeout) defer cancel() return cdpclient.DispatchStartURL(navCtx, upstream, spec.url) } diff --git a/server/cmd/wrapper/snapshot_start_page.go b/server/cmd/wrapper/snapshot_start_page.go index b1c25afe..7e1401e2 100644 --- a/server/cmd/wrapper/snapshot_start_page.go +++ b/server/cmd/wrapper/snapshot_start_page.go @@ -62,7 +62,7 @@ func prepareSnapshotStartPage(ctx context.Context, internalPort string) (retErr logf("WARNING: snapshot start page unavailable, using about:blank: %v", navErr) blankCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() - if err := cdpclient.DispatchStartURL(blankCtx, devtoolsURL, "about:blank"); err != nil { + if _, err := cdpclient.DispatchStartURL(blankCtx, devtoolsURL, "about:blank"); err != nil { return fmt.Errorf("reset snapshot start page: %w", err) } return nil diff --git a/server/lib/cdpclient/cdpclient.go b/server/lib/cdpclient/cdpclient.go index a59073c5..1cf89cbf 100644 --- a/server/lib/cdpclient/cdpclient.go +++ b/server/lib/cdpclient/cdpclient.go @@ -220,19 +220,26 @@ func (c *Client) CountPageTargets(ctx context.Context) (int, error) { return n, nil } -// DispatchStartURL closes extra page targets and dispatches a navigation on the -// first page target. It does not wait for lifecycle events; Chrome owns the -// eventual navigation result. -func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { +// DispatchStartURL closes extra page targets and navigates the first page +// target. Page.navigate resolves once the navigation commits, so this returns +// after the new document has replaced the old one but before subresources +// finish loading. +// +// The first return value is Chrome's navigation errorText, non-empty when the +// navigation committed an error page instead of the requested URL (an +// unreachable host, for example). Callers that need the requested page to have +// actually loaded must check it; a nil error alone only means the command was +// accepted. +func DispatchStartURL(ctx context.Context, devtoolsURL, url string) (string, error) { c, err := Dial(ctx, devtoolsURL) if err != nil { - return fmt.Errorf("dial devtools: %w", err) + return "", fmt.Errorf("dial devtools: %w", err) } defer c.Close() targetsResult, err := c.send(ctx, "Target.getTargets", nil, "") if err != nil { - return fmt.Errorf("Target.getTargets: %w", err) + return "", fmt.Errorf("Target.getTargets: %w", err) } var targets struct { @@ -242,7 +249,7 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { } `json:"targetInfos"` } if err := json.Unmarshal(targetsResult, &targets); err != nil { - return fmt.Errorf("unmarshal targets: %w", err) + return "", fmt.Errorf("unmarshal targets: %w", err) } var pageTargetID string @@ -263,13 +270,13 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { "url": "about:blank", }, "") if err != nil { - return fmt.Errorf("Target.createTarget: %w", err) + return "", fmt.Errorf("Target.createTarget: %w", err) } var created struct { TargetID string `json:"targetId"` } if err := json.Unmarshal(createResult, &created); err != nil { - return fmt.Errorf("unmarshal create target: %w", err) + return "", fmt.Errorf("unmarshal create target: %w", err) } pageTargetID = created.TargetID } @@ -279,14 +286,14 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { "flatten": true, }, "") if err != nil { - return fmt.Errorf("Target.attachToTarget: %w", err) + return "", fmt.Errorf("Target.attachToTarget: %w", err) } var attach struct { SessionID string `json:"sessionId"` } if err := json.Unmarshal(attachResult, &attach); err != nil { - return fmt.Errorf("unmarshal attach: %w", err) + return "", fmt.Errorf("unmarshal attach: %w", err) } defer func() { detachCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -296,16 +303,25 @@ func DispatchStartURL(ctx context.Context, devtoolsURL, url string) error { }, "") }() - if _, err := c.send(ctx, "Page.navigate", map[string]any{"url": url}, attach.SessionID); err != nil { - return fmt.Errorf("Page.navigate: %w", err) + navResult, err := c.send(ctx, "Page.navigate", map[string]any{"url": url}, attach.SessionID) + if err != nil { + return "", fmt.Errorf("Page.navigate: %w", err) } - return nil + var nav struct { + ErrorText string `json:"errorText"` + } + if err := json.Unmarshal(navResult, &nav); err != nil { + return "", fmt.Errorf("unmarshal navigate result: %w", err) + } + return nav.ErrorText, nil } // DispatchStartURLAndWait navigates through navigationURL and waits for // destination to load without resolving to Chrome's network error page. func DispatchStartURLAndWait(ctx context.Context, devtoolsURL, navigationURL, destination string) error { - if err := DispatchStartURL(ctx, devtoolsURL, navigationURL); err != nil { + // The polling loop below re-navigates through chrome-error:// pages, so a + // reported navigation error is not terminal here. + if _, err := DispatchStartURL(ctx, devtoolsURL, navigationURL); err != nil { return err } diff --git a/server/lib/cdpclient/cdpclient_test.go b/server/lib/cdpclient/cdpclient_test.go index 8154b361..54cb1d69 100644 --- a/server/lib/cdpclient/cdpclient_test.go +++ b/server/lib/cdpclient/cdpclient_test.go @@ -34,6 +34,8 @@ type fakeCDP struct { navigateCalled bool navigateCalls int navigateURL string + navigateErrorText string + navigateBlock time.Duration pageStates []string pageStateIndex int } @@ -115,7 +117,20 @@ func (f *fakeCDP) handler(w http.ResponseWriter, r *http.Request) { var params map[string]any _ = json.Unmarshal(req.Params, ¶ms) f.navigateURL, _ = params["url"].(string) - result = map[string]any{"frameId": "frame-1"} + // Page.navigate resolves on commit, so a slow origin holds the + // response open rather than replying and navigating later. + if f.navigateBlock > 0 { + select { + case <-time.After(f.navigateBlock): + case <-ctx.Done(): + return + } + } + nav := map[string]any{"frameId": "frame-1"} + if f.navigateErrorText != "" { + nav["errorText"] = f.navigateErrorText + } + result = nav case "Runtime.evaluate": state := `{"url":"about:blank","readyState":"loading"}` if len(f.pageStates) > 0 { @@ -235,6 +250,54 @@ func TestSetDeviceMetricsOverride(t *testing.T) { }) } +func TestDispatchStartURL(t *testing.T) { + t.Run("reports no error text on a committed navigation", func(t *testing.T) { + f := &fakeCDP{pageTargetID: "target-123", sessionID: "session-abc"} + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + errorText, err := DispatchStartURL(ctx, url, "https://example.com/") + require.NoError(t, err) + assert.Empty(t, errorText) + assert.Equal(t, "https://example.com/", f.navigateURL) + }) + + // Chrome answers Page.navigate with an errorText and commits an error page + // when the requested URL cannot load, which must not read as success. + t.Run("surfaces the navigation error text", func(t *testing.T) { + f := &fakeCDP{ + pageTargetID: "target-123", + sessionID: "session-abc", + navigateErrorText: "net::ERR_CONNECTION_REFUSED", + } + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + errorText, err := DispatchStartURL(ctx, url, "https://unreachable.example/") + require.NoError(t, err) + assert.Equal(t, "net::ERR_CONNECTION_REFUSED", errorText) + }) + + // When the origin is slower to commit than the caller's budget, the caller + // has to see the failure rather than assume the page was replaced. + t.Run("errors when the navigation does not commit within the budget", func(t *testing.T) { + f := &fakeCDP{ + pageTargetID: "target-123", + sessionID: "session-abc", + navigateBlock: 2 * time.Second, + } + url := startFakeCDP(t, f) + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + _, err := DispatchStartURL(ctx, url, "https://slow.example/") + require.Error(t, err) + assert.Contains(t, err.Error(), "Page.navigate") + }) +} + func TestDispatchStartURLAndWait(t *testing.T) { t.Run("waits for loaded destination", func(t *testing.T) { f := &fakeCDP{ diff --git a/server/openapi.yaml b/server/openapi.yaml index 20dfdef9..450e1050 100644 --- a/server/openapi.yaml +++ b/server/openapi.yaml @@ -1253,9 +1253,12 @@ paths: description: | Optional multipart parts apply configuration while Chromium stays stopped once (policy, flags, extensions, profile archive, optional display sizing), then Chromium is started exactly - once and DevTools readiness is awaited. Optional `start_url` dispatches a best-effort - navigation after readiness without waiting for page load. Bare hosts are normalized to - `https://`. Omit any part you do not need. At least one actionable part must be present. + once and DevTools readiness is awaited. Optional `start_url` is navigated after readiness and + awaited until the navigation commits, so the requested document has replaced whatever the + browser was showing; subresources may still be loading. A navigation that fails or exceeds + the internal budget is logged and still returns 200, leaving the browser on its + pre-navigation page. Bare hosts are normalized to `https://`. Omit any part you do not need. + At least one actionable part must be present. Required configuration steps run in this order: policies, extensions, display, flags, then profile archive. Chromium is started only once at the end. The endpoint is not transactional: if a later required step fails, earlier successful side effects may remain @@ -1315,7 +1318,7 @@ paths: required: [zip_file, name] responses: "200": - description: Configuration applied; optional navigate completed successfully. + description: Configuration applied; optional navigate committed, failed, or timed out. content: application/json: schema: