266e0b8 refactor(cef): tighten popup registration follow-ups · dumber

M internal/infrastructure/cef/webview_popup_proxy.go +10 -22


@@ -21,7 +21,13 @@ func (wv WebView) syntheticPopupState(proxyID string) syntheticPopupState {
21	21
22	22		wv.syntheticPopupMu.Lock()
23	23		defer wv.syntheticPopupMu.Unlock()
	24	+	return wv.syntheticPopupStateLocked(proxyID)
	25	+	}
24	26
	27	+	func (wv WebView) syntheticPopupStateLocked(proxyID string) syntheticPopupState {
	28	+	if wv == nil \|\| proxyID == "" {
	29	+	return nil
	30	+	}
25	31		if wv.syntheticPopups == nil {
26	32		wv.syntheticPopups = make(map[string]*syntheticPopupState)
27	33		}

@@ -33,16 +39,6 @@ func (wv WebView) syntheticPopupState(proxyID string) syntheticPopupState {
33	39		return state
34	40		}
35	41
36		-	func (wv WebView) lookupSyntheticPopupState(proxyID string) syntheticPopupState {
37		-	if wv == nil \|\| proxyID == "" {
38		-	return nil
39		-	}
40		-
41		-	wv.syntheticPopupMu.Lock()
42		-	defer wv.syntheticPopupMu.Unlock()
43		-	return wv.syntheticPopups[proxyID]
44		-	}
45		-
46	42		func (wv *WebView) deleteSyntheticPopupState(proxyID string) {
47	43		if wv == nil \|\| proxyID == "" {
48	44		return

@@ -58,12 +54,11 @@ func (wv *WebView) handleSyntheticPopupOpen(targetURL, frameName, proxyID string
58	54		return
59	55		}
60	56
61		-	state := wv.lookupSyntheticPopupState(proxyID)
62		-	if state != nil {
63		-	wv.syntheticPopupMu.Lock()
	57	+	wv.syntheticPopupMu.Lock()
	58	+	if state := wv.syntheticPopups[proxyID]; state != nil {
64	59		state.NoJavaScriptAccess = noJavaScriptAccess
65		-	wv.syntheticPopupMu.Unlock()
66	60		}
	61	+	wv.syntheticPopupMu.Unlock()
67	62
68	63		wv.runOnGTK(func() {
69	64		if wv.destroyed.Load() {

@@ -92,14 +87,7 @@ func (wv *WebView) handleSyntheticPopupOpen(targetURL, frameName, proxyID string
92	87
93	88		pendingURI := ""
94	89		wv.syntheticPopupMu.Lock()
95		-	if wv.syntheticPopups == nil {
96		-	wv.syntheticPopups = make(map[string]*syntheticPopupState)
97		-	}
98		-	state := wv.syntheticPopups[proxyID]
99		-	if state == nil {
100		-	state = &syntheticPopupState{}
101		-	wv.syntheticPopups[proxyID] = state
102		-	}
	90	+	state := wv.syntheticPopupStateLocked(proxyID)
103	91		state.WebView = popupWV
104	92		state.NoJavaScriptAccess = noJavaScriptAccess
105	93		pendingURI = strings.TrimSpace(state.PendingURI)

 	wv.syntheticPopupMu.Lock()
 	defer wv.syntheticPopupMu.Unlock()
+	return wv.syntheticPopupStateLocked(proxyID)
+}
+func (wv *WebView) syntheticPopupStateLocked(proxyID string) *syntheticPopupState {
+	if wv == nil || proxyID == "" {
+		return nil
+	}
 	if wv.syntheticPopups == nil {
 		wv.syntheticPopups = make(map[string]*syntheticPopupState)
 	}
 	return state
 }
-func (wv *WebView) lookupSyntheticPopupState(proxyID string) *syntheticPopupState {
-	if wv == nil || proxyID == "" {
-		return nil
-	}
-	wv.syntheticPopupMu.Lock()
-	defer wv.syntheticPopupMu.Unlock()
-	return wv.syntheticPopups[proxyID]
-}
 func (wv *WebView) deleteSyntheticPopupState(proxyID string) {
 	if wv == nil || proxyID == "" {
 		return
 		return
 	}
-	state := wv.lookupSyntheticPopupState(proxyID)
+	wv.syntheticPopupMu.Lock()
-	if state != nil {
+	if state := wv.syntheticPopups[proxyID]; state != nil {
-		wv.syntheticPopupMu.Lock()
 		state.NoJavaScriptAccess = noJavaScriptAccess
-		wv.syntheticPopupMu.Unlock()
 	}
+	wv.syntheticPopupMu.Unlock()
 	wv.runOnGTK(func() {
 		if wv.destroyed.Load() {
 		pendingURI := ""
 		wv.syntheticPopupMu.Lock()
-		if wv.syntheticPopups == nil {
+		state := wv.syntheticPopupStateLocked(proxyID)
-			wv.syntheticPopups = make(map[string]*syntheticPopupState)
-		}
-		state := wv.syntheticPopups[proxyID]
-		if state == nil {
-			state = &syntheticPopupState{}
-			wv.syntheticPopups[proxyID] = state
-		}
 		state.WebView = popupWV
 		state.NoJavaScriptAccess = noJavaScriptAccess
 		pendingURI = strings.TrimSpace(state.PendingURI)

M internal/ui/coordinator/content/popup.go +15 -8


@@ -526,10 +526,23 @@ func (c *Coordinator) finishPopupCreate(
526	526		Str("uri", logging.TruncateURL(req.TargetURI, logURLMaxLen)).
527	527		Msg("popup OAuth check")
528	528
	529	+	// Initialize callbacks before publishing the WebView so any reader that sees
	530	+	// the popup during workspace insertion gets a fully configured instance.
	531	+	c.setupWebViewCallbacks(ctx, paneID, popupWV)
	532	+
529	533		// Register the popup WebView before workspace insertion so split/stack UI
530	534		// updates can reuse the real popup instead of acquiring a placeholder pane
531	535		// WebView that races the actual popup flow.
532		-	c.setWebViewLocked(paneID, popupWV)
	536	+	c.RegisterPopupWebView(paneID, popupWV)
	537	+	inserted := false
	538	+	defer func() {
	539	+	if inserted {
	540	+	return
	541	+	}
	542	+	if current := c.getWebViewLocked(paneID); current == popupWV {
	543	+	c.deleteWebViewLocked(paneID)
	544	+	}
	545	+	}()
533	546
534	547		// Insert into workspace IMMEDIATELY (WebView stays hidden)
535	548		// This is required for WebKit to establish window.opener relationship

@@ -546,22 +559,16 @@ func (c *Coordinator) finishPopupCreate(
546	559
547	560		if err := c.onInsertPopup(ctx, popupInput); err != nil {
548	561		log.Error().Err(err).Msg("failed to insert popup into workspace")
549		-	if current := c.getWebViewLocked(paneID); current == popupWV {
550		-	c.deleteWebViewLocked(paneID)
551		-	}
552	562		popupWV.Destroy()
553	563		return nil
554	564		}
555	565		}
	566	+	inserted = true
556	567
557		-	// WebView already registered before insertion.
558	568		if !req.NoJavaScriptAccess {
559	569		c.storeReusableNamedPopup(parentPaneID, req.FrameName, popupWV)
560	570		}
561	571
562		-	// Setup standard callbacks (after successful insertion to avoid leak)
563		-	c.setupWebViewCallbacks(ctx, paneID, popupWV)
564		-
565	572		// Engines without native popup lifecycle hooks can still support
566	573		// programmatic popup closure (proxy.close(), OAuth auto-close) by exposing
567	574		// the optional OAuthCallbackCapable interface.

 		Str("uri", logging.TruncateURL(req.TargetURI, logURLMaxLen)).
 		Msg("popup OAuth check")
+	// Initialize callbacks before publishing the WebView so any reader that sees
+	// the popup during workspace insertion gets a fully configured instance.
+	c.setupWebViewCallbacks(ctx, paneID, popupWV)
 	// Register the popup WebView before workspace insertion so split/stack UI
 	// updates can reuse the real popup instead of acquiring a placeholder pane
 	// WebView that races the actual popup flow.
-	c.setWebViewLocked(paneID, popupWV)
+	c.RegisterPopupWebView(paneID, popupWV)
+	inserted := false
+	defer func() {
+		if inserted {
+			return
+		}
+		if current := c.getWebViewLocked(paneID); current == popupWV {
+			c.deleteWebViewLocked(paneID)
+		}
+	}()
 	// Insert into workspace IMMEDIATELY (WebView stays hidden)
 	// This is required for WebKit to establish window.opener relationship
 		if err := c.onInsertPopup(ctx, popupInput); err != nil {
 			log.Error().Err(err).Msg("failed to insert popup into workspace")
-			if current := c.getWebViewLocked(paneID); current == popupWV {
-				c.deleteWebViewLocked(paneID)
-			}
 			popupWV.Destroy()
 			return nil
 		}
 	}
+	inserted = true
-	// WebView already registered before insertion.
 	if !req.NoJavaScriptAccess {
 		c.storeReusableNamedPopup(parentPaneID, req.FrameName, popupWV)
 	}
-	// Setup standard callbacks (after successful insertion to avoid leak)
-	c.setupWebViewCallbacks(ctx, paneID, popupWV)
 	// Engines without native popup lifecycle hooks can still support
 	// programmatic popup closure (proxy.close(), OAuth auto-close) by exposing
 	// the optional OAuthCallbackCapable interface.

M internal/ui/coordinator/content/popup_test.go +19 -23


@@ -143,19 +143,16 @@ func TestNormalizePopupTargetURIForFallback_PreservesUnknownURLs(t *testing.T) {
143	143		assert.Equal(t, raw, normalizePopupTargetURIForFallback(raw))
144	144		}
145	145
146		-	func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
	146	+	func newPopupCreateCoordinatorForTest(t testing.T, popupID port.WebViewID) (context.Context, entity.PaneID, mocks.MockWebView, mocks.MockWebView, Coordinator) {
	147	+	t.Helper()
	148	+
147	149		ctx := context.Background()
148	150		parentPaneID := entity.PaneID("parent-pane")
149	151		parentWV := mocks.NewMockWebView(t)
150	152		parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
151	153
152	154		popupWV := mocks.NewMockWebView(t)
153		-	popupWV.EXPECT().ID().Return(port.WebViewID(150)).Once()
154		-	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
155		-	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
156		-	popupWV.EXPECT().IsLoading().Return(false).Once()
157		-	popupWV.EXPECT().URI().Return("").Once()
158		-	popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
	155	+	popupWV.EXPECT().ID().Return(popupID).Once()
159	156
160	157		factory := mocks.NewMockWebViewFactory(t)
161	158		factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()

@@ -166,6 +163,18 @@ func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *test
166	163		popupOAuth: make(map[port.WebViewID]*popupOAuthState),
167	164		}
168	165		c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
	166	+
	167	+	return ctx, parentPaneID, parentWV, popupWV, c
	168	+	}
	169	+
	170	+	func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
	171	+	ctx, parentPaneID, parentWV, popupWV, c := newPopupCreateCoordinatorForTest(t, port.WebViewID(150))
	172	+	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
	173	+	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
	174	+	popupWV.EXPECT().IsLoading().Return(false).Once()
	175	+	popupWV.EXPECT().URI().Return("").Once()
	176	+	popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
	177	+
169	178		c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
170	179		require.Equal(t, entity.PaneID("popup-pane"), input.PopupPane.ID)
171	180		require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))

@@ -183,24 +192,11 @@ func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *test
183	192		}
184	193
185	194		func TestHandlePopupCreate_CleansUpPreRegisteredPopupWebViewWhenInsertionFails(t *testing.T) {
186		-	ctx := context.Background()
187		-	parentPaneID := entity.PaneID("parent-pane")
188		-	parentWV := mocks.NewMockWebView(t)
189		-	parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
190		-
191		-	popupWV := mocks.NewMockWebView(t)
192		-	popupWV.EXPECT().ID().Return(port.WebViewID(151)).Once()
	195	+	ctx, parentPaneID, parentWV, popupWV, c := newPopupCreateCoordinatorForTest(t, port.WebViewID(151))
	196	+	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
	197	+	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
193	198		popupWV.EXPECT().Destroy().Once()
194	199
195		-	factory := mocks.NewMockWebViewFactory(t)
196		-	factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
197		-
198		-	c := &Coordinator{
199		-	webViews: make(map[entity.PaneID]port.WebView),
200		-	pendingPopups: make(map[port.WebViewID]*PendingPopup),
201		-	popupOAuth: make(map[port.WebViewID]*popupOAuthState),
202		-	}
203		-	c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
204	200		c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
205	201		require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
206	202		return assert.AnError

 	assert.Equal(t, raw, normalizePopupTargetURIForFallback(raw))
 }
-func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
+func newPopupCreateCoordinatorForTest(t *testing.T, popupID port.WebViewID) (context.Context, entity.PaneID, *mocks.MockWebView, *mocks.MockWebView, *Coordinator) {
+	t.Helper()
 	ctx := context.Background()
 	parentPaneID := entity.PaneID("parent-pane")
 	parentWV := mocks.NewMockWebView(t)
 	parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
 	popupWV := mocks.NewMockWebView(t)
-	popupWV.EXPECT().ID().Return(port.WebViewID(150)).Once()
+	popupWV.EXPECT().ID().Return(popupID).Once()
-	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
-	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
-	popupWV.EXPECT().IsLoading().Return(false).Once()
-	popupWV.EXPECT().URI().Return("").Once()
-	popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
 	factory := mocks.NewMockWebViewFactory(t)
 	factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
 		popupOAuth:    make(map[port.WebViewID]*popupOAuthState),
 	}
 	c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
+	return ctx, parentPaneID, parentWV, popupWV, c
+}
+func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
+	ctx, parentPaneID, parentWV, popupWV, c := newPopupCreateCoordinatorForTest(t, port.WebViewID(150))
+	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
+	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
+	popupWV.EXPECT().IsLoading().Return(false).Once()
+	popupWV.EXPECT().URI().Return("").Once()
+	popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
 	c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
 		require.Equal(t, entity.PaneID("popup-pane"), input.PopupPane.ID)
 		require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
 }
 func TestHandlePopupCreate_CleansUpPreRegisteredPopupWebViewWhenInsertionFails(t *testing.T) {
-	ctx := context.Background()
+	ctx, parentPaneID, parentWV, popupWV, c := newPopupCreateCoordinatorForTest(t, port.WebViewID(151))
-	parentPaneID := entity.PaneID("parent-pane")
+	popupWV.EXPECT().Generation().Return(uint64(1)).Once()
-	parentWV := mocks.NewMockWebView(t)
+	popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
-	parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
-	popupWV := mocks.NewMockWebView(t)
-	popupWV.EXPECT().ID().Return(port.WebViewID(151)).Once()
 	popupWV.EXPECT().Destroy().Once()
-	factory := mocks.NewMockWebViewFactory(t)
-	factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
-	c := &Coordinator{
-		webViews:      make(map[entity.PaneID]port.WebView),
-		pendingPopups: make(map[port.WebViewID]*PendingPopup),
-		popupOAuth:    make(map[port.WebViewID]*popupOAuthState),
-	}
-	c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
 	c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
 		require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
 		return assert.AnError