From f07dd1f22666f182893a8eeefa1e8e602d536e6a Mon Sep 17 00:00:00 2001 From: tnsardesai <18272584+tnsardesai@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:20:41 +0000 Subject: [PATCH] Give a starting Chromium longer to observe the browser location timezone --- server/cmd/api/api/browser_location.go | 33 +++++++++++++++++-- server/cmd/api/api/browser_location_test.go | 36 +++++++++++++++++++++ server/lib/devtoolsproxy/proxy.go | 14 ++++++++ 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/server/cmd/api/api/browser_location.go b/server/cmd/api/api/browser_location.go index f617f1f6..65fa4ce8 100644 --- a/server/cmd/api/api/browser_location.go +++ b/server/cmd/api/api/browser_location.go @@ -21,6 +21,11 @@ import ( const ( browserLocationApplyTimeout = 5 * time.Second + // browserLocationStartupTimeout bounds how long after its DevTools endpoint + // comes up Chromium may take to notice /etc/localtime being replaced. Until + // then it ignores the replacement, and one restoring a slow saved tab can + // take longer than browserLocationApplyTimeout. + browserLocationStartupTimeout = 10 * time.Second // browserLocationRoundTimeout bounds how long one attempt waits for // Chromium's acknowledgements before it sends the location again. browserLocationRoundTimeout = 1500 * time.Millisecond @@ -143,7 +148,7 @@ func (s *ApiService) startBrowserLocationReconcileLocked(bundle oapi.BrowserLoca func (s *ApiService) reconcileBrowserLocation(ctx context.Context, bundle oapi.BrowserLocationBundle) { started := time.Now() - deadline := started.Add(browserLocationApplyTimeout) + deadline := browserLocationDeadline(started, s.chromiumSince()) for { if ctx.Err() != nil || !s.browserLocationCurrent(bundle) { return @@ -183,8 +188,9 @@ func (s *ApiService) reconcileBrowserLocation(ctx context.Context, bundle oapi.B s.browserLocation.lastError = err.Error() s.browserLocation.failures.Add(1) } + components := s.browserLocation.components s.browserLocationMu.Unlock() - logger.FromContext(ctx).Error("browser location did not converge", "error", err, "generation", bundle.Generation) + logger.FromContext(ctx).Error("browser location did not converge", "error", err, "generation", bundle.Generation, "components", components) return } s.browserLocation.retries.Add(1) @@ -196,6 +202,27 @@ func (s *ApiService) reconcileBrowserLocation(ctx context.Context, bundle oapi.B } } +// browserLocationDeadline is when an attempt that started at started stops +// retrying: browserLocationApplyTimeout after it started, or +// browserLocationStartupTimeout after Chromium's DevTools endpoint came up at +// chromiumSince, whichever is later. +func browserLocationDeadline(started, chromiumSince time.Time) time.Time { + deadline := started.Add(browserLocationApplyTimeout) + if startup := chromiumSince.Add(browserLocationStartupTimeout); startup.After(deadline) { + return startup + } + return deadline +} + +// chromiumSince is when the running Chromium's DevTools endpoint came up, or +// the zero time if it is unknown. +func (s *ApiService) chromiumSince() time.Time { + if s.upstreamMgr == nil { + return time.Time{} + } + return s.upstreamMgr.Since() +} + // watchPendingRenderers follows the renderers that were pending when bundle // applied until they acknowledge it. If the browser then reports a component // that didn't converge, the location is no longer applied and is sent again. @@ -597,7 +624,7 @@ func browserLocationError(message string) *string { // waitBrowserLocationApplied waits until the running browser confirms bundle, // or reports why it didn't. func (s *ApiService) waitBrowserLocationApplied(ctx context.Context, bundle oapi.BrowserLocationBundle) error { - ctx, cancel := context.WithTimeout(ctx, browserLocationApplyTimeout+time.Second) + ctx, cancel := context.WithDeadline(ctx, browserLocationDeadline(time.Now(), s.chromiumSince()).Add(time.Second)) defer cancel() ticker := time.NewTicker(25 * time.Millisecond) defer ticker.Stop() diff --git a/server/cmd/api/api/browser_location_test.go b/server/cmd/api/api/browser_location_test.go index 39a9ad2d..257a9ffd 100644 --- a/server/cmd/api/api/browser_location_test.go +++ b/server/cmd/api/api/browser_location_test.go @@ -499,6 +499,42 @@ func TestSetBrowserLocationAllowsPendingRenderers(t *testing.T) { assert.Equal(t, 1, status.Components.RenderersPending) } +// Chromium ignores /etc/localtime being replaced until it has finished +// starting, which restoring a slow saved tab delays past +// browserLocationApplyTimeout. A location sent to a Chromium that just started +// keeps replacing it until Chromium observes it. +func TestSetBrowserLocationWaitsForStartingChromiumToObserveTimezone(t *testing.T) { + service := newBrowserLocationStateService(t) + observesFrom := time.Now().Add(browserLocationApplyTimeout + 500*time.Millisecond) + var observed atomic.Bool + previous := setBrowserLocaltime + setBrowserLocaltime = func(string) error { + if time.Now().After(observesFrom) { + observed.Store(true) + } + return nil + } + t.Cleanup(func() { setBrowserLocaltime = previous }) + fakeLocationChromium(t, service, func() map[string]any { + return map[string]any{"renderersConverged": true, "renderersPending": 0, "timezoneConverged": observed.Load()} + }) + + resp := service.applyBrowserLocation(context.Background(), testLocationBundle(1)) + + require.IsType(t, oapi.SetBrowserLocation200JSONResponse{}, resp, "error: %s", snapshotError(service)) + assert.True(t, observed.Load()) +} + +func TestBrowserLocationDeadline(t *testing.T) { + started := time.Now() + + assert.Equal(t, started.Add(browserLocationApplyTimeout), browserLocationDeadline(started, time.Time{}), "an unknown Chromium gets the apply timeout") + settled := started.Add(-time.Minute) + assert.Equal(t, started.Add(browserLocationApplyTimeout), browserLocationDeadline(started, settled), "a Chromium past startup gets the apply timeout") + starting := started.Add(-time.Second) + assert.Equal(t, starting.Add(browserLocationStartupTimeout), browserLocationDeadline(started, starting), "a starting Chromium gets until it is past startup") +} + func TestBrowserLocationConfirmed(t *testing.T) { converged := oapi.BrowserLocationComponents{Browser: true, Timezone: true, Renderers: true, NetworkContexts: true} pending := converged diff --git a/server/lib/devtoolsproxy/proxy.go b/server/lib/devtoolsproxy/proxy.go index aa09b41d..ead51385 100644 --- a/server/lib/devtoolsproxy/proxy.go +++ b/server/lib/devtoolsproxy/proxy.go @@ -101,6 +101,9 @@ type UpstreamManager struct { logger *slog.Logger currentURL atomic.Value // string + // currentSince is when currentURL last changed, in wall-clock Unix + // nanoseconds. + currentSince atomic.Int64 startOnce sync.Once stopOnce sync.Once @@ -154,10 +157,21 @@ func (u *UpstreamManager) Current() string { return val } +// Since returns when the current upstream URL was discovered, or the zero +// time if none has been. +func (u *UpstreamManager) Since() time.Time { + since := u.currentSince.Load() + if since == 0 { + return time.Time{} + } + return time.Unix(0, since) +} + func (u *UpstreamManager) setCurrent(url string) { prev := u.Current() if url != "" && url != prev { u.logger.Info("devtools upstream updated", slog.String("url", url)) + u.currentSince.Store(time.Now().UnixNano()) u.currentURL.Store(url) // Broadcast update to subscribers without blocking. If a subscriber's // channel buffer (size 1) is full, replace the buffered value with the