fix(popup): harden deferred and CEF failure handling
- commit
- 47627e6d5aa7f8edb39f55294f7b9728cfe96c88
- author
- brice · · 2026-07-27 10:08 UTC
- parent
- 697b5f3
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
6 files · +156 -10
M internal/application/dto/browsing_context.go +2 -0
| @@ -20,6 +20,8 @@ const ( | |||
| 20 | 20 | ) | |
| 21 | 21 | ||
| 22 | 22 | // PopupFeatureState describes whether popup metadata is available and explicit. | |
| 23 | + | // Host policy treats its zero value as featureless, not PopupFeaturesUnknown; | |
| 24 | + | // popup producers must set State explicitly when resolution must be deferred. | |
| 23 | 25 | type PopupFeatureState string | |
| 24 | 26 | ||
| 25 | 27 | const ( | |
| @@ -20,6 +20,8 @@ const ( | |||||
| 20 | ) | 20 | ) | ||
| 21 | 21 | ||||
| 22 | // PopupFeatureState describes whether popup metadata is available and explicit. | 22 | // PopupFeatureState describes whether popup metadata is available and explicit. | ||
| 23 | + | // Host policy treats its zero value as featureless, not PopupFeaturesUnknown; | |||
| 24 | + | // popup producers must set State explicitly when resolution must be deferred. | |||
| 23 | type PopupFeatureState string | 25 | type PopupFeatureState string | ||
| 24 | 26 | ||||
| 25 | const ( | 27 | const ( | ||
M internal/infrastructure/cef/webview_handlers.go +12 -4
| @@ -728,7 +728,8 @@ func (h *handlerSet) OnBeforePopup( | |||
| 728 | 728 | // browser and retain the synthetic opener bridge when allowed. | |
| 729 | 729 | cefPopup.preparePopupShellDirectBrowserCreation() | |
| 730 | 730 | } | |
| 731 | - | if !cefPopup.abortNativePopupHost() { | |
| 731 | + | abortInvoked := cefPopup.abortNativePopupHost() | |
| 732 | + | if !abortInvoked { | |
| 732 | 733 | cefPopup.Destroy() | |
| 733 | 734 | } | |
| 734 | 735 | logging.FromContext(h.currentContext()).Warn(). | |
| @@ -740,7 +741,8 @@ func (h *handlerSet) OnBeforePopup( | |||
| 740 | 741 | Int32("popup_id", popupID). | |
| 741 | 742 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | |
| 742 | 743 | Bool("fallback_eligible", fallbackEligible). | |
| 743 | - | Msg("cef: native popup arming failed and host abort was invoked") | |
| 744 | + | Bool("host_abort_invoked", abortInvoked). | |
| 745 | + | Msg("cef: native popup arming failed") | |
| 744 | 746 | return true | |
| 745 | 747 | case dto.HostDecisionCreateBrowserWindow, | |
| 746 | 748 | dto.HostDecisionCreatePane, | |
| @@ -748,12 +750,18 @@ func (h *handlerSet) OnBeforePopup( | |||
| 748 | 750 | // CEF's native popup is blocked. A related shell adopted by a Dumber | |
| 749 | 751 | // host must create its browser directly; noopener controls whether the | |
| 750 | 752 | // synthetic opener/postMessage bridge is installed. | |
| 751 | - | cefPopup.preparePopupShellDirectBrowserCreation() | |
| 753 | + | directCreationPrepared := cefPopup.preparePopupShellDirectBrowserCreation() | |
| 754 | + | if !directCreationPrepared && cefPopup.isNativePopupCandidate() { | |
| 755 | + | // Clear only a still-unprepared candidate; an already-started fallback | |
| 756 | + | // owns its opener state and must remain intact. | |
| 757 | + | cefPopup.discardNativePopupCandidate() | |
| 758 | + | } | |
| 752 | 759 | logging.FromContext(h.currentContext()).Debug(). | |
| 753 | 760 | Int32("popup_id", popupID). | |
| 754 | 761 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | |
| 755 | 762 | Str("decision", string(decision.Kind)). | |
| 756 | - | Msg("cef: blocking native popup and using related shell browser creation") | |
| 763 | + | Bool("direct_creation_prepared", directCreationPrepared). | |
| 764 | + | Msg("cef: blocking native popup for related shell browser creation") | |
| 757 | 765 | return true | |
| 758 | 766 | case dto.HostDecisionAwaitPopupFeatures, dto.HostDecisionDeny: | |
| 759 | 767 | cefPopup.Destroy() | |
| @@ -728,7 +728,8 @@ func (h *handlerSet) OnBeforePopup( | |||||
| 728 | // browser and retain the synthetic opener bridge when allowed. | 728 | // browser and retain the synthetic opener bridge when allowed. | ||
| 729 | cefPopup.preparePopupShellDirectBrowserCreation() | 729 | cefPopup.preparePopupShellDirectBrowserCreation() | ||
| 730 | } | 730 | } | ||
| 731 | - | if !cefPopup.abortNativePopupHost() { | 731 | + | abortInvoked := cefPopup.abortNativePopupHost() |
| 732 | + | if !abortInvoked { | |||
| 732 | cefPopup.Destroy() | 733 | cefPopup.Destroy() | ||
| 733 | } | 734 | } | ||
| 734 | logging.FromContext(h.currentContext()).Warn(). | 735 | logging.FromContext(h.currentContext()).Warn(). | ||
| @@ -740,7 +741,8 @@ func (h *handlerSet) OnBeforePopup( | |||||
| 740 | Int32("popup_id", popupID). | 741 | Int32("popup_id", popupID). | ||
| 741 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | 742 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | ||
| 742 | Bool("fallback_eligible", fallbackEligible). | 743 | Bool("fallback_eligible", fallbackEligible). | ||
| 743 | - | Msg("cef: native popup arming failed and host abort was invoked") | 744 | + | Bool("host_abort_invoked", abortInvoked). |
| 745 | + | Msg("cef: native popup arming failed") | |||
| 744 | return true | 746 | return true | ||
| 745 | case dto.HostDecisionCreateBrowserWindow, | 747 | case dto.HostDecisionCreateBrowserWindow, | ||
| 746 | dto.HostDecisionCreatePane, | 748 | dto.HostDecisionCreatePane, | ||
| @@ -748,12 +750,18 @@ func (h *handlerSet) OnBeforePopup( | |||||
| 748 | // CEF's native popup is blocked. A related shell adopted by a Dumber | 750 | // CEF's native popup is blocked. A related shell adopted by a Dumber | ||
| 749 | // host must create its browser directly; noopener controls whether the | 751 | // host must create its browser directly; noopener controls whether the | ||
| 750 | // synthetic opener/postMessage bridge is installed. | 752 | // synthetic opener/postMessage bridge is installed. | ||
| 751 | - | cefPopup.preparePopupShellDirectBrowserCreation() | 753 | + | directCreationPrepared := cefPopup.preparePopupShellDirectBrowserCreation() |
| 754 | + | if !directCreationPrepared && cefPopup.isNativePopupCandidate() { | |||
| 755 | + | // Clear only a still-unprepared candidate; an already-started fallback | |||
| 756 | + | // owns its opener state and must remain intact. | |||
| 757 | + | cefPopup.discardNativePopupCandidate() | |||
| 758 | + | } | |||
| 752 | logging.FromContext(h.currentContext()).Debug(). | 759 | logging.FromContext(h.currentContext()).Debug(). | ||
| 753 | Int32("popup_id", popupID). | 760 | Int32("popup_id", popupID). | ||
| 754 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | 761 | Str("target_url", logging.TruncateURL(targetURL, logging.PermissionLogURLMaxLen)). | ||
| 755 | Str("decision", string(decision.Kind)). | 762 | Str("decision", string(decision.Kind)). | ||
| 756 | - | Msg("cef: blocking native popup and using related shell browser creation") | 763 | + | Bool("direct_creation_prepared", directCreationPrepared). |
| 764 | + | Msg("cef: blocking native popup for related shell browser creation") | |||
| 757 | return true | 765 | return true | ||
| 758 | case dto.HostDecisionAwaitPopupFeatures, dto.HostDecisionDeny: | 766 | case dto.HostDecisionAwaitPopupFeatures, dto.HostDecisionDeny: | ||
| 759 | cefPopup.Destroy() | 767 | cefPopup.Destroy() | ||
M internal/infrastructure/cef/webview_popup_routing_test.go +64 -0
| @@ -212,6 +212,33 @@ func TestOnBeforePopup_OpenerRequiredNativeArmFailureDeniesWithoutFallback(t *te | |||
| 212 | 212 | assert.Equal(t, "create-native-popup", record["decision"]) | |
| 213 | 213 | assert.Equal(t, "new-popup", record["target_disposition"]) | |
| 214 | 214 | assert.Equal(t, "native-arm-failed", record["reason_code"]) | |
| 215 | + | assert.Equal(t, true, record["host_abort_invoked"]) | |
| 216 | + | assert.Equal(t, "cef: native popup arming failed", record["message"]) | |
| 217 | + | } | |
| 218 | + | ||
| 219 | + | func TestOnBeforePopup_NativeArmFailureWithoutHostAbortLogsDirectCleanup(t *testing.T) { | |
| 220 | + | var output bytes.Buffer | |
| 221 | + | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | |
| 222 | + | ctx := logging.WithContext(context.Background(), logger) | |
| 223 | + | parentWV := &WebView{ctx: ctx, id: 20} | |
| 224 | + | popupWV := &WebView{ctx: context.Background(), id: 28, pendingCreate: &pendingBrowserCreate{}} | |
| 225 | + | popupWV.markNativePopupCandidate(parentWV) | |
| 226 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{ | |
| 227 | + | Kind: dto.HostDecisionCreateNativePopup, RequiresNativeOpener: true, | |
| 228 | + | }) | |
| 229 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |
| 230 | + | ||
| 231 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |
| 232 | + | nil, nil, 94, "https://example.com/opener-required", "required", | |
| 233 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |
| 234 | + | ) | |
| 235 | + | ||
| 236 | + | require.True(t, blocked) | |
| 237 | + | require.True(t, popupWV.IsDestroyed()) | |
| 238 | + | var record map[string]any | |
| 239 | + | require.NoError(t, json.Unmarshal(output.Bytes(), &record)) | |
| 240 | + | assert.Equal(t, false, record["host_abort_invoked"]) | |
| 241 | + | assert.Equal(t, "cef: native popup arming failed", record["message"]) | |
| 215 | 242 | } | |
| 216 | 243 | ||
| 217 | 244 | func TestOnBeforePopup_AuthNativeArmFailureDeniesWithoutFallback(t *testing.T) { | |
| @@ -274,6 +301,43 @@ func TestOnBeforePopup_ReuseNamedPaneBlocksNativePopup(t *testing.T) { | |||
| 274 | 301 | require.True(t, popupWV.nativePopupFallbackStarted) | |
| 275 | 302 | } | |
| 276 | 303 | ||
| 304 | + | func TestOnBeforePopup_HostedDecisionDiscardsUnpreparedNativeCandidate(t *testing.T) { | |
| 305 | + | parentWV := &WebView{ctx: context.Background(), id: 24} | |
| 306 | + | popupWV := &WebView{ctx: context.Background(), id: 32} | |
| 307 | + | popupWV.markNativePopupCandidate(parentWV) | |
| 308 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{Kind: dto.HostDecisionCreateBrowserWindow}) | |
| 309 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |
| 310 | + | ||
| 311 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |
| 312 | + | nil, nil, 98, "https://example.com/window", "window", | |
| 313 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |
| 314 | + | ) | |
| 315 | + | ||
| 316 | + | require.True(t, blocked) | |
| 317 | + | require.False(t, popupWV.isNativePopupCandidate()) | |
| 318 | + | require.False(t, popupWV.nativePopupFallbackStarted) | |
| 319 | + | } | |
| 320 | + | ||
| 321 | + | func TestOnBeforePopup_HostedDecisionPreservesAlreadyPreparedFallbackOpener(t *testing.T) { | |
| 322 | + | parentWV := &WebView{ctx: context.Background(), id: 25} | |
| 323 | + | parentWV.updateURI("https://example.com/opener") | |
| 324 | + | popupWV := &WebView{ctx: context.Background(), id: 33, pendingCreate: &pendingBrowserCreate{}} | |
| 325 | + | popupWV.markNativePopupCandidate(parentWV) | |
| 326 | + | require.True(t, popupWV.preparePopupShellDirectBrowserCreation()) | |
| 327 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{Kind: dto.HostDecisionCreateBrowserWindow}) | |
| 328 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |
| 329 | + | ||
| 330 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |
| 331 | + | nil, nil, 99, "https://example.com/window", "window", | |
| 332 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |
| 333 | + | ) | |
| 334 | + | ||
| 335 | + | require.True(t, blocked) | |
| 336 | + | require.True(t, popupWV.nativePopupFallbackStarted) | |
| 337 | + | require.Same(t, parentWV, popupWV.popupOpenerBridgeParent) | |
| 338 | + | require.Equal(t, "https://example.com/opener", popupWV.popupOpenerBridgeParentURI) | |
| 339 | + | } | |
| 340 | + | ||
| 277 | 341 | func TestOnBeforePopup_DenyCleansUp(t *testing.T) { | |
| 278 | 342 | parentWV := &WebView{ctx: context.Background(), id: 24} | |
| 279 | 343 | popupWV := &WebView{ctx: context.Background(), id: 32} | |
| @@ -212,6 +212,33 @@ func TestOnBeforePopup_OpenerRequiredNativeArmFailureDeniesWithoutFallback(t *te | |||||
| 212 | assert.Equal(t, "create-native-popup", record["decision"]) | 212 | assert.Equal(t, "create-native-popup", record["decision"]) | ||
| 213 | assert.Equal(t, "new-popup", record["target_disposition"]) | 213 | assert.Equal(t, "new-popup", record["target_disposition"]) | ||
| 214 | assert.Equal(t, "native-arm-failed", record["reason_code"]) | 214 | assert.Equal(t, "native-arm-failed", record["reason_code"]) | ||
| 215 | + | assert.Equal(t, true, record["host_abort_invoked"]) | |||
| 216 | + | assert.Equal(t, "cef: native popup arming failed", record["message"]) | |||
| 217 | + | } | |||
| 218 | + | ||||
| 219 | + | func TestOnBeforePopup_NativeArmFailureWithoutHostAbortLogsDirectCleanup(t *testing.T) { | |||
| 220 | + | var output bytes.Buffer | |||
| 221 | + | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | |||
| 222 | + | ctx := logging.WithContext(context.Background(), logger) | |||
| 223 | + | parentWV := &WebView{ctx: ctx, id: 20} | |||
| 224 | + | popupWV := &WebView{ctx: context.Background(), id: 28, pendingCreate: &pendingBrowserCreate{}} | |||
| 225 | + | popupWV.markNativePopupCandidate(parentWV) | |||
| 226 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{ | |||
| 227 | + | Kind: dto.HostDecisionCreateNativePopup, RequiresNativeOpener: true, | |||
| 228 | + | }) | |||
| 229 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |||
| 230 | + | ||||
| 231 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |||
| 232 | + | nil, nil, 94, "https://example.com/opener-required", "required", | |||
| 233 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |||
| 234 | + | ) | |||
| 235 | + | ||||
| 236 | + | require.True(t, blocked) | |||
| 237 | + | require.True(t, popupWV.IsDestroyed()) | |||
| 238 | + | var record map[string]any | |||
| 239 | + | require.NoError(t, json.Unmarshal(output.Bytes(), &record)) | |||
| 240 | + | assert.Equal(t, false, record["host_abort_invoked"]) | |||
| 241 | + | assert.Equal(t, "cef: native popup arming failed", record["message"]) | |||
| 215 | } | 242 | } | ||
| 216 | 243 | ||||
| 217 | func TestOnBeforePopup_AuthNativeArmFailureDeniesWithoutFallback(t *testing.T) { | 244 | func TestOnBeforePopup_AuthNativeArmFailureDeniesWithoutFallback(t *testing.T) { | ||
| @@ -274,6 +301,43 @@ func TestOnBeforePopup_ReuseNamedPaneBlocksNativePopup(t *testing.T) { | |||||
| 274 | require.True(t, popupWV.nativePopupFallbackStarted) | 301 | require.True(t, popupWV.nativePopupFallbackStarted) | ||
| 275 | } | 302 | } | ||
| 276 | 303 | ||||
| 304 | + | func TestOnBeforePopup_HostedDecisionDiscardsUnpreparedNativeCandidate(t *testing.T) { | |||
| 305 | + | parentWV := &WebView{ctx: context.Background(), id: 24} | |||
| 306 | + | popupWV := &WebView{ctx: context.Background(), id: 32} | |||
| 307 | + | popupWV.markNativePopupCandidate(parentWV) | |||
| 308 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{Kind: dto.HostDecisionCreateBrowserWindow}) | |||
| 309 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |||
| 310 | + | ||||
| 311 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |||
| 312 | + | nil, nil, 98, "https://example.com/window", "window", | |||
| 313 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |||
| 314 | + | ) | |||
| 315 | + | ||||
| 316 | + | require.True(t, blocked) | |||
| 317 | + | require.False(t, popupWV.isNativePopupCandidate()) | |||
| 318 | + | require.False(t, popupWV.nativePopupFallbackStarted) | |||
| 319 | + | } | |||
| 320 | + | ||||
| 321 | + | func TestOnBeforePopup_HostedDecisionPreservesAlreadyPreparedFallbackOpener(t *testing.T) { | |||
| 322 | + | parentWV := &WebView{ctx: context.Background(), id: 25} | |||
| 323 | + | parentWV.updateURI("https://example.com/opener") | |||
| 324 | + | popupWV := &WebView{ctx: context.Background(), id: 33, pendingCreate: &pendingBrowserCreate{}} | |||
| 325 | + | popupWV.markNativePopupCandidate(parentWV) | |||
| 326 | + | require.True(t, popupWV.preparePopupShellDirectBrowserCreation()) | |||
| 327 | + | popupWV.SetBrowsingContextHostDecision(dto.HostDecision{Kind: dto.HostDecisionCreateBrowserWindow}) | |||
| 328 | + | parentWV.SetCallbacks(&port.WebViewCallbacks{OnCreate: func(port.PopupRequest) port.WebView { return popupWV }}) | |||
| 329 | + | ||||
| 330 | + | blocked := (&handlerSet{wv: parentWV}).OnBeforePopup( | |||
| 331 | + | nil, nil, 99, "https://example.com/window", "window", | |||
| 332 | + | purecef.WindowOpenDispositionWodNewPopup, 1, nil, nil, nil, nil, nil, nil, | |||
| 333 | + | ) | |||
| 334 | + | ||||
| 335 | + | require.True(t, blocked) | |||
| 336 | + | require.True(t, popupWV.nativePopupFallbackStarted) | |||
| 337 | + | require.Same(t, parentWV, popupWV.popupOpenerBridgeParent) | |||
| 338 | + | require.Equal(t, "https://example.com/opener", popupWV.popupOpenerBridgeParentURI) | |||
| 339 | + | } | |||
| 340 | + | ||||
| 277 | func TestOnBeforePopup_DenyCleansUp(t *testing.T) { | 341 | func TestOnBeforePopup_DenyCleansUp(t *testing.T) { | ||
| 278 | parentWV := &WebView{ctx: context.Background(), id: 24} | 342 | parentWV := &WebView{ctx: context.Background(), id: 24} | ||
| 279 | popupWV := &WebView{ctx: context.Background(), id: 32} | 343 | popupWV := &WebView{ctx: context.Background(), id: 32} | ||
M internal/ui/coordinator/content/popup_deferred.go +3 -1
| @@ -202,7 +202,9 @@ func (pm *popupManager) resolvePendingPopupFeatures( | |||
| 202 | 202 | case dto.HostDecisionReuseNamedPane: | |
| 203 | 203 | // Reuse transfers no ownership: the existing context is navigated while | |
| 204 | 204 | // the newly staged WebView remains ours and must be discarded below. | |
| 205 | - | _, _ = pm.reuseNamedPopup(ctx, hooks, pending.ParentPaneID, request.FrameName, request.TargetURI) | |
| 205 | + | if _, ok := pm.reuseNamedPopup(ctx, hooks, pending.ParentPaneID, request.FrameName, request.TargetURI); !ok { | |
| 206 | + | logBrowsingContextFailure(*logging.FromContext(ctx), normalized, decision, dto.BrowsingContextFailureHostUnavailable, nil) | |
| 207 | + | } | |
| 206 | 208 | default: | |
| 207 | 209 | logBrowsingContextFailure(*logging.FromContext(ctx), normalized, decision, dto.BrowsingContextFailureFeatureResolution, nil) | |
| 208 | 210 | } | |
| @@ -202,7 +202,9 @@ func (pm *popupManager) resolvePendingPopupFeatures( | |||||
| 202 | case dto.HostDecisionReuseNamedPane: | 202 | case dto.HostDecisionReuseNamedPane: | ||
| 203 | // Reuse transfers no ownership: the existing context is navigated while | 203 | // Reuse transfers no ownership: the existing context is navigated while | ||
| 204 | // the newly staged WebView remains ours and must be discarded below. | 204 | // the newly staged WebView remains ours and must be discarded below. | ||
| 205 | - | _, _ = pm.reuseNamedPopup(ctx, hooks, pending.ParentPaneID, request.FrameName, request.TargetURI) | 205 | + | if _, ok := pm.reuseNamedPopup(ctx, hooks, pending.ParentPaneID, request.FrameName, request.TargetURI); !ok { |
| 206 | + | logBrowsingContextFailure(*logging.FromContext(ctx), normalized, decision, dto.BrowsingContextFailureHostUnavailable, nil) | |||
| 207 | + | } | |||
| 206 | default: | 208 | default: | ||
| 207 | logBrowsingContextFailure(*logging.FromContext(ctx), normalized, decision, dto.BrowsingContextFailureFeatureResolution, nil) | 209 | logBrowsingContextFailure(*logging.FromContext(ctx), normalized, decision, dto.BrowsingContextFailureFeatureResolution, nil) | ||
| 208 | } | 210 | } | ||
M internal/ui/coordinator/content/popup_lifecycle_test.go +73 -1
| @@ -410,6 +410,52 @@ func TestPopupDeferredNamedReuseDestroysNewlyStagedWebViewExactlyOnce(t *testing | |||
| 410 | 410 | assert.Equal(t, 1, staging.destroyCalls) | |
| 411 | 411 | } | |
| 412 | 412 | ||
| 413 | + | func TestPopupDeferredNamedReuseRaceLogsHostUnavailableAndCleansStagingExactlyOnce(t *testing.T) { | |
| 414 | + | var output bytes.Buffer | |
| 415 | + | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | |
| 416 | + | ctx := logging.WithContext(context.Background(), logger) | |
| 417 | + | parentPaneID := entity.PaneID("floating") | |
| 418 | + | existingPaneID := entity.PaneID("existing") | |
| 419 | + | parent := mocks.NewMockWebView(t) | |
| 420 | + | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | |
| 421 | + | staged := &deferredPopupWebViewStub{MockWebView: mocks.NewMockWebView(t), features: dto.PopupFeatures{State: dto.PopupFeaturesNone}} | |
| 422 | + | staged.EXPECT().ID().Return(port.WebViewID(208)).Maybe() | |
| 423 | + | staged.EXPECT().Destroy().Once() | |
| 424 | + | existing := mocks.NewMockWebView(t) | |
| 425 | + | destroyedChecks := 0 | |
| 426 | + | existing.EXPECT().IsDestroyed().RunAndReturn(func() bool { | |
| 427 | + | destroyedChecks++ | |
| 428 | + | return destroyedChecks > 1 | |
| 429 | + | }).Twice() | |
| 430 | + | existing.EXPECT().ID().Return(port.WebViewID(301)).Once() | |
| 431 | + | factory := mocks.NewMockWebViewFactory(t) | |
| 432 | + | factory.EXPECT().CreateRelated(mock.Anything, port.WebViewID(101)).Return(staged, nil).Once() | |
| 433 | + | staging := &popupStagingHostStub{} | |
| 434 | + | c := &Coordinator{webViews: make(map[entity.PaneID]port.WebView), popups: newPopupManager()} | |
| 435 | + | c.SetPopupConfig(factory, &entity.BrowsingContextConfig{OpenInNewPane: false}, nil) | |
| 436 | + | c.SetPopupSourceHostResolver(func(entity.PaneID) dto.SourceHostKind { return dto.SourceHostFloating }) | |
| 437 | + | c.SetPopupWindowIDResolver(func(paneID entity.PaneID) (string, bool) { | |
| 438 | + | windowByPane := map[entity.PaneID]string{parentPaneID: "owner", existingPaneID: "host"} | |
| 439 | + | windowID, ok := windowByPane[paneID] | |
| 440 | + | return windowID, ok | |
| 441 | + | }) | |
| 442 | + | c.SetOnStagePopup(func(context.Context, StagePopupInput) (PopupStagingHost, error) { return staging, nil }) | |
| 443 | + | ||
| 444 | + | require.Same(t, staged, c.handlePopupCreate(ctx, parentPaneID, parent, port.PopupRequest{ | |
| 445 | + | Engine: dto.BrowserEngineWebKit, TargetURI: "https://example.com/reused", FrameName: "shared", | |
| 446 | + | PopupFeatures: dto.PopupFeatures{State: dto.PopupFeaturesUnknown}, | |
| 447 | + | })) | |
| 448 | + | c.RegisterPopupWebView(existingPaneID, existing) | |
| 449 | + | c.popups.namedContexts.Register("owner", "host", "shared", existingPaneID, port.WebViewID(301)) | |
| 450 | + | staged.ready() | |
| 451 | + | staged.ready() | |
| 452 | + | ||
| 453 | + | assert.Equal(t, 2, destroyedChecks) | |
| 454 | + | assert.Equal(t, 1, staging.destroyCalls) | |
| 455 | + | record := decodeBrowsingContextLog(t, output.Bytes()) | |
| 456 | + | assert.Equal(t, "host-unavailable", record["reason_code"]) | |
| 457 | + | } | |
| 458 | + | ||
| 413 | 459 | func TestPopupDeferredNativeAbortWithoutFallbackLogsStructuredFailure(t *testing.T) { | |
| 414 | 460 | var output bytes.Buffer | |
| 415 | 461 | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | |
| @@ -476,6 +522,31 @@ func TestPopupDeferredBrowserHostFailureCleansUpExactlyOnce(t *testing.T) { | |||
| 476 | 522 | assert.Equal(t, 1, staging.destroyCalls) | |
| 477 | 523 | } | |
| 478 | 524 | ||
| 525 | + | func TestPopupDeferredStagingDetachFailureCleansUpExactlyOnce(t *testing.T) { | |
| 526 | + | parent := mocks.NewMockWebView(t) | |
| 527 | + | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | |
| 528 | + | popup := &deferredPopupWebViewStub{MockWebView: mocks.NewMockWebView(t), features: dto.PopupFeatures{State: dto.PopupFeaturesNone}} | |
| 529 | + | popup.EXPECT().ID().Return(port.WebViewID(209)).Maybe() | |
| 530 | + | popup.EXPECT().Destroy().Once() | |
| 531 | + | factory := mocks.NewMockWebViewFactory(t) | |
| 532 | + | factory.EXPECT().CreateRelated(mock.Anything, port.WebViewID(101)).Return(popup, nil).Once() | |
| 533 | + | staging := &popupStagingHostStub{detachErr: errors.New("detach failed")} | |
| 534 | + | c := &Coordinator{popups: newPopupManager()} | |
| 535 | + | c.SetPopupConfig(factory, &entity.BrowsingContextConfig{OpenInNewPane: false}, nil) | |
| 536 | + | c.SetPopupSourceHostResolver(func(entity.PaneID) dto.SourceHostKind { return dto.SourceHostFloating }) | |
| 537 | + | c.SetOnStagePopup(func(context.Context, StagePopupInput) (PopupStagingHost, error) { return staging, nil }) | |
| 538 | + | ||
| 539 | + | require.Same(t, popup, c.handlePopupCreate(context.Background(), "floating", parent, port.PopupRequest{ | |
| 540 | + | Engine: dto.BrowserEngineWebKit, TargetURI: "https://example.com/open", | |
| 541 | + | PopupFeatures: dto.PopupFeatures{State: dto.PopupFeaturesUnknown}, | |
| 542 | + | })) | |
| 543 | + | popup.ready() | |
| 544 | + | popup.ready() | |
| 545 | + | ||
| 546 | + | assert.True(t, staging.detached) | |
| 547 | + | assert.Equal(t, 1, staging.destroyCalls) | |
| 548 | + | } | |
| 549 | + | ||
| 479 | 550 | func TestPopupDeferredStagingFailureDestroysCallerOwnedWebViewOnce(t *testing.T) { | |
| 480 | 551 | parent := mocks.NewMockWebView(t) | |
| 481 | 552 | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | |
| @@ -606,7 +677,8 @@ func (s *deferredPopupWebViewStub) close() { | |||
| 606 | 677 | type popupStagingHostStub struct { | |
| 607 | 678 | attached, detached bool | |
| 608 | 679 | destroyCalls int | |
| 680 | + | detachErr error | |
| 609 | 681 | } | |
| 610 | 682 | ||
| 611 | - | func (s *popupStagingHostStub) Detach() error { s.detached = true; return nil } | |
| 683 | + | func (s *popupStagingHostStub) Detach() error { s.detached = true; return s.detachErr } | |
| 612 | 684 | func (s *popupStagingHostStub) Destroy() { s.destroyCalls++ } | |
| @@ -410,6 +410,52 @@ func TestPopupDeferredNamedReuseDestroysNewlyStagedWebViewExactlyOnce(t *testing | |||||
| 410 | assert.Equal(t, 1, staging.destroyCalls) | 410 | assert.Equal(t, 1, staging.destroyCalls) | ||
| 411 | } | 411 | } | ||
| 412 | 412 | ||||
| 413 | + | func TestPopupDeferredNamedReuseRaceLogsHostUnavailableAndCleansStagingExactlyOnce(t *testing.T) { | |||
| 414 | + | var output bytes.Buffer | |||
| 415 | + | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | |||
| 416 | + | ctx := logging.WithContext(context.Background(), logger) | |||
| 417 | + | parentPaneID := entity.PaneID("floating") | |||
| 418 | + | existingPaneID := entity.PaneID("existing") | |||
| 419 | + | parent := mocks.NewMockWebView(t) | |||
| 420 | + | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | |||
| 421 | + | staged := &deferredPopupWebViewStub{MockWebView: mocks.NewMockWebView(t), features: dto.PopupFeatures{State: dto.PopupFeaturesNone}} | |||
| 422 | + | staged.EXPECT().ID().Return(port.WebViewID(208)).Maybe() | |||
| 423 | + | staged.EXPECT().Destroy().Once() | |||
| 424 | + | existing := mocks.NewMockWebView(t) | |||
| 425 | + | destroyedChecks := 0 | |||
| 426 | + | existing.EXPECT().IsDestroyed().RunAndReturn(func() bool { | |||
| 427 | + | destroyedChecks++ | |||
| 428 | + | return destroyedChecks > 1 | |||
| 429 | + | }).Twice() | |||
| 430 | + | existing.EXPECT().ID().Return(port.WebViewID(301)).Once() | |||
| 431 | + | factory := mocks.NewMockWebViewFactory(t) | |||
| 432 | + | factory.EXPECT().CreateRelated(mock.Anything, port.WebViewID(101)).Return(staged, nil).Once() | |||
| 433 | + | staging := &popupStagingHostStub{} | |||
| 434 | + | c := &Coordinator{webViews: make(map[entity.PaneID]port.WebView), popups: newPopupManager()} | |||
| 435 | + | c.SetPopupConfig(factory, &entity.BrowsingContextConfig{OpenInNewPane: false}, nil) | |||
| 436 | + | c.SetPopupSourceHostResolver(func(entity.PaneID) dto.SourceHostKind { return dto.SourceHostFloating }) | |||
| 437 | + | c.SetPopupWindowIDResolver(func(paneID entity.PaneID) (string, bool) { | |||
| 438 | + | windowByPane := map[entity.PaneID]string{parentPaneID: "owner", existingPaneID: "host"} | |||
| 439 | + | windowID, ok := windowByPane[paneID] | |||
| 440 | + | return windowID, ok | |||
| 441 | + | }) | |||
| 442 | + | c.SetOnStagePopup(func(context.Context, StagePopupInput) (PopupStagingHost, error) { return staging, nil }) | |||
| 443 | + | ||||
| 444 | + | require.Same(t, staged, c.handlePopupCreate(ctx, parentPaneID, parent, port.PopupRequest{ | |||
| 445 | + | Engine: dto.BrowserEngineWebKit, TargetURI: "https://example.com/reused", FrameName: "shared", | |||
| 446 | + | PopupFeatures: dto.PopupFeatures{State: dto.PopupFeaturesUnknown}, | |||
| 447 | + | })) | |||
| 448 | + | c.RegisterPopupWebView(existingPaneID, existing) | |||
| 449 | + | c.popups.namedContexts.Register("owner", "host", "shared", existingPaneID, port.WebViewID(301)) | |||
| 450 | + | staged.ready() | |||
| 451 | + | staged.ready() | |||
| 452 | + | ||||
| 453 | + | assert.Equal(t, 2, destroyedChecks) | |||
| 454 | + | assert.Equal(t, 1, staging.destroyCalls) | |||
| 455 | + | record := decodeBrowsingContextLog(t, output.Bytes()) | |||
| 456 | + | assert.Equal(t, "host-unavailable", record["reason_code"]) | |||
| 457 | + | } | |||
| 458 | + | ||||
| 413 | func TestPopupDeferredNativeAbortWithoutFallbackLogsStructuredFailure(t *testing.T) { | 459 | func TestPopupDeferredNativeAbortWithoutFallbackLogsStructuredFailure(t *testing.T) { | ||
| 414 | var output bytes.Buffer | 460 | var output bytes.Buffer | ||
| 415 | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | 461 | logger := zerolog.New(&output).Level(zerolog.WarnLevel) | ||
| @@ -476,6 +522,31 @@ func TestPopupDeferredBrowserHostFailureCleansUpExactlyOnce(t *testing.T) { | |||||
| 476 | assert.Equal(t, 1, staging.destroyCalls) | 522 | assert.Equal(t, 1, staging.destroyCalls) | ||
| 477 | } | 523 | } | ||
| 478 | 524 | ||||
| 525 | + | func TestPopupDeferredStagingDetachFailureCleansUpExactlyOnce(t *testing.T) { | |||
| 526 | + | parent := mocks.NewMockWebView(t) | |||
| 527 | + | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | |||
| 528 | + | popup := &deferredPopupWebViewStub{MockWebView: mocks.NewMockWebView(t), features: dto.PopupFeatures{State: dto.PopupFeaturesNone}} | |||
| 529 | + | popup.EXPECT().ID().Return(port.WebViewID(209)).Maybe() | |||
| 530 | + | popup.EXPECT().Destroy().Once() | |||
| 531 | + | factory := mocks.NewMockWebViewFactory(t) | |||
| 532 | + | factory.EXPECT().CreateRelated(mock.Anything, port.WebViewID(101)).Return(popup, nil).Once() | |||
| 533 | + | staging := &popupStagingHostStub{detachErr: errors.New("detach failed")} | |||
| 534 | + | c := &Coordinator{popups: newPopupManager()} | |||
| 535 | + | c.SetPopupConfig(factory, &entity.BrowsingContextConfig{OpenInNewPane: false}, nil) | |||
| 536 | + | c.SetPopupSourceHostResolver(func(entity.PaneID) dto.SourceHostKind { return dto.SourceHostFloating }) | |||
| 537 | + | c.SetOnStagePopup(func(context.Context, StagePopupInput) (PopupStagingHost, error) { return staging, nil }) | |||
| 538 | + | ||||
| 539 | + | require.Same(t, popup, c.handlePopupCreate(context.Background(), "floating", parent, port.PopupRequest{ | |||
| 540 | + | Engine: dto.BrowserEngineWebKit, TargetURI: "https://example.com/open", | |||
| 541 | + | PopupFeatures: dto.PopupFeatures{State: dto.PopupFeaturesUnknown}, | |||
| 542 | + | })) | |||
| 543 | + | popup.ready() | |||
| 544 | + | popup.ready() | |||
| 545 | + | ||||
| 546 | + | assert.True(t, staging.detached) | |||
| 547 | + | assert.Equal(t, 1, staging.destroyCalls) | |||
| 548 | + | } | |||
| 549 | + | ||||
| 479 | func TestPopupDeferredStagingFailureDestroysCallerOwnedWebViewOnce(t *testing.T) { | 550 | func TestPopupDeferredStagingFailureDestroysCallerOwnedWebViewOnce(t *testing.T) { | ||
| 480 | parent := mocks.NewMockWebView(t) | 551 | parent := mocks.NewMockWebView(t) | ||
| 481 | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | 552 | parent.EXPECT().ID().Return(port.WebViewID(101)).Once() | ||
| @@ -606,7 +677,8 @@ func (s *deferredPopupWebViewStub) close() { | |||||
| 606 | type popupStagingHostStub struct { | 677 | type popupStagingHostStub struct { | ||
| 607 | attached, detached bool | 678 | attached, detached bool | ||
| 608 | destroyCalls int | 679 | destroyCalls int | ||
| 680 | + | detachErr error | |||
| 609 | } | 681 | } | ||
| 610 | 682 | ||||
| 611 | - | func (s *popupStagingHostStub) Detach() error { s.detached = true; return nil } | 683 | + | func (s *popupStagingHostStub) Detach() error { s.detached = true; return s.detachErr } |
| 612 | func (s *popupStagingHostStub) Destroy() { s.destroyCalls++ } | 684 | func (s *popupStagingHostStub) Destroy() { s.destroyCalls++ } | ||
M internal/ui/popup_staging_host.go +2 -4
| @@ -68,13 +68,11 @@ func (a *App) stagePopup(ctx context.Context, input content.StagePopupInput) (co | |||
| 68 | 68 | return nil, err | |
| 69 | 69 | } | |
| 70 | 70 | widget := a.contentCoord.WrapWidget(ctx, input.PopupWebView) | |
| 71 | - | if widget == nil || widget.GtkWidget() == nil { | |
| 71 | + | gtkWidget, err := prepareNativePopupContentWidget(widget) | |
| 72 | + | if err != nil { | |
| 72 | 73 | shell.Destroy() | |
| 73 | 74 | return nil, fmt.Errorf("failed to wrap popup staging webview widget") | |
| 74 | 75 | } | |
| 75 | - | gtkWidget := widget.GtkWidget() | |
| 76 | - | widget.SetHexpand(true) | |
| 77 | - | widget.SetVexpand(true) | |
| 78 | 76 | shell.SetContent(gtkWidget) | |
| 79 | 77 | return &popupStagingHost{shell: shell, attached: gtkWidget}, nil | |
| 80 | 78 | } | |
| @@ -68,13 +68,11 @@ func (a *App) stagePopup(ctx context.Context, input content.StagePopupInput) (co | |||||
| 68 | return nil, err | 68 | return nil, err | ||
| 69 | } | 69 | } | ||
| 70 | widget := a.contentCoord.WrapWidget(ctx, input.PopupWebView) | 70 | widget := a.contentCoord.WrapWidget(ctx, input.PopupWebView) | ||
| 71 | - | if widget == nil || widget.GtkWidget() == nil { | 71 | + | gtkWidget, err := prepareNativePopupContentWidget(widget) |
| 72 | + | if err != nil { | |||
| 72 | shell.Destroy() | 73 | shell.Destroy() | ||
| 73 | return nil, fmt.Errorf("failed to wrap popup staging webview widget") | 74 | return nil, fmt.Errorf("failed to wrap popup staging webview widget") | ||
| 74 | } | 75 | } | ||
| 75 | - | gtkWidget := widget.GtkWidget() | |||
| 76 | - | widget.SetHexpand(true) | |||
| 77 | - | widget.SetVexpand(true) | |||
| 78 | shell.SetContent(gtkWidget) | 76 | shell.SetContent(gtkWidget) | ||
| 79 | return &popupStagingHost{shell: shell, attached: gtkWidget}, nil | 77 | return &popupStagingHost{shell: shell, attached: gtkWidget}, nil | ||
| 80 | } | 78 | } | ||