cef: harden popup bridge follow-ups
- commit
- 1df2f44751a562d098f3e859c248f392d8de7b74
- author
- brice · · 2026-04-22 18:01 UTC
- parent
- 40f2f13
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
13 files · +105 -23
M internal/domain/errors/errors.go +6 -2
| @@ -1,7 +1,11 @@ | |||
| 1 | 1 | package domainerrors | |
| 2 | 2 | ||
| 3 | - | import "errors" | |
| 3 | + | type domainError string | |
| 4 | + | ||
| 5 | + | func (e domainError) Error() string { | |
| 6 | + | return string(e) | |
| 7 | + | } | |
| 4 | 8 | ||
| 5 | 9 | // ErrRelatedWebViewUnsupported reports that the active engine cannot create | |
| 6 | 10 | // a popup WebView related to an existing parent WebView. | |
| 7 | - | var ErrRelatedWebViewUnsupported = errors.New("related popup webviews are not supported") | |
| 11 | + | var ErrRelatedWebViewUnsupported error = domainError("related popup webviews are not supported") | |
| @@ -1,7 +1,11 @@ | |||||
| 1 | package domainerrors | 1 | package domainerrors | ||
| 2 | 2 | ||||
| 3 | - | import "errors" | 3 | + | type domainError string |
| 4 | + | ||||
| 5 | + | func (e domainError) Error() string { | |||
| 6 | + | return string(e) | |||
| 7 | + | } | |||
| 4 | 8 | ||||
| 5 | // ErrRelatedWebViewUnsupported reports that the active engine cannot create | 9 | // ErrRelatedWebViewUnsupported reports that the active engine cannot create | ||
| 6 | // a popup WebView related to an existing parent WebView. | 10 | // a popup WebView related to an existing parent WebView. | ||
| 7 | - | var ErrRelatedWebViewUnsupported = errors.New("related popup webviews are not supported") | 11 | + | var ErrRelatedWebViewUnsupported error = domainError("related popup webviews are not supported") |
M internal/infrastructure/cef/bridge_script_templates.go +6 -1
| @@ -226,8 +226,13 @@ func buildTrustedPageFetchBridgeBaseJS() string { | |||
| 226 | 226 | return clipboardSelectionFetchBridgeJSTemplate + "\n" + popupBridge | |
| 227 | 227 | } | |
| 228 | 228 | ||
| 229 | + | const bridgeNoncePlaceholder = "__DUMBER_BRIDGE_NONCE__" | |
| 230 | + | ||
| 229 | 231 | func buildTrustedPageFetchBridgeJS(bridgeNonce string) string { | |
| 230 | - | return strings.ReplaceAll(trustedPageFetchBridgeBaseJS, "__DUMBER_BRIDGE_NONCE__", bridgeNonce) | |
| 232 | + | if strings.Contains(bridgeNonce, bridgeNoncePlaceholder) { | |
| 233 | + | panic("bridge nonce contains reserved placeholder") | |
| 234 | + | } | |
| 235 | + | return strings.ReplaceAll(trustedPageFetchBridgeBaseJS, bridgeNoncePlaceholder, bridgeNonce) | |
| 231 | 236 | } | |
| 232 | 237 | ||
| 233 | 238 | func buildRendererBridgeExtensionJS() string { | |
| @@ -226,8 +226,13 @@ func buildTrustedPageFetchBridgeBaseJS() string { | |||||
| 226 | return clipboardSelectionFetchBridgeJSTemplate + "\n" + popupBridge | 226 | return clipboardSelectionFetchBridgeJSTemplate + "\n" + popupBridge | ||
| 227 | } | 227 | } | ||
| 228 | 228 | ||||
| 229 | + | const bridgeNoncePlaceholder = "__DUMBER_BRIDGE_NONCE__" | |||
| 230 | + | ||||
| 229 | func buildTrustedPageFetchBridgeJS(bridgeNonce string) string { | 231 | func buildTrustedPageFetchBridgeJS(bridgeNonce string) string { | ||
| 230 | - | return strings.ReplaceAll(trustedPageFetchBridgeBaseJS, "__DUMBER_BRIDGE_NONCE__", bridgeNonce) | 232 | + | if strings.Contains(bridgeNonce, bridgeNoncePlaceholder) { |
| 233 | + | panic("bridge nonce contains reserved placeholder") | |||
| 234 | + | } | |||
| 235 | + | return strings.ReplaceAll(trustedPageFetchBridgeBaseJS, bridgeNoncePlaceholder, bridgeNonce) | |||
| 231 | } | 236 | } | ||
| 232 | 237 | ||||
| 233 | func buildRendererBridgeExtensionJS() string { | 238 | func buildRendererBridgeExtensionJS() string { | ||
M internal/infrastructure/cef/factory.go +1 -3
| @@ -270,9 +270,7 @@ func (f *WebViewFactory) postPendingBrowserCreate(ctx context.Context, wv *WebVi | |||
| 270 | 270 | postResult := purecef.PostTask(purecef.ThreadIDTidUi, task) | |
| 271 | 271 | if postResult != 1 { | |
| 272 | 272 | wv.mu.Lock() | |
| 273 | - | if wv.pendingCreate == nil { | |
| 274 | - | wv.pendingCreate = pc | |
| 275 | - | } | |
| 273 | + | wv.pendingCreate = pc | |
| 276 | 274 | wv.mu.Unlock() | |
| 277 | 275 | log.Error(). | |
| 278 | 276 | Int32("result", postResult). | |
| @@ -270,9 +270,7 @@ func (f *WebViewFactory) postPendingBrowserCreate(ctx context.Context, wv *WebVi | |||||
| 270 | postResult := purecef.PostTask(purecef.ThreadIDTidUi, task) | 270 | postResult := purecef.PostTask(purecef.ThreadIDTidUi, task) | ||
| 271 | if postResult != 1 { | 271 | if postResult != 1 { | ||
| 272 | wv.mu.Lock() | 272 | wv.mu.Lock() | ||
| 273 | - | if wv.pendingCreate == nil { | 273 | + | wv.pendingCreate = pc |
| 274 | - | wv.pendingCreate = pc | |||
| 275 | - | } | |||
| 276 | wv.mu.Unlock() | 274 | wv.mu.Unlock() | ||
| 277 | log.Error(). | 275 | log.Error(). | ||
| 278 | Int32("result", postResult). | 276 | Int32("result", postResult). | ||
M internal/infrastructure/cef/popup_opener_bridge.go +17 -4
| @@ -182,13 +182,13 @@ func resolvePopupOpenerNavigationTarget(rawTarget, openerURI string) string { | |||
| 182 | 182 | } | |
| 183 | 183 | parsedTarget, err := neturl.Parse(trimmedTarget) | |
| 184 | 184 | if err == nil && parsedTarget.IsAbs() { | |
| 185 | - | return parsedTarget.String() | |
| 185 | + | return sanitizePopupOpenerNavigationTarget(parsedTarget) | |
| 186 | 186 | } | |
| 187 | 187 | if strings.HasPrefix(trimmedTarget, "//") { | |
| 188 | 188 | base, baseErr := neturl.Parse(strings.TrimSpace(openerURI)) | |
| 189 | 189 | if baseErr == nil && base != nil && base.Scheme != "" { | |
| 190 | 190 | parsedTarget.Scheme = base.Scheme | |
| 191 | - | return parsedTarget.String() | |
| 191 | + | return sanitizePopupOpenerNavigationTarget(parsedTarget) | |
| 192 | 192 | } | |
| 193 | 193 | return "" | |
| 194 | 194 | } | |
| @@ -200,7 +200,19 @@ func resolvePopupOpenerNavigationTarget(rawTarget, openerURI string) string { | |||
| 200 | 200 | if err != nil { | |
| 201 | 201 | return "" | |
| 202 | 202 | } | |
| 203 | - | return base.ResolveReference(ref).String() | |
| 203 | + | return sanitizePopupOpenerNavigationTarget(base.ResolveReference(ref)) | |
| 204 | + | } | |
| 205 | + | ||
| 206 | + | func sanitizePopupOpenerNavigationTarget(target *neturl.URL) string { | |
| 207 | + | if target == nil { | |
| 208 | + | return "" | |
| 209 | + | } | |
| 210 | + | switch strings.ToLower(strings.TrimSpace(target.Scheme)) { | |
| 211 | + | case "http", "https": | |
| 212 | + | return target.String() | |
| 213 | + | default: | |
| 214 | + | return "" | |
| 215 | + | } | |
| 204 | 216 | } | |
| 205 | 217 | ||
| 206 | 218 | func (wv *WebView) handlePopupOpenerPostMessage(payload popupOpenerPostMessagePayload) { | |
| @@ -272,7 +284,8 @@ func targetOriginMatchesPopupOpener(targetOrigin, openerURI string) bool { | |||
| 272 | 284 | if trimmedTarget == "" || trimmedTarget == "*" { | |
| 273 | 285 | return true | |
| 274 | 286 | } | |
| 275 | - | return canonicalOrigin(trimmedTarget) != "" && canonicalOrigin(trimmedTarget) == canonicalOrigin(openerURI) | |
| 287 | + | targetCanonical := canonicalOrigin(trimmedTarget) | |
| 288 | + | return targetCanonical != "" && targetCanonical == canonicalOrigin(openerURI) | |
| 276 | 289 | } | |
| 277 | 290 | ||
| 278 | 291 | func popupSourceOrigin(payload popupOpenerPostMessagePayload) string { | |
| @@ -182,13 +182,13 @@ func resolvePopupOpenerNavigationTarget(rawTarget, openerURI string) string { | |||||
| 182 | } | 182 | } | ||
| 183 | parsedTarget, err := neturl.Parse(trimmedTarget) | 183 | parsedTarget, err := neturl.Parse(trimmedTarget) | ||
| 184 | if err == nil && parsedTarget.IsAbs() { | 184 | if err == nil && parsedTarget.IsAbs() { | ||
| 185 | - | return parsedTarget.String() | 185 | + | return sanitizePopupOpenerNavigationTarget(parsedTarget) |
| 186 | } | 186 | } | ||
| 187 | if strings.HasPrefix(trimmedTarget, "//") { | 187 | if strings.HasPrefix(trimmedTarget, "//") { | ||
| 188 | base, baseErr := neturl.Parse(strings.TrimSpace(openerURI)) | 188 | base, baseErr := neturl.Parse(strings.TrimSpace(openerURI)) | ||
| 189 | if baseErr == nil && base != nil && base.Scheme != "" { | 189 | if baseErr == nil && base != nil && base.Scheme != "" { | ||
| 190 | parsedTarget.Scheme = base.Scheme | 190 | parsedTarget.Scheme = base.Scheme | ||
| 191 | - | return parsedTarget.String() | 191 | + | return sanitizePopupOpenerNavigationTarget(parsedTarget) |
| 192 | } | 192 | } | ||
| 193 | return "" | 193 | return "" | ||
| 194 | } | 194 | } | ||
| @@ -200,7 +200,19 @@ func resolvePopupOpenerNavigationTarget(rawTarget, openerURI string) string { | |||||
| 200 | if err != nil { | 200 | if err != nil { | ||
| 201 | return "" | 201 | return "" | ||
| 202 | } | 202 | } | ||
| 203 | - | return base.ResolveReference(ref).String() | 203 | + | return sanitizePopupOpenerNavigationTarget(base.ResolveReference(ref)) |
| 204 | + | } | |||
| 205 | + | ||||
| 206 | + | func sanitizePopupOpenerNavigationTarget(target *neturl.URL) string { | |||
| 207 | + | if target == nil { | |||
| 208 | + | return "" | |||
| 209 | + | } | |||
| 210 | + | switch strings.ToLower(strings.TrimSpace(target.Scheme)) { | |||
| 211 | + | case "http", "https": | |||
| 212 | + | return target.String() | |||
| 213 | + | default: | |||
| 214 | + | return "" | |||
| 215 | + | } | |||
| 204 | } | 216 | } | ||
| 205 | 217 | ||||
| 206 | func (wv *WebView) handlePopupOpenerPostMessage(payload popupOpenerPostMessagePayload) { | 218 | func (wv *WebView) handlePopupOpenerPostMessage(payload popupOpenerPostMessagePayload) { | ||
| @@ -272,7 +284,8 @@ func targetOriginMatchesPopupOpener(targetOrigin, openerURI string) bool { | |||||
| 272 | if trimmedTarget == "" || trimmedTarget == "*" { | 284 | if trimmedTarget == "" || trimmedTarget == "*" { | ||
| 273 | return true | 285 | return true | ||
| 274 | } | 286 | } | ||
| 275 | - | return canonicalOrigin(trimmedTarget) != "" && canonicalOrigin(trimmedTarget) == canonicalOrigin(openerURI) | 287 | + | targetCanonical := canonicalOrigin(trimmedTarget) |
| 288 | + | return targetCanonical != "" && targetCanonical == canonicalOrigin(openerURI) | |||
| 276 | } | 289 | } | ||
| 277 | 290 | ||||
| 278 | func popupSourceOrigin(payload popupOpenerPostMessagePayload) string { | 291 | func popupSourceOrigin(payload popupOpenerPostMessagePayload) string { | ||
M internal/infrastructure/cef/popup_opener_bridge_test.go +10 -2
| @@ -1,8 +1,10 @@ | |||
| 1 | 1 | package cef | |
| 2 | 2 | ||
| 3 | - | import "testing" | |
| 3 | + | import ( | |
| 4 | + | "testing" | |
| 4 | 5 | ||
| 5 | - | import "github.com/stretchr/testify/require" | |
| 6 | + | "github.com/stretchr/testify/require" | |
| 7 | + | ) | |
| 6 | 8 | ||
| 7 | 9 | func TestResolvePopupOpenerNavigationTarget_ResolvesRelativeAgainstOpener(t *testing.T) { | |
| 8 | 10 | require.Equal( | |
| @@ -20,6 +22,12 @@ func TestResolvePopupOpenerNavigationTarget_PreservesAbsoluteTarget(t *testing.T | |||
| 20 | 22 | ) | |
| 21 | 23 | } | |
| 22 | 24 | ||
| 25 | + | func TestResolvePopupOpenerNavigationTarget_RejectsNonHTTPAbsoluteTarget(t *testing.T) { | |
| 26 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("javascript:alert(1)", "https://example.com/oauth/start")) | |
| 27 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("data:text/html,boom", "https://example.com/oauth/start")) | |
| 28 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("file:///tmp/boom", "https://example.com/oauth/start")) | |
| 29 | + | } | |
| 30 | + | ||
| 23 | 31 | func TestOriginFromURL_PreservesNonDefaultPort(t *testing.T) { | |
| 24 | 32 | require.Equal(t, "https://example.com:8443", originFromURL("https://example.com:8443/callback")) | |
| 25 | 33 | } | |
| @@ -1,8 +1,10 @@ | |||||
| 1 | package cef | 1 | package cef | ||
| 2 | 2 | ||||
| 3 | - | import "testing" | 3 | + | import ( |
| 4 | + | "testing" | |||
| 4 | 5 | ||||
| 5 | - | import "github.com/stretchr/testify/require" | 6 | + | "github.com/stretchr/testify/require" |
| 7 | + | ) | |||
| 6 | 8 | ||||
| 7 | func TestResolvePopupOpenerNavigationTarget_ResolvesRelativeAgainstOpener(t *testing.T) { | 9 | func TestResolvePopupOpenerNavigationTarget_ResolvesRelativeAgainstOpener(t *testing.T) { | ||
| 8 | require.Equal( | 10 | require.Equal( | ||
| @@ -20,6 +22,12 @@ func TestResolvePopupOpenerNavigationTarget_PreservesAbsoluteTarget(t *testing.T | |||||
| 20 | ) | 22 | ) | ||
| 21 | } | 23 | } | ||
| 22 | 24 | ||||
| 25 | + | func TestResolvePopupOpenerNavigationTarget_RejectsNonHTTPAbsoluteTarget(t *testing.T) { | |||
| 26 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("javascript:alert(1)", "https://example.com/oauth/start")) | |||
| 27 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("data:text/html,boom", "https://example.com/oauth/start")) | |||
| 28 | + | require.Empty(t, resolvePopupOpenerNavigationTarget("file:///tmp/boom", "https://example.com/oauth/start")) | |||
| 29 | + | } | |||
| 30 | + | ||||
| 23 | func TestOriginFromURL_PreservesNonDefaultPort(t *testing.T) { | 31 | func TestOriginFromURL_PreservesNonDefaultPort(t *testing.T) { | ||
| 24 | require.Equal(t, "https://example.com:8443", originFromURL("https://example.com:8443/callback")) | 32 | require.Equal(t, "https://example.com:8443", originFromURL("https://example.com:8443/callback")) | ||
| 25 | } | 33 | } | ||
M internal/infrastructure/cef/scheme_handler.go +3 -0
| @@ -442,6 +442,9 @@ func decodePopupOpenerPostMessagePayload(body []byte) (popupOpenerPostMessagePay | |||
| 442 | 442 | payload.TargetOrigin = strings.TrimSpace(payload.TargetOrigin) | |
| 443 | 443 | payload.SourceOrigin = strings.TrimSpace(payload.SourceOrigin) | |
| 444 | 444 | payload.SourceHref = strings.TrimSpace(payload.SourceHref) | |
| 445 | + | if payload.TargetOrigin == "" { | |
| 446 | + | return payload, fmt.Errorf("missing target origin") | |
| 447 | + | } | |
| 445 | 448 | return payload, nil | |
| 446 | 449 | } | |
| 447 | 450 | ||
| @@ -442,6 +442,9 @@ func decodePopupOpenerPostMessagePayload(body []byte) (popupOpenerPostMessagePay | |||||
| 442 | payload.TargetOrigin = strings.TrimSpace(payload.TargetOrigin) | 442 | payload.TargetOrigin = strings.TrimSpace(payload.TargetOrigin) | ||
| 443 | payload.SourceOrigin = strings.TrimSpace(payload.SourceOrigin) | 443 | payload.SourceOrigin = strings.TrimSpace(payload.SourceOrigin) | ||
| 444 | payload.SourceHref = strings.TrimSpace(payload.SourceHref) | 444 | payload.SourceHref = strings.TrimSpace(payload.SourceHref) | ||
| 445 | + | if payload.TargetOrigin == "" { | |||
| 446 | + | return payload, fmt.Errorf("missing target origin") | |||
| 447 | + | } | |||
| 445 | return payload, nil | 448 | return payload, nil | ||
| 446 | } | 449 | } | ||
| 447 | 450 | ||||
M internal/infrastructure/cef/scheme_handler_test.go +14 -0
| @@ -70,6 +70,20 @@ func TestReadBodyFromHeader_DecodesBase64Payload(t *testing.T) { | |||
| 70 | 70 | require.JSONEq(t, `{"text":"copied from js"}`, string(body)) | |
| 71 | 71 | } | |
| 72 | 72 | ||
| 73 | + | func TestDecodePopupOpenerPostMessagePayload_RejectsMissingTargetOrigin(t *testing.T) { | |
| 74 | + | _, err := decodePopupOpenerPostMessagePayload([]byte(`{"data":"{}","data_kind":"json","target_origin":" "}`)) | |
| 75 | + | require.Error(t, err) | |
| 76 | + | require.Contains(t, err.Error(), "target origin") | |
| 77 | + | } | |
| 78 | + | ||
| 79 | + | func TestDecodePopupOpenerPostMessagePayload_TrimsAndAcceptsTargetOrigin(t *testing.T) { | |
| 80 | + | payload, err := decodePopupOpenerPostMessagePayload([]byte(`{"data":"{}","data_kind":"json","target_origin":" https://example.com ","source_origin":" https://popup.example.com ","source_href":" https://popup.example.com/callback "}`)) | |
| 81 | + | require.NoError(t, err) | |
| 82 | + | require.Equal(t, "https://example.com", payload.TargetOrigin) | |
| 83 | + | require.Equal(t, "https://popup.example.com", payload.SourceOrigin) | |
| 84 | + | require.Equal(t, "https://popup.example.com/callback", payload.SourceHref) | |
| 85 | + | } | |
| 86 | + | ||
| 73 | 87 | func TestSchemeHandler_APIClipboardSetPathWritesClipboardPayload(t *testing.T) { | |
| 74 | 88 | oldNewResourceHandler := cefNewResourceHandler | |
| 75 | 89 | cefNewResourceHandler = func(impl purecef.ResourceHandler) purecef.ResourceHandler { return impl } | |
| @@ -70,6 +70,20 @@ func TestReadBodyFromHeader_DecodesBase64Payload(t *testing.T) { | |||||
| 70 | require.JSONEq(t, `{"text":"copied from js"}`, string(body)) | 70 | require.JSONEq(t, `{"text":"copied from js"}`, string(body)) | ||
| 71 | } | 71 | } | ||
| 72 | 72 | ||||
| 73 | + | func TestDecodePopupOpenerPostMessagePayload_RejectsMissingTargetOrigin(t *testing.T) { | |||
| 74 | + | _, err := decodePopupOpenerPostMessagePayload([]byte(`{"data":"{}","data_kind":"json","target_origin":" "}`)) | |||
| 75 | + | require.Error(t, err) | |||
| 76 | + | require.Contains(t, err.Error(), "target origin") | |||
| 77 | + | } | |||
| 78 | + | ||||
| 79 | + | func TestDecodePopupOpenerPostMessagePayload_TrimsAndAcceptsTargetOrigin(t *testing.T) { | |||
| 80 | + | payload, err := decodePopupOpenerPostMessagePayload([]byte(`{"data":"{}","data_kind":"json","target_origin":" https://example.com ","source_origin":" https://popup.example.com ","source_href":" https://popup.example.com/callback "}`)) | |||
| 81 | + | require.NoError(t, err) | |||
| 82 | + | require.Equal(t, "https://example.com", payload.TargetOrigin) | |||
| 83 | + | require.Equal(t, "https://popup.example.com", payload.SourceOrigin) | |||
| 84 | + | require.Equal(t, "https://popup.example.com/callback", payload.SourceHref) | |||
| 85 | + | } | |||
| 86 | + | ||||
| 73 | func TestSchemeHandler_APIClipboardSetPathWritesClipboardPayload(t *testing.T) { | 87 | func TestSchemeHandler_APIClipboardSetPathWritesClipboardPayload(t *testing.T) { | ||
| 74 | oldNewResourceHandler := cefNewResourceHandler | 88 | oldNewResourceHandler := cefNewResourceHandler | ||
| 75 | cefNewResourceHandler = func(impl purecef.ResourceHandler) purecef.ResourceHandler { return impl } | 89 | cefNewResourceHandler = func(impl purecef.ResourceHandler) purecef.ResourceHandler { return impl } | ||
M internal/infrastructure/cef/webview.go +9 -1
| @@ -1544,11 +1544,19 @@ func (wv *WebView) runOnGTK(fn func()) { | |||
| 1544 | 1544 | glib.IdleAddOnce(cb, 0) | |
| 1545 | 1545 | } | |
| 1546 | 1546 | ||
| 1547 | + | func (wv *WebView) isOnGTKThread() bool { | |
| 1548 | + | mainContext := glib.MainContextDefault() | |
| 1549 | + | return mainContext != nil && mainContext.IsOwner() | |
| 1550 | + | } | |
| 1551 | + | ||
| 1552 | + | // runOnGTKSync executes fn on the GTK main context and waits for completion. | |
| 1553 | + | // Callers already running on the GTK thread must execute inline to avoid | |
| 1554 | + | // self-deadlocking while waiting for an IdleAddOnce callback. | |
| 1547 | 1555 | func (wv *WebView) runOnGTKSync(fn func()) { | |
| 1548 | 1556 | if fn == nil { | |
| 1549 | 1557 | return | |
| 1550 | 1558 | } | |
| 1551 | - | if wv == nil || wv.engine == nil { | |
| 1559 | + | if wv == nil || wv.engine == nil || wv.isOnGTKThread() { | |
| 1552 | 1560 | fn() | |
| 1553 | 1561 | return | |
| 1554 | 1562 | } | |
| @@ -1544,11 +1544,19 @@ func (wv *WebView) runOnGTK(fn func()) { | |||||
| 1544 | glib.IdleAddOnce(cb, 0) | 1544 | glib.IdleAddOnce(cb, 0) | ||
| 1545 | } | 1545 | } | ||
| 1546 | 1546 | ||||
| 1547 | + | func (wv *WebView) isOnGTKThread() bool { | |||
| 1548 | + | mainContext := glib.MainContextDefault() | |||
| 1549 | + | return mainContext != nil && mainContext.IsOwner() | |||
| 1550 | + | } | |||
| 1551 | + | ||||
| 1552 | + | // runOnGTKSync executes fn on the GTK main context and waits for completion. | |||
| 1553 | + | // Callers already running on the GTK thread must execute inline to avoid | |||
| 1554 | + | // self-deadlocking while waiting for an IdleAddOnce callback. | |||
| 1547 | func (wv *WebView) runOnGTKSync(fn func()) { | 1555 | func (wv *WebView) runOnGTKSync(fn func()) { | ||
| 1548 | if fn == nil { | 1556 | if fn == nil { | ||
| 1549 | return | 1557 | return | ||
| 1550 | } | 1558 | } | ||
| 1551 | - | if wv == nil || wv.engine == nil { | 1559 | + | if wv == nil || wv.engine == nil || wv.isOnGTKThread() { |
| 1552 | fn() | 1560 | fn() | ||
| 1553 | return | 1561 | return | ||
| 1554 | } | 1562 | } | ||
M internal/infrastructure/cef/webview_popup_proxy.go +2 -0
| @@ -28,6 +28,8 @@ func (wv *WebView) syntheticPopupState(proxyID string) *syntheticPopupState { | |||
| 28 | 28 | return wv.syntheticPopups[proxyID] | |
| 29 | 29 | } | |
| 30 | 30 | ||
| 31 | + | // syntheticPopupStateLocked returns the synthetic popup state for proxyID. | |
| 32 | + | // Caller must hold wv.syntheticPopupMu. | |
| 31 | 33 | func (wv *WebView) syntheticPopupStateLocked(proxyID string) *syntheticPopupState { | |
| 32 | 34 | if wv == nil || proxyID == "" { | |
| 33 | 35 | return nil | |
| @@ -28,6 +28,8 @@ func (wv *WebView) syntheticPopupState(proxyID string) *syntheticPopupState { | |||||
| 28 | return wv.syntheticPopups[proxyID] | 28 | return wv.syntheticPopups[proxyID] | ||
| 29 | } | 29 | } | ||
| 30 | 30 | ||||
| 31 | + | // syntheticPopupStateLocked returns the synthetic popup state for proxyID. | |||
| 32 | + | // Caller must hold wv.syntheticPopupMu. | |||
| 31 | func (wv *WebView) syntheticPopupStateLocked(proxyID string) *syntheticPopupState { | 33 | func (wv *WebView) syntheticPopupStateLocked(proxyID string) *syntheticPopupState { | ||
| 32 | if wv == nil || proxyID == "" { | 34 | if wv == nil || proxyID == "" { | ||
| 33 | return nil | 35 | return nil | ||
M internal/ui/coordinator/content/coordinator.go +18 -7
| @@ -83,7 +83,8 @@ type Coordinator struct { | |||
| 83 | 83 | ||
| 84 | 84 | // Popup handling stays in a dedicated UI-layer manager so popup-specific | |
| 85 | 85 | // pane state does not bloat the main coordinator. | |
| 86 | - | popups *popupManager | |
| 86 | + | popups *popupManager | |
| 87 | + | popupsOnce sync.Once | |
| 87 | 88 | ||
| 88 | 89 | // Idle inhibitor for fullscreen video playback | |
| 89 | 90 | idleInhibitor port.IdleInhibitor | |
| @@ -155,9 +156,11 @@ func (c *Coordinator) ensurePopupManager() *popupManager { | |||
| 155 | 156 | if c == nil { | |
| 156 | 157 | return nil | |
| 157 | 158 | } | |
| 158 | - | if c.popups == nil { | |
| 159 | - | c.popups = newPopupManager() | |
| 160 | - | } | |
| 159 | + | c.popupsOnce.Do(func() { | |
| 160 | + | if c.popups == nil { | |
| 161 | + | c.popups = newPopupManager() | |
| 162 | + | } | |
| 163 | + | }) | |
| 161 | 164 | c.popups.ensureInitialized() | |
| 162 | 165 | return c.popups | |
| 163 | 166 | } | |
| @@ -322,20 +325,28 @@ func (c *Coordinator) deleteWebViewLocked(paneID entity.PaneID) port.WebView { | |||
| 322 | 325 | func (c *Coordinator) paneIDByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | |
| 323 | 326 | c.webViewsMu.RLock() | |
| 324 | 327 | defer c.webViewsMu.RUnlock() | |
| 328 | + | return c.paneIDByWebViewIDLocked(webViewID) | |
| 329 | + | } | |
| 330 | + | ||
| 331 | + | func (c *Coordinator) paneIDByWebViewIDLocked(webViewID port.WebViewID) (entity.PaneID, bool) { | |
| 325 | 332 | paneID, ok := c.webViewPaneIDs[webViewID] | |
| 326 | 333 | return paneID, ok | |
| 327 | 334 | } | |
| 328 | 335 | ||
| 329 | 336 | func (c *Coordinator) findPaneByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | |
| 330 | - | if paneID, ok := c.paneIDByWebViewID(webViewID); ok && paneID != "" { | |
| 331 | - | return paneID, true | |
| 332 | - | } | |
| 333 | 337 | if webViewID == 0 { | |
| 334 | 338 | return "", false | |
| 335 | 339 | } | |
| 336 | 340 | ||
| 337 | 341 | c.webViewsMu.RLock() | |
| 338 | 342 | defer c.webViewsMu.RUnlock() | |
| 343 | + | if paneID, ok := c.paneIDByWebViewIDLocked(webViewID); ok && paneID != "" { | |
| 344 | + | return paneID, true | |
| 345 | + | } | |
| 346 | + | ||
| 347 | + | c.logger.Debug(). | |
| 348 | + | Uint64("webview_id", uint64(webViewID)). | |
| 349 | + | Msg("findPaneByWebViewID: using fallback scan") | |
| 339 | 350 | for paneID, wv := range c.webViews { | |
| 340 | 351 | if wv != nil && wv.ID() == webViewID { | |
| 341 | 352 | return paneID, true | |
| @@ -83,7 +83,8 @@ type Coordinator struct { | |||||
| 83 | 83 | ||||
| 84 | // Popup handling stays in a dedicated UI-layer manager so popup-specific | 84 | // Popup handling stays in a dedicated UI-layer manager so popup-specific | ||
| 85 | // pane state does not bloat the main coordinator. | 85 | // pane state does not bloat the main coordinator. | ||
| 86 | - | popups *popupManager | 86 | + | popups *popupManager |
| 87 | + | popupsOnce sync.Once | |||
| 87 | 88 | ||||
| 88 | // Idle inhibitor for fullscreen video playback | 89 | // Idle inhibitor for fullscreen video playback | ||
| 89 | idleInhibitor port.IdleInhibitor | 90 | idleInhibitor port.IdleInhibitor | ||
| @@ -155,9 +156,11 @@ func (c *Coordinator) ensurePopupManager() *popupManager { | |||||
| 155 | if c == nil { | 156 | if c == nil { | ||
| 156 | return nil | 157 | return nil | ||
| 157 | } | 158 | } | ||
| 158 | - | if c.popups == nil { | 159 | + | c.popupsOnce.Do(func() { |
| 159 | - | c.popups = newPopupManager() | 160 | + | if c.popups == nil { |
| 160 | - | } | 161 | + | c.popups = newPopupManager() |
| 162 | + | } | |||
| 163 | + | }) | |||
| 161 | c.popups.ensureInitialized() | 164 | c.popups.ensureInitialized() | ||
| 162 | return c.popups | 165 | return c.popups | ||
| 163 | } | 166 | } | ||
| @@ -322,20 +325,28 @@ func (c *Coordinator) deleteWebViewLocked(paneID entity.PaneID) port.WebView { | |||||
| 322 | func (c *Coordinator) paneIDByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | 325 | func (c *Coordinator) paneIDByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | ||
| 323 | c.webViewsMu.RLock() | 326 | c.webViewsMu.RLock() | ||
| 324 | defer c.webViewsMu.RUnlock() | 327 | defer c.webViewsMu.RUnlock() | ||
| 328 | + | return c.paneIDByWebViewIDLocked(webViewID) | |||
| 329 | + | } | |||
| 330 | + | ||||
| 331 | + | func (c *Coordinator) paneIDByWebViewIDLocked(webViewID port.WebViewID) (entity.PaneID, bool) { | |||
| 325 | paneID, ok := c.webViewPaneIDs[webViewID] | 332 | paneID, ok := c.webViewPaneIDs[webViewID] | ||
| 326 | return paneID, ok | 333 | return paneID, ok | ||
| 327 | } | 334 | } | ||
| 328 | 335 | ||||
| 329 | func (c *Coordinator) findPaneByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | 336 | func (c *Coordinator) findPaneByWebViewID(webViewID port.WebViewID) (entity.PaneID, bool) { | ||
| 330 | - | if paneID, ok := c.paneIDByWebViewID(webViewID); ok && paneID != "" { | |||
| 331 | - | return paneID, true | |||
| 332 | - | } | |||
| 333 | if webViewID == 0 { | 337 | if webViewID == 0 { | ||
| 334 | return "", false | 338 | return "", false | ||
| 335 | } | 339 | } | ||
| 336 | 340 | ||||
| 337 | c.webViewsMu.RLock() | 341 | c.webViewsMu.RLock() | ||
| 338 | defer c.webViewsMu.RUnlock() | 342 | defer c.webViewsMu.RUnlock() | ||
| 343 | + | if paneID, ok := c.paneIDByWebViewIDLocked(webViewID); ok && paneID != "" { | |||
| 344 | + | return paneID, true | |||
| 345 | + | } | |||
| 346 | + | ||||
| 347 | + | c.logger.Debug(). | |||
| 348 | + | Uint64("webview_id", uint64(webViewID)). | |||
| 349 | + | Msg("findPaneByWebViewID: using fallback scan") | |||
| 339 | for paneID, wv := range c.webViews { | 350 | for paneID, wv := range c.webViews { | ||
| 340 | if wv != nil && wv.ID() == webViewID { | 351 | if wv != nil && wv.ID() == webViewID { | ||
| 341 | return paneID, true | 352 | return paneID, true | ||
M internal/ui/coordinator/content/oauth.go +5 -0
| @@ -386,6 +386,11 @@ func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.We | |||
| 386 | 386 | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.ParentURIAtOpen, state.CallbackURI) | |
| 387 | 387 | } | |
| 388 | 388 | ||
| 389 | + | // oauthParentResumeGraceRetries gives the parent pane up to | |
| 390 | + | // oauthParentResumeGraceRetries * oauthParentRefreshDebounce (~1s with the | |
| 391 | + | // current 200ms debounce) to finish its own in-flight same-site OAuth | |
| 392 | + | // navigation before scheduleParentPaneOAuthResume falls back to Reload(). Five | |
| 393 | + | // retries keeps the UI responsive without racing the natural callback handoff. | |
| 389 | 394 | const oauthParentResumeGraceRetries = 5 | |
| 390 | 395 | ||
| 391 | 396 | func (c *Coordinator) scheduleParentPaneOAuthResume( | |
| @@ -386,6 +386,11 @@ func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.We | |||||
| 386 | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.ParentURIAtOpen, state.CallbackURI) | 386 | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.ParentURIAtOpen, state.CallbackURI) | ||
| 387 | } | 387 | } | ||
| 388 | 388 | ||||
| 389 | + | // oauthParentResumeGraceRetries gives the parent pane up to | |||
| 390 | + | // oauthParentResumeGraceRetries * oauthParentRefreshDebounce (~1s with the | |||
| 391 | + | // current 200ms debounce) to finish its own in-flight same-site OAuth | |||
| 392 | + | // navigation before scheduleParentPaneOAuthResume falls back to Reload(). Five | |||
| 393 | + | // retries keeps the UI responsive without racing the natural callback handoff. | |||
| 389 | const oauthParentResumeGraceRetries = 5 | 394 | const oauthParentResumeGraceRetries = 5 | ||
| 390 | 395 | ||||
| 391 | func (c *Coordinator) scheduleParentPaneOAuthResume( | 396 | func (c *Coordinator) scheduleParentPaneOAuthResume( | ||
M internal/ui/coordinator/content/oauth_test.go +2 -0
| @@ -153,9 +153,11 @@ func TestResumeParentPaneAfterOAuth_ChangedParentSkipsIntervention(t *testing.T) | |||
| 153 | 153 | webViews: map[entity.PaneID]port.WebView{ | |
| 154 | 154 | parentPaneID: parentWV, | |
| 155 | 155 | }, | |
| 156 | + | popups: newPopupManager(), | |
| 156 | 157 | } | |
| 157 | 158 | ||
| 158 | 159 | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | |
| 160 | + | parentWV.AssertNotCalled(t, "Reload", mock.Anything) | |
| 159 | 161 | } | |
| 160 | 162 | ||
| 161 | 163 | func TestResumeParentPaneAfterOAuthAttempt_GraceExhaustedFallsBackToReload(t *testing.T) { | |
| @@ -153,9 +153,11 @@ func TestResumeParentPaneAfterOAuth_ChangedParentSkipsIntervention(t *testing.T) | |||||
| 153 | webViews: map[entity.PaneID]port.WebView{ | 153 | webViews: map[entity.PaneID]port.WebView{ | ||
| 154 | parentPaneID: parentWV, | 154 | parentPaneID: parentWV, | ||
| 155 | }, | 155 | }, | ||
| 156 | + | popups: newPopupManager(), | |||
| 156 | } | 157 | } | ||
| 157 | 158 | ||||
| 158 | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | 159 | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | ||
| 160 | + | parentWV.AssertNotCalled(t, "Reload", mock.Anything) | |||
| 159 | } | 161 | } | ||
| 160 | 162 | ||||
| 161 | func TestResumeParentPaneAfterOAuthAttempt_GraceExhaustedFallsBackToReload(t *testing.T) { | 163 | func TestResumeParentPaneAfterOAuthAttempt_GraceExhaustedFallsBackToReload(t *testing.T) { | ||
M internal/ui/coordinator/content/popup_manager.go +12 -3
| @@ -77,6 +77,8 @@ func (pm *popupManager) ensureInitialized() { | |||
| 77 | 77 | if pm == nil { | |
| 78 | 78 | return | |
| 79 | 79 | } | |
| 80 | + | pm.mu.Lock() | |
| 81 | + | defer pm.mu.Unlock() | |
| 80 | 82 | if pm.pendingPopups == nil { | |
| 81 | 83 | pm.pendingPopups = make(map[port.WebViewID]*PendingPopup) | |
| 82 | 84 | } | |
| @@ -321,16 +323,23 @@ func (pm *popupManager) schedulePopupRefresh(parentPaneID entity.PaneID, debounc | |||
| 321 | 323 | if pm == nil || fn == nil { | |
| 322 | 324 | return | |
| 323 | 325 | } | |
| 326 | + | ||
| 327 | + | var timer *time.Timer | |
| 324 | 328 | pm.mu.Lock() | |
| 325 | 329 | if existing := pm.popupRefresh[parentPaneID]; existing != nil { | |
| 326 | 330 | existing.Stop() | |
| 327 | 331 | } | |
| 328 | - | pm.popupRefresh[parentPaneID] = time.AfterFunc(debounce, func() { | |
| 332 | + | timer = time.AfterFunc(debounce, func() { | |
| 329 | 333 | pm.mu.Lock() | |
| 334 | + | if pm.popupRefresh[parentPaneID] != timer { | |
| 335 | + | pm.mu.Unlock() | |
| 336 | + | return | |
| 337 | + | } | |
| 330 | 338 | delete(pm.popupRefresh, parentPaneID) | |
| 331 | 339 | pm.mu.Unlock() | |
| 332 | 340 | fn() | |
| 333 | 341 | }) | |
| 342 | + | pm.popupRefresh[parentPaneID] = timer | |
| 334 | 343 | pm.mu.Unlock() | |
| 335 | 344 | } | |
| 336 | 345 | ||
| @@ -641,10 +650,10 @@ func (pm *popupManager) handlePopupReadyToShow(ctx context.Context, popupID port | |||
| 641 | 650 | Msg("popup ready to show - making visible") | |
| 642 | 651 | ||
| 643 | 652 | if pending.WebView != nil { | |
| 644 | - | if lifecycle, ok := pending.WebView.(port.PopupLifecycleCapable); ok { | |
| 653 | + | lifecycle, preloadsNavigation := pending.WebView.(port.PopupLifecycleCapable) | |
| 654 | + | if preloadsNavigation { | |
| 645 | 655 | lifecycle.Show() | |
| 646 | 656 | } | |
| 647 | - | _, preloadsNavigation := pending.WebView.(port.PopupLifecycleCapable) | |
| 648 | 657 | if !preloadsNavigation && pending.TargetURI != "" && !pending.WebView.IsLoading() && pending.WebView.URI() == "" { | |
| 649 | 658 | if err := pending.WebView.LoadURI(ctx, pending.TargetURI); err != nil { | |
| 650 | 659 | log.Warn().Err(err). | |
| @@ -77,6 +77,8 @@ func (pm *popupManager) ensureInitialized() { | |||||
| 77 | if pm == nil { | 77 | if pm == nil { | ||
| 78 | return | 78 | return | ||
| 79 | } | 79 | } | ||
| 80 | + | pm.mu.Lock() | |||
| 81 | + | defer pm.mu.Unlock() | |||
| 80 | if pm.pendingPopups == nil { | 82 | if pm.pendingPopups == nil { | ||
| 81 | pm.pendingPopups = make(map[port.WebViewID]*PendingPopup) | 83 | pm.pendingPopups = make(map[port.WebViewID]*PendingPopup) | ||
| 82 | } | 84 | } | ||
| @@ -321,16 +323,23 @@ func (pm *popupManager) schedulePopupRefresh(parentPaneID entity.PaneID, debounc | |||||
| 321 | if pm == nil || fn == nil { | 323 | if pm == nil || fn == nil { | ||
| 322 | return | 324 | return | ||
| 323 | } | 325 | } | ||
| 326 | + | ||||
| 327 | + | var timer *time.Timer | |||
| 324 | pm.mu.Lock() | 328 | pm.mu.Lock() | ||
| 325 | if existing := pm.popupRefresh[parentPaneID]; existing != nil { | 329 | if existing := pm.popupRefresh[parentPaneID]; existing != nil { | ||
| 326 | existing.Stop() | 330 | existing.Stop() | ||
| 327 | } | 331 | } | ||
| 328 | - | pm.popupRefresh[parentPaneID] = time.AfterFunc(debounce, func() { | 332 | + | timer = time.AfterFunc(debounce, func() { |
| 329 | pm.mu.Lock() | 333 | pm.mu.Lock() | ||
| 334 | + | if pm.popupRefresh[parentPaneID] != timer { | |||
| 335 | + | pm.mu.Unlock() | |||
| 336 | + | return | |||
| 337 | + | } | |||
| 330 | delete(pm.popupRefresh, parentPaneID) | 338 | delete(pm.popupRefresh, parentPaneID) | ||
| 331 | pm.mu.Unlock() | 339 | pm.mu.Unlock() | ||
| 332 | fn() | 340 | fn() | ||
| 333 | }) | 341 | }) | ||
| 342 | + | pm.popupRefresh[parentPaneID] = timer | |||
| 334 | pm.mu.Unlock() | 343 | pm.mu.Unlock() | ||
| 335 | } | 344 | } | ||
| 336 | 345 | ||||
| @@ -641,10 +650,10 @@ func (pm *popupManager) handlePopupReadyToShow(ctx context.Context, popupID port | |||||
| 641 | Msg("popup ready to show - making visible") | 650 | Msg("popup ready to show - making visible") | ||
| 642 | 651 | ||||
| 643 | if pending.WebView != nil { | 652 | if pending.WebView != nil { | ||
| 644 | - | if lifecycle, ok := pending.WebView.(port.PopupLifecycleCapable); ok { | 653 | + | lifecycle, preloadsNavigation := pending.WebView.(port.PopupLifecycleCapable) |
| 654 | + | if preloadsNavigation { | |||
| 645 | lifecycle.Show() | 655 | lifecycle.Show() | ||
| 646 | } | 656 | } | ||
| 647 | - | _, preloadsNavigation := pending.WebView.(port.PopupLifecycleCapable) | |||
| 648 | if !preloadsNavigation && pending.TargetURI != "" && !pending.WebView.IsLoading() && pending.WebView.URI() == "" { | 657 | if !preloadsNavigation && pending.TargetURI != "" && !pending.WebView.IsLoading() && pending.WebView.URI() == "" { | ||
| 649 | if err := pending.WebView.LoadURI(ctx, pending.TargetURI); err != nil { | 658 | if err := pending.WebView.LoadURI(ctx, pending.TargetURI); err != nil { | ||
| 650 | log.Warn().Err(err). | 659 | log.Warn().Err(err). | ||