From 61b501984f7ec9b604d9bb023b8c2b6762e4b105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Mon, 7 Sep 2026 12:19:14 +0200 Subject: [PATCH] [client] Serialize window operations while a slot is being created Callers arriving after the window is published but before the creator has drained the queue took the existing-window fast path and could run ahead of older queued operations, so a newer SetURL could be overwritten by an older one. withWindow now queues every caller while the creating flag is set and clears the flag only once the queue is seen empty under the lock. A factory panic or a nil window left the creating flag set and the slot dead; creation and drain now reset that state on early exit. hideOtherWindows records the windows it hid only when no restore ran in between, tracked by a generation counter, and re-shows them otherwise, so a restore racing the hide cannot strand hidden windows. --- client/ui/services/windowmanager.go | 92 +++++++++++++++++++----- client/ui/services/windowmanager_test.go | 70 ++++++++++++++++++ 2 files changed, 145 insertions(+), 17 deletions(-) diff --git a/client/ui/services/windowmanager.go b/client/ui/services/windowmanager.go index 68542ae53..d93b25821 100644 --- a/client/ui/services/windowmanager.go +++ b/client/ui/services/windowmanager.go @@ -131,6 +131,7 @@ type WindowManager struct { newMain func(startURL string) *application.WebviewWindow creating map[string]bool pendingOps map[string][]windowOp + restoreGen uint64 ready map[uint]bool showPending map[uint]bool pendingTab map[uint]string @@ -567,6 +568,11 @@ func (s *WindowManager) ensureMain(startURL string, op windowOp) { func (s *WindowManager) withWindow(name string, slot **application.WebviewWindow, factory func() *application.WebviewWindow, op windowOp) { s.mu.Lock() + if s.creating[name] { + s.pendingOps[name] = append(s.pendingOps[name], op) + s.mu.Unlock() + return + } if w := *slot; w != nil { s.mu.Unlock() op(w, false) @@ -576,29 +582,66 @@ func (s *WindowManager) withWindow(name string, slot **application.WebviewWindow s.mu.Unlock() return } - if s.creating[name] { - s.pendingOps[name] = append(s.pendingOps[name], op) - s.mu.Unlock() - return - } s.creating[name] = true s.mu.Unlock() - w := factory() - - s.mu.Lock() - *slot = w - delete(s.creating, name) - queued := s.pendingOps[name] - delete(s.pendingOps, name) - s.mu.Unlock() - + w := s.createWindow(name, slot, factory) if w == nil { return } + s.finishCreation(name, w, op) +} + +func (s *WindowManager) createWindow(name string, slot **application.WebviewWindow, factory func() *application.WebviewWindow) *application.WebviewWindow { + created := false + defer func() { + if created { + return + } + s.mu.Lock() + delete(s.creating, name) + delete(s.pendingOps, name) + s.mu.Unlock() + }() + + w := factory() + if w == nil { + return nil + } + s.mu.Lock() + *slot = w + s.mu.Unlock() + created = true + return w +} + +func (s *WindowManager) finishCreation(name string, w *application.WebviewWindow, op windowOp) { + finished := false + defer func() { + if finished { + return + } + s.mu.Lock() + delete(s.creating, name) + delete(s.pendingOps, name) + s.mu.Unlock() + }() + op(w, true) - for _, queuedOp := range queued { - queuedOp(w, false) + for { + s.mu.Lock() + queued := s.pendingOps[name] + delete(s.pendingOps, name) + if len(queued) == 0 { + delete(s.creating, name) + finished = true + s.mu.Unlock() + return + } + s.mu.Unlock() + for _, queuedOp := range queued { + queuedOp(w, false) + } } } @@ -927,6 +970,10 @@ func (s *WindowManager) retitleAll() { } func (s *WindowManager) hideOtherWindows(keepName string) { + s.mu.Lock() + gen := s.restoreGen + s.mu.Unlock() + var hidden []application.Window for _, w := range s.app.Window.GetAll() { if w == nil || w.Name() == keepName || !w.IsVisible() { @@ -938,9 +985,19 @@ func (s *WindowManager) hideOtherWindows(keepName string) { if len(hidden) == 0 { return } + s.mu.Lock() - s.hiddenForLogin = append(s.hiddenForLogin, hidden...) + restored := s.restoreGen != gen + if !restored { + s.hiddenForLogin = append(s.hiddenForLogin, hidden...) + } s.mu.Unlock() + if !restored { + return + } + for _, w := range hidden { + w.Show() + } } // restoreHiddenWindows re-shows windows hidden by hideOtherWindows. If the main @@ -951,6 +1008,7 @@ func (s *WindowManager) restoreHiddenWindows() { s.mu.Lock() hidden := s.hiddenForLogin s.hiddenForLogin = nil + s.restoreGen++ mainWindow := s.mainWindow s.mu.Unlock() diff --git a/client/ui/services/windowmanager_test.go b/client/ui/services/windowmanager_test.go index c98d2db2a..9de9f13b8 100644 --- a/client/ui/services/windowmanager_test.go +++ b/client/ui/services/windowmanager_test.go @@ -125,3 +125,73 @@ func TestWithWindowConcurrentCallersShareOneCreation(t *testing.T) { require.Equal(t, int32(2), opCalls.Load()) require.NotNil(t, slot) } + +func TestWithWindowOpsQueuedDuringCreationRunInArrivalOrder(t *testing.T) { + s := newTestWindowManager() + var slot *application.WebviewWindow + var order []string + record := func(label string) windowOp { + return func(_ *application.WebviewWindow, created bool) { + order = append(order, fmt.Sprintf("%s:%v", label, created)) + } + } + var factory func() *application.WebviewWindow + factory = func() *application.WebviewWindow { + s.withWindow(windowMain, &slot, factory, func(w *application.WebviewWindow, created bool) { + record("a")(w, created) + // Arrives while the creator is still draining the queue: it must not + // jump ahead of "b" through the existing-window fast path. + s.withWindow(windowMain, &slot, factory, record("c")) + }) + s.withWindow(windowMain, &slot, factory, record("b")) + return &application.WebviewWindow{} + } + + done := make(chan struct{}) + go func() { + defer close(done) + s.withWindow(windowMain, &slot, factory, record("outer")) + }() + waitDone(t, done, "withWindow deadlocked while draining queued operations") + + require.Equal(t, []string{"outer:true", "a:false", "b:false", "c:false"}, order) + require.Empty(t, s.creating) + require.Empty(t, s.pendingOps) +} + +func TestWithWindowFactoryPanicReleasesCreation(t *testing.T) { + s := newTestWindowManager() + var slot *application.WebviewWindow + func() { + defer func() { require.NotNil(t, recover()) }() + s.withWindow(windowMain, &slot, func() *application.WebviewWindow { + panic("factory failed") + }, func(*application.WebviewWindow, bool) {}) + }() + require.Empty(t, s.creating) + require.Empty(t, s.pendingOps) + require.Nil(t, slot) + + created := false + s.withWindow(windowMain, &slot, func() *application.WebviewWindow { + return &application.WebviewWindow{} + }, func(_ *application.WebviewWindow, c bool) { + created = c + }) + require.True(t, created) + require.NotNil(t, slot) +} + +func TestWithWindowNilFromFactoryReleasesCreation(t *testing.T) { + s := newTestWindowManager() + var slot *application.WebviewWindow + opCalls := 0 + s.withWindow(windowMain, &slot, func() *application.WebviewWindow { + return nil + }, func(*application.WebviewWindow, bool) { + opCalls++ + }) + require.Equal(t, 0, opCalls) + require.Empty(t, s.creating) + require.Nil(t, slot) +}