diff --git a/internal/driver/chrome/driver.go b/internal/driver/chrome/driver.go index 4914672..3137afd 100644 --- a/internal/driver/chrome/driver.go +++ b/internal/driver/chrome/driver.go @@ -97,19 +97,31 @@ func New() *Driver { } func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ map[string]string) error { + // Allocate the browser against the driver's own context before anything + // caller-bound runs. chromedp starts Chrome under whichever context first + // calls Run, so allocating under a caller deadline would tie the browser + // process to this one call and kill it the moment Launch returns. + if err := chromedp.Run(d.tabCtx); err != nil { + return err + } + // Everything after allocation goes through runCtx, so a caller deadline or + // a SIGTERM aborts a launch that would otherwise wait forever on a target + // that accepts the connection and never answers. + runCtx, cancel := d.runCtx(ctx) + defer cancel() if clearState { - if err := d.clearState(bundleID); err != nil { + if err := d.clearState(runCtx, bundleID); err != nil { return err } } - if err := chromedp.Run(d.tabCtx, chromedp.Navigate(bundleID)); err != nil { + if err := chromedp.Run(runCtx, chromedp.Navigate(bundleID)); err != nil { return err } // After navigation, read CSS custom properties --frame-w / --frame-h (common // mobile-frame convention) so screenshots fit the app without grey borders. // Falls back to the body scroll dimensions if the properties are absent. var dims [2]int64 - if err := chromedp.Run(d.tabCtx, chromedp.Evaluate(` + if err := chromedp.Run(runCtx, chromedp.Evaluate(` (function() { const s = getComputedStyle(document.documentElement); const pw = parseInt(s.getPropertyValue('--frame-w'), 10); @@ -118,7 +130,7 @@ func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ const h = isNaN(ph) ? document.body.scrollHeight : ph; return [w, h]; })()`, &dims)); err == nil && dims[0] > 0 && dims[1] > 0 { - _ = chromedp.Run(d.tabCtx, chromedp.EmulateViewport(dims[0], dims[1])) + _ = chromedp.Run(runCtx, chromedp.EmulateViewport(dims[0], dims[1])) } return nil } @@ -130,8 +142,8 @@ func (d *Driver) Launch(ctx context.Context, bundleID string, clearState bool, _ // which needs no navigation. sessionStorage is per-tab and outside that // domain's reach; it only survives when a relaunch reuses a tab already on // the target origin, which is the one case where script can reach it. -func (d *Driver) clearState(bundleID string) error { - if err := chromedp.Run(d.tabCtx, network.ClearBrowserCookies()); err != nil { +func (d *Driver) clearState(runCtx context.Context, bundleID string) error { + if err := chromedp.Run(runCtx, network.ClearBrowserCookies()); err != nil { return fmt.Errorf("clear cookies: %w", err) } origin := securityOrigin(bundleID) @@ -139,12 +151,12 @@ func (d *Driver) clearState(bundleID string) error { return nil } clearForOrigin := storage.ClearDataForOrigin(origin, string(storage.TypeAll)) - if err := chromedp.Run(d.tabCtx, clearForOrigin); err != nil { + if err := chromedp.Run(runCtx, clearForOrigin); err != nil { return fmt.Errorf("clear storage for %s: %w", origin, err) } script := fmt.Sprintf( `location.origin === %q && (sessionStorage.clear(), true)`, origin) - return chromedp.Run(d.tabCtx, chromedp.ActionFunc(func(ctx context.Context) error { + return chromedp.Run(runCtx, chromedp.ActionFunc(func(ctx context.Context) error { _, exception, err := runtime.Evaluate(script).Do(ctx) if err != nil { return fmt.Errorf("clear session storage: %w", err) diff --git a/internal/driver/chrome/driver_test.go b/internal/driver/chrome/driver_test.go index 7f21087..d561f6f 100644 --- a/internal/driver/chrome/driver_test.go +++ b/internal/driver/chrome/driver_test.go @@ -6,6 +6,7 @@ import ( "context" "encoding/json" "errors" + "net" "net/http" "net/http/httptest" "testing" @@ -398,3 +399,78 @@ func TestHierarchy_RootsAtDocumentElementWithoutHead(t *testing.T) { } } } + +// TestLaunch_HonorsCallerDeadline points the driver at a listener that accepts +// the connection and never answers. Chrome has no page-load deadline of its +// own, so a Launch that ignored its caller context waited forever: an +// unattended campaign worker aimed at an unreachable target wedged with no +// diagnostic and did not even answer SIGTERM. +func TestLaunch_HonorsCallerDeadline(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer listener.Close() + held := make(chan net.Conn, 8) + go func() { + for { + conn, err := listener.Accept() + if err != nil { + return + } + held <- conn + } + }() + defer func() { + close(held) + for conn := range held { + conn.Close() + } + }() + + d := New() + defer d.Terminate(context.Background()) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + done := make(chan error, 1) + go func() { done <- d.Launch(ctx, "http://"+listener.Addr().String(), false, nil) }() + select { + case err := <-done: + if err == nil { + t.Fatal("Launch returned nil against a target that never answers") + } + if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, context.Canceled) { + t.Fatalf("Launch error = %v, want a context error", err) + } + case <-time.After(45 * time.Second): + t.Fatal("Launch ignored the caller deadline and blocked") + } +} + +// TestLaunch_KeepsBrowserAliveAfterCallerContextEnds guards the trap that made +// Launch use the driver context in the first place: chromedp starts Chrome +// under whichever context runs first, so allocating under the caller's +// short-lived context would kill the browser as soon as Launch returned. +func TestLaunch_KeepsBrowserAliveAfterCallerContextEnds(t *testing.T) { + d := New() + defer d.Terminate(context.Background()) + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + if err := d.Launch(ctx, "data:text/html,alive", false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + cancel() + + var text string + if err := chromedp.Run(d.tabCtx, + chromedp.Evaluate(`document.body.textContent`, &text)); err != nil { + t.Fatalf("browser died with the caller context: %v", err) + } + if text != "alive" { + t.Errorf("body text = %q, want \"alive\"", text) + } + if err := d.Launch(context.Background(), "data:text/html,again", false, nil); err != nil { + t.Fatalf("second Launch after the first caller context ended: %v", err) + } +}