fix(webkit): prevent SIGSEGV crash when closing popups by disconnecting signals

Add DisconnectSignals() method to WebView to properly disconnect all GLib signal
handlers before destruction. Update pool.Release() to always destroy used WebViews
instead of recycling them, preventing crashes from async GLib signals firing after
the widget tree has been modified. Add Close() method for programmatic popup closure.

commit: 884f77a5287f4973f64da8cbcb13e63208acab3d
author: brice · 10mo ago · 2025-12-17 18:44 UTC
committer: brice · 10mo ago · 2025-12-17 18:28 UTC
parent: 8a8a4e5
signature: unsigned

2 files · +110 -24

M internal/infrastructure/webkit/pool.go +12 -20


@@ -121,7 +121,9 @@ func (p WebViewPool) createWebView(ctx context.Context) (WebView, error) {
121	121		}
122	122
123	123		// Release returns a WebView to the pool.
124		-	// The WebView is reset to about:blank before being pooled.
	124	+	// For safety, we always destroy and never pool WebViews that have been used.
	125	+	// This prevents crashes from GLib signals firing after the widget tree is modified.
	126	+	// The pool is only used for prewarmed WebViews that haven't been attached to UI yet.
125	127		func (p WebViewPool) Release(ctx context.Context, wv WebView) {
126	128		log := logging.FromContext(ctx)
127	129

@@ -132,25 +134,15 @@ func (p WebViewPool) Release(ctx context.Context, wv WebView) {
132	134		return
133	135		}
134	136
135		-	// Reset to blank state
136		-	_ = wv.LoadURI(ctx, "about:blank")
137		-
138		-	// Clear callbacks
139		-	wv.OnLoadChanged = nil
140		-	wv.OnTitleChanged = nil
141		-	wv.OnURIChanged = nil
142		-	wv.OnProgressChanged = nil
143		-	wv.OnClose = nil
144		-
145		-	// Try to return to pool (non-blocking)
146		-	select {
147		-	case p.pool <- wv:
148		-	log.Debug().Uint64("id", uint64(wv.ID())).Msg("returned webview to pool")
149		-	default:
150		-	// Pool full, destroy the view
151		-	log.Debug().Uint64("id", uint64(wv.ID())).Msg("pool full, destroying webview")
152		-	wv.Destroy()
153		-	}
	137	+	// CRITICAL: Always destroy used WebViews instead of pooling them.
	138	+	// GLib signals are connected with closures that can fire asynchronously
	139	+	// even after we clear Go callbacks. Loading about:blank triggers signals
	140	+	// that can crash if the widget tree has been modified.
	141	+	//
	142	+	// The pool is designed for prewarmed WebViews only, not for recycling
	143	+	// WebViews that have been attached to the UI.
	144	+	log.Debug().Uint64("id", uint64(wv.ID())).Msg("destroying webview (not pooling used views)")
	145	+	wv.Destroy()
154	146		}
155	147
156	148		// Prewarm creates WebViews synchronously to populate the pool.


@@ -121,7 +121,9 @@ func (p WebViewPool) createWebView(ctx context.Context) (WebView, error) {
121		}	121		}
122			122
123		// Release returns a WebView to the pool.	123		// Release returns a WebView to the pool.
124	-	// The WebView is reset to about:blank before being pooled.	124	+	// For safety, we always destroy and never pool WebViews that have been used.
			125	+	// This prevents crashes from GLib signals firing after the widget tree is modified.
			126	+	// The pool is only used for prewarmed WebViews that haven't been attached to UI yet.
125		func (p WebViewPool) Release(ctx context.Context, wv WebView) {	127		func (p WebViewPool) Release(ctx context.Context, wv WebView) {
126		log := logging.FromContext(ctx)	128		log := logging.FromContext(ctx)
127			129

@@ -132,25 +134,15 @@ func (p WebViewPool) Release(ctx context.Context, wv WebView) {
132		return	134		return
133		}	135		}
134			136
135	-	// Reset to blank state	137	+	// CRITICAL: Always destroy used WebViews instead of pooling them.
136	-	_ = wv.LoadURI(ctx, "about:blank")	138	+	// GLib signals are connected with closures that can fire asynchronously
137	-		139	+	// even after we clear Go callbacks. Loading about:blank triggers signals
138	-	// Clear callbacks	140	+	// that can crash if the widget tree has been modified.
139	-	wv.OnLoadChanged = nil	141	+	//
140	-	wv.OnTitleChanged = nil	142	+	// The pool is designed for prewarmed WebViews only, not for recycling
141	-	wv.OnURIChanged = nil	143	+	// WebViews that have been attached to the UI.
142	-	wv.OnProgressChanged = nil	144	+	log.Debug().Uint64("id", uint64(wv.ID())).Msg("destroying webview (not pooling used views)")
143	-	wv.OnClose = nil	145	+	wv.Destroy()
144	-
145	-	// Try to return to pool (non-blocking)
146	-	select {
147	-	case p.pool <- wv:
148	-	log.Debug().Uint64("id", uint64(wv.ID())).Msg("returned webview to pool")
149	-	default:
150	-	// Pool full, destroy the view
151	-	log.Debug().Uint64("id", uint64(wv.ID())).Msg("pool full, destroying webview")
152	-	wv.Destroy()
153	-	}
154		}	146		}
155			147
156		// Prewarm creates WebViews synchronously to populate the pool.	148		// Prewarm creates WebViews synchronously to populate the pool.

M internal/infrastructure/webkit/webview.go +98 -4


@@ -106,6 +106,7 @@ type WebView struct {
106	106		OnClose func()
107	107		OnCreate func(PopupRequest) *WebView // Return new WebView or nil to block popup
108	108		OnReadyToShow func() // Called when popup is ready to display
	109	+	OnLinkMiddleClick func(uri string) bool // Return true if handled (blocks navigation)
109	110
110	111		logger zerolog.Logger
111	112		mu sync.RWMutex

@@ -337,6 +338,72 @@ func (wv *WebView) connectSignals() {
337	338		}
338	339		sigID = gobject.SignalConnect(wv.inner.GoPointer(), "notify::estimated-load-progress", glib.NewCallback(&progressCb))
339	340		wv.signalIDs = append(wv.signalIDs, sigID)
	341	+
	342	+	// decide-policy signal for intercepting middle-click and Ctrl+click on links
	343	+	decidePolicyCb := func(inner webkit.WebView, decisionPtr uintptr, decisionType webkit.PolicyDecisionType) bool {
	344	+	// Only handle navigation actions (not new window or response policies)
	345	+	if decisionType != webkit.PolicyDecisionTypeNavigationActionValue {
	346	+	return false // Let WebKit handle
	347	+	}
	348	+
	349	+	// Cast to NavigationPolicyDecision
	350	+	navDecision := webkit.NavigationPolicyDecisionNewFromInternalPtr(decisionPtr)
	351	+	if navDecision == nil {
	352	+	return false
	353	+	}
	354	+
	355	+	navAction := navDecision.GetNavigationAction()
	356	+	if navAction == nil {
	357	+	return false
	358	+	}
	359	+
	360	+	// Only handle link clicks
	361	+	if navAction.GetNavigationType() != webkit.NavigationTypeLinkClickedValue {
	362	+	return false
	363	+	}
	364	+
	365	+	// Get mouse button and modifiers
	366	+	mouseButton := navAction.GetMouseButton()
	367	+	modifiers := navAction.GetModifiers()
	368	+
	369	+	// Check for middle-click (button 2) or Ctrl+left-click (button 1 + Ctrl)
	370	+	isMiddleClick := mouseButton == 2
	371	+	isCtrlClick := mouseButton == 1 && (gdk.ModifierType(modifiers)&gdk.ControlMaskValue) != 0
	372	+
	373	+	if !isMiddleClick && !isCtrlClick {
	374	+	return false // Normal click, let WebKit handle
	375	+	}
	376	+
	377	+	// Get the link URL
	378	+	request := navAction.GetRequest()
	379	+	if request == nil {
	380	+	return false
	381	+	}
	382	+
	383	+	linkURI := request.GetUri()
	384	+	if linkURI == "" {
	385	+	return false
	386	+	}
	387	+
	388	+	wv.logger.Debug().
	389	+	Str("uri", linkURI).
	390	+	Uint("button", mouseButton).
	391	+	Bool("ctrl", isCtrlClick).
	392	+	Msg("middle-click/ctrl+click on link detected")
	393	+
	394	+	// Call the handler
	395	+	if wv.OnLinkMiddleClick != nil {
	396	+	if wv.OnLinkMiddleClick(linkURI) {
	397	+	// Handler processed it, block the navigation
	398	+	navDecision.Ignore()
	399	+	return true
	400	+	}
	401	+	}
	402	+
	403	+	return false // Let WebKit handle if not handled
	404	+	}
	405	+	sigID = wv.inner.ConnectDecidePolicy(&decidePolicyCb)
	406	+	wv.signalIDs = append(wv.signalIDs, sigID)
340	407		}
341	408
342	409		// ID returns the unique identifier for this WebView.

@@ -579,19 +646,46 @@ func (wv *WebView) IsDestroyed() bool {
579	646		return wv.destroyed.Load()
580	647		}
581	648
	649	+	// Close triggers the close callback as if window.close() was called.
	650	+	// This is used for programmatic popup closure (e.g., OAuth auto-close).
	651	+	func (wv *WebView) Close() {
	652	+	if wv.destroyed.Load() {
	653	+	return
	654	+	}
	655	+	if wv.OnClose != nil {
	656	+	wv.OnClose()
	657	+	}
	658	+	}
	659	+
	660	+	// DisconnectSignals disconnects all GLib signal handlers from the WebView.
	661	+	// This must be called before releasing the WebView to the pool or destroying it
	662	+	// to prevent callbacks from firing on freed/pooled WebViews.
	663	+	func (wv *WebView) DisconnectSignals() {
	664	+	if wv.inner == nil {
	665	+	return
	666	+	}
	667	+
	668	+	obj := gobject.ObjectNewFromInternalPtr(wv.inner.GoPointer())
	669	+	for _, sigID := range wv.signalIDs {
	670	+	gobject.SignalHandlerDisconnect(obj, sigID)
	671	+	}
	672	+	wv.signalIDs = wv.signalIDs[:0] // Clear the slice
	673	+
	674	+	wv.logger.Debug().Uint64("id", uint64(wv.id)).Msg("signals disconnected")
	675	+	}
	676	+
582	677		// Destroy cleans up the WebView resources.
583	678		func (wv *WebView) Destroy() {
584	679		if wv.destroyed.Swap(true) {
585	680		return // Already destroyed
586	681		}
587	682
	683	+	// Disconnect all signal handlers first
	684	+	wv.DisconnectSignals()
	685	+
588	686		// Unregister from global registry
589	687		globalRegistry.unregister(wv.id)
590	688
591		-	// Note: Signal disconnection and GTK widget destruction
592		-	// would happen here in a full implementation.
593		-	// For now, we rely on GTK's reference counting.
594		-
595	689		wv.logger.Debug().Uint64("id", uint64(wv.id)).Msg("webview destroyed")
596	690		}
597	691


@@ -106,6 +106,7 @@ type WebView struct {
106		OnClose func()	106		OnClose func()
107		OnCreate func(PopupRequest) *WebView // Return new WebView or nil to block popup	107		OnCreate func(PopupRequest) *WebView // Return new WebView or nil to block popup
108		OnReadyToShow func() // Called when popup is ready to display	108		OnReadyToShow func() // Called when popup is ready to display
			109	+	OnLinkMiddleClick func(uri string) bool // Return true if handled (blocks navigation)
109			110
110		logger zerolog.Logger	111		logger zerolog.Logger
111		mu sync.RWMutex	112		mu sync.RWMutex

@@ -337,6 +338,72 @@ func (wv *WebView) connectSignals() {
337		}	338		}
338		sigID = gobject.SignalConnect(wv.inner.GoPointer(), "notify::estimated-load-progress", glib.NewCallback(&progressCb))	339		sigID = gobject.SignalConnect(wv.inner.GoPointer(), "notify::estimated-load-progress", glib.NewCallback(&progressCb))
339		wv.signalIDs = append(wv.signalIDs, sigID)	340		wv.signalIDs = append(wv.signalIDs, sigID)
			341	+
			342	+	// decide-policy signal for intercepting middle-click and Ctrl+click on links
			343	+	decidePolicyCb := func(inner webkit.WebView, decisionPtr uintptr, decisionType webkit.PolicyDecisionType) bool {
			344	+	// Only handle navigation actions (not new window or response policies)
			345	+	if decisionType != webkit.PolicyDecisionTypeNavigationActionValue {
			346	+	return false // Let WebKit handle
			347	+	}
			348	+
			349	+	// Cast to NavigationPolicyDecision
			350	+	navDecision := webkit.NavigationPolicyDecisionNewFromInternalPtr(decisionPtr)
			351	+	if navDecision == nil {
			352	+	return false
			353	+	}
			354	+
			355	+	navAction := navDecision.GetNavigationAction()
			356	+	if navAction == nil {
			357	+	return false
			358	+	}
			359	+
			360	+	// Only handle link clicks
			361	+	if navAction.GetNavigationType() != webkit.NavigationTypeLinkClickedValue {
			362	+	return false
			363	+	}
			364	+
			365	+	// Get mouse button and modifiers
			366	+	mouseButton := navAction.GetMouseButton()
			367	+	modifiers := navAction.GetModifiers()
			368	+
			369	+	// Check for middle-click (button 2) or Ctrl+left-click (button 1 + Ctrl)
			370	+	isMiddleClick := mouseButton == 2
			371	+	isCtrlClick := mouseButton == 1 && (gdk.ModifierType(modifiers)&gdk.ControlMaskValue) != 0
			372	+
			373	+	if !isMiddleClick && !isCtrlClick {
			374	+	return false // Normal click, let WebKit handle
			375	+	}
			376	+
			377	+	// Get the link URL
			378	+	request := navAction.GetRequest()
			379	+	if request == nil {
			380	+	return false
			381	+	}
			382	+
			383	+	linkURI := request.GetUri()
			384	+	if linkURI == "" {
			385	+	return false
			386	+	}
			387	+
			388	+	wv.logger.Debug().
			389	+	Str("uri", linkURI).
			390	+	Uint("button", mouseButton).
			391	+	Bool("ctrl", isCtrlClick).
			392	+	Msg("middle-click/ctrl+click on link detected")
			393	+
			394	+	// Call the handler
			395	+	if wv.OnLinkMiddleClick != nil {
			396	+	if wv.OnLinkMiddleClick(linkURI) {
			397	+	// Handler processed it, block the navigation
			398	+	navDecision.Ignore()
			399	+	return true
			400	+	}
			401	+	}
			402	+
			403	+	return false // Let WebKit handle if not handled
			404	+	}
			405	+	sigID = wv.inner.ConnectDecidePolicy(&decidePolicyCb)
			406	+	wv.signalIDs = append(wv.signalIDs, sigID)
340		}	407		}
341			408
342		// ID returns the unique identifier for this WebView.	409		// ID returns the unique identifier for this WebView.

@@ -579,19 +646,46 @@ func (wv *WebView) IsDestroyed() bool {
579		return wv.destroyed.Load()	646		return wv.destroyed.Load()
580		}	647		}
581			648
			649	+	// Close triggers the close callback as if window.close() was called.
			650	+	// This is used for programmatic popup closure (e.g., OAuth auto-close).
			651	+	func (wv *WebView) Close() {
			652	+	if wv.destroyed.Load() {
			653	+	return
			654	+	}
			655	+	if wv.OnClose != nil {
			656	+	wv.OnClose()
			657	+	}
			658	+	}
			659	+
			660	+	// DisconnectSignals disconnects all GLib signal handlers from the WebView.
			661	+	// This must be called before releasing the WebView to the pool or destroying it
			662	+	// to prevent callbacks from firing on freed/pooled WebViews.
			663	+	func (wv *WebView) DisconnectSignals() {
			664	+	if wv.inner == nil {
			665	+	return
			666	+	}
			667	+
			668	+	obj := gobject.ObjectNewFromInternalPtr(wv.inner.GoPointer())
			669	+	for _, sigID := range wv.signalIDs {
			670	+	gobject.SignalHandlerDisconnect(obj, sigID)
			671	+	}
			672	+	wv.signalIDs = wv.signalIDs[:0] // Clear the slice
			673	+
			674	+	wv.logger.Debug().Uint64("id", uint64(wv.id)).Msg("signals disconnected")
			675	+	}
			676	+
582		// Destroy cleans up the WebView resources.	677		// Destroy cleans up the WebView resources.
583		func (wv *WebView) Destroy() {	678		func (wv *WebView) Destroy() {
584		if wv.destroyed.Swap(true) {	679		if wv.destroyed.Swap(true) {
585		return // Already destroyed	680		return // Already destroyed
586		}	681		}
587			682
			683	+	// Disconnect all signal handlers first
			684	+	wv.DisconnectSignals()
			685	+
588		// Unregister from global registry	686		// Unregister from global registry
589		globalRegistry.unregister(wv.id)	687		globalRegistry.unregister(wv.id)
590			688
591	-	// Note: Signal disconnection and GTK widget destruction
592	-	// would happen here in a full implementation.
593	-	// For now, we rely on GTK's reference counting.
594	-
595		wv.logger.Debug().Uint64("id", uint64(wv.id)).Msg("webview destroyed")	689		wv.logger.Debug().Uint64("id", uint64(wv.id)).Msg("webview destroyed")
596		}	690		}
597			691