fix: harden async callbacks with URI check and timer generation counter
Add URI comparison in favicon GetOrFetch callback to prevent a stale fetch from overwriting a newer navigation's favicon. Add generation counter to ModalState so stale timer callbacks (from time.Timer.Stop races) are ignored in startTimeoutLocked.
- commit
- 47070ab664e00df9d2bc6b68c63ab3ddfab152cb
- author
- brice · · 2026-03-13 07:15 UTC
- parent
- ff5eb4f
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +22 -4
M internal/ui/coordinator/content.go +5 -0
| @@ -1090,6 +1090,7 @@ func (c *ContentCoordinator) resolveCommittedFavicon(ctx context.Context, paneID | |||
| 1090 | 1090 | // Fall back to GetOrFetch (checks service cache, WebKit DB, then DuckDuckGo API) | |
| 1091 | 1091 | // Capture current generation to guard against stale callbacks | |
| 1092 | 1092 | gen := wv.Generation() | |
| 1093 | + | capturedURI := uri | |
| 1093 | 1094 | c.faviconAdapter.GetOrFetch(ctx, uri, func(texture *gdk.Texture) { | |
| 1094 | 1095 | // Skip nil results — a nil means "couldn't resolve", not "no favicon". | |
| 1095 | 1096 | // Without this guard, a late nil callback can overwrite a good favicon | |
| @@ -1105,6 +1106,10 @@ func (c *ContentCoordinator) resolveCommittedFavicon(ctx context.Context, paneID | |||
| 1105 | 1106 | if currentWV != wv { | |
| 1106 | 1107 | return | |
| 1107 | 1108 | } | |
| 1109 | + | // Verify the WebView hasn't navigated away since we started the fetch | |
| 1110 | + | if currentWV.URI() != capturedURI { | |
| 1111 | + | return | |
| 1112 | + | } | |
| 1108 | 1113 | c.updateStackedFaviconForPane(ctx, paneID, texture) | |
| 1109 | 1114 | }) | |
| 1110 | 1115 | } | |
| @@ -1090,6 +1090,7 @@ func (c *ContentCoordinator) resolveCommittedFavicon(ctx context.Context, paneID | |||||
| 1090 | // Fall back to GetOrFetch (checks service cache, WebKit DB, then DuckDuckGo API) | 1090 | // Fall back to GetOrFetch (checks service cache, WebKit DB, then DuckDuckGo API) | ||
| 1091 | // Capture current generation to guard against stale callbacks | 1091 | // Capture current generation to guard against stale callbacks | ||
| 1092 | gen := wv.Generation() | 1092 | gen := wv.Generation() | ||
| 1093 | + | capturedURI := uri | |||
| 1093 | c.faviconAdapter.GetOrFetch(ctx, uri, func(texture *gdk.Texture) { | 1094 | c.faviconAdapter.GetOrFetch(ctx, uri, func(texture *gdk.Texture) { | ||
| 1094 | // Skip nil results — a nil means "couldn't resolve", not "no favicon". | 1095 | // Skip nil results — a nil means "couldn't resolve", not "no favicon". | ||
| 1095 | // Without this guard, a late nil callback can overwrite a good favicon | 1096 | // Without this guard, a late nil callback can overwrite a good favicon | ||
| @@ -1105,6 +1106,10 @@ func (c *ContentCoordinator) resolveCommittedFavicon(ctx context.Context, paneID | |||||
| 1105 | if currentWV != wv { | 1106 | if currentWV != wv { | ||
| 1106 | return | 1107 | return | ||
| 1107 | } | 1108 | } | ||
| 1109 | + | // Verify the WebView hasn't navigated away since we started the fetch | |||
| 1110 | + | if currentWV.URI() != capturedURI { | |||
| 1111 | + | return | |||
| 1112 | + | } | |||
| 1108 | c.updateStackedFaviconForPane(ctx, paneID, texture) | 1113 | c.updateStackedFaviconForPane(ctx, paneID, texture) | ||
| 1109 | }) | 1114 | }) | ||
| 1110 | } | 1115 | } | ||
M internal/ui/input/modal.go +17 -4
| @@ -60,9 +60,10 @@ func (m Mode) DisplayName() string { | |||
| 60 | 60 | ||
| 61 | 61 | // ModalState manages the current input mode with optional timeout. | |
| 62 | 62 | type ModalState struct { | |
| 63 | - | mode Mode | |
| 64 | - | timeout time.Duration | |
| 65 | - | timer *time.Timer | |
| 63 | + | mode Mode | |
| 64 | + | timeout time.Duration | |
| 65 | + | timer *time.Timer | |
| 66 | + | timerGen int64 // incremented each time a timer is started; used to ignore stale callbacks | |
| 66 | 67 | ||
| 67 | 68 | // Callback for mode changes (called synchronously under lock). | |
| 68 | 69 | onModeChange func(from, to Mode) | |
| @@ -215,9 +216,21 @@ func (m *ModalState) cancelTimerLocked() { | |||
| 215 | 216 | // because onModeChange may make GTK calls (e.g., switching controller phase). | |
| 216 | 217 | // Must be called with m.mu held. | |
| 217 | 218 | func (m *ModalState) startTimeoutLocked(ctx context.Context, timeout time.Duration) { | |
| 219 | + | m.timerGen++ | |
| 220 | + | gen := m.timerGen | |
| 218 | 221 | schedule := m.scheduleOnMainThread | |
| 219 | 222 | m.timer = time.AfterFunc(timeout, func() { | |
| 220 | - | schedule(func() { m.ExitMode(ctx) }) | |
| 223 | + | schedule(func() { | |
| 224 | + | // Ignore stale callback: time.Timer.Stop can race with the | |
| 225 | + | // timer firing, so a canceled timer's callback may still run. | |
| 226 | + | m.mu.RLock() | |
| 227 | + | stale := m.timerGen != gen | |
| 228 | + | m.mu.RUnlock() | |
| 229 | + | if stale { | |
| 230 | + | return | |
| 231 | + | } | |
| 232 | + | m.ExitMode(ctx) | |
| 233 | + | }) | |
| 221 | 234 | }) | |
| 222 | 235 | } | |
| 223 | 236 | ||
| @@ -60,9 +60,10 @@ func (m Mode) DisplayName() string { | |||||
| 60 | 60 | ||||
| 61 | // ModalState manages the current input mode with optional timeout. | 61 | // ModalState manages the current input mode with optional timeout. | ||
| 62 | type ModalState struct { | 62 | type ModalState struct { | ||
| 63 | - | mode Mode | 63 | + | mode Mode |
| 64 | - | timeout time.Duration | 64 | + | timeout time.Duration |
| 65 | - | timer *time.Timer | 65 | + | timer *time.Timer |
| 66 | + | timerGen int64 // incremented each time a timer is started; used to ignore stale callbacks | |||
| 66 | 67 | ||||
| 67 | // Callback for mode changes (called synchronously under lock). | 68 | // Callback for mode changes (called synchronously under lock). | ||
| 68 | onModeChange func(from, to Mode) | 69 | onModeChange func(from, to Mode) | ||
| @@ -215,9 +216,21 @@ func (m *ModalState) cancelTimerLocked() { | |||||
| 215 | // because onModeChange may make GTK calls (e.g., switching controller phase). | 216 | // because onModeChange may make GTK calls (e.g., switching controller phase). | ||
| 216 | // Must be called with m.mu held. | 217 | // Must be called with m.mu held. | ||
| 217 | func (m *ModalState) startTimeoutLocked(ctx context.Context, timeout time.Duration) { | 218 | func (m *ModalState) startTimeoutLocked(ctx context.Context, timeout time.Duration) { | ||
| 219 | + | m.timerGen++ | |||
| 220 | + | gen := m.timerGen | |||
| 218 | schedule := m.scheduleOnMainThread | 221 | schedule := m.scheduleOnMainThread | ||
| 219 | m.timer = time.AfterFunc(timeout, func() { | 222 | m.timer = time.AfterFunc(timeout, func() { | ||
| 220 | - | schedule(func() { m.ExitMode(ctx) }) | 223 | + | schedule(func() { |
| 224 | + | // Ignore stale callback: time.Timer.Stop can race with the | |||
| 225 | + | // timer firing, so a canceled timer's callback may still run. | |||
| 226 | + | m.mu.RLock() | |||
| 227 | + | stale := m.timerGen != gen | |||
| 228 | + | m.mu.RUnlock() | |||
| 229 | + | if stale { | |||
| 230 | + | return | |||
| 231 | + | } | |||
| 232 | + | m.ExitMode(ctx) | |||
| 233 | + | }) | |||
| 221 | }) | 234 | }) | ||
| 222 | } | 235 | } | ||
| 223 | 236 | ||||