Skip to content

Fix: Unhandled completion handlers cause crashes when delegate is deallocated#233

Merged
svara merged 2 commits into
hotwired:mainfrom
noah44846:fix/webkit-completion-handler-nil-delegate
Jul 2, 2026
Merged

Fix: Unhandled completion handlers cause crashes when delegate is deallocated#233
svara merged 2 commits into
hotwired:mainfrom
noah44846:fix/webkit-completion-handler-nil-delegate

Conversation

@noah44846

Copy link
Copy Markdown

Hey! We're running a production app with quite a large user base and have been seeing a few rare but hard crashes traced back to these methods. After digging into the code I think I found the cause and put together a fix — happy to be corrected if I'm reading this wrong.

The issue

Several WKNavigationDelegate / WKUIDelegate methods pass their mandatory completion handlers through an optional delegate chain. If the delegate has been deallocated by the time WebKit fires the callback, the optional chain silently does nothing, the handler is never called, and WebKit raises NSInternalInconsistencyException — crashing the entire app.

Affected methods

ColdBootVisitdidReceiveAuthenticationChallenge

ColdBootVisit.delegate is a weak reference to Session. While Session strongly owns the WKWebView, but from what I gathered WebKit's network process can keep the web view alive independently during an in-flight request. If Session is torn down mid-cold-boot (e.g. during a navigator replacement), ColdBootVisit can outlive it just long enough for the challenge callback to arrive with a nil delegate.

Worth noting: this challenge apparently fires on the first HTTPS connection as part of standard TLS server trust evaluation, not just pages with HTTP auth (as I originally thought) — so the race can happen on any cold boot.

WKUIControllerrunJavaScriptConfirmPanelWithMessage and runJavaScriptAlertPanelWithMessage

WKUIController is strongly owned by Navigator via webkitUIDelegate, but WKWebView.uiDelegate holds it weakly. If Navigator is torn down while WebKit is simultaneously invoking one of these methods (WebKit temporarily retains the delegate during the call), WKUIController.delegate is nil, the alert is never presented, its actions never fire, and the handler is never called.

Crash reports

ColdBootVisit

OS Version:     iOS 26.3 (23D127)
Hardware Model: iPhone17,1

Exception Type:     EXC_CRASH (SIGABRT)
Termination Reason: SIGNAL 6

NSInternalInconsistencyException:
Completion handler passed to -[HotwireNative.ColdBootVisit
webView:didReceiveAuthenticationChallenge:completionHandler:] was not called

Thread 0 Crashed:
0   CoreFoundation       objc_exception_throw
1   libobjc.A.dylib      objc_exception_throw + 88
2   CoreFoundation
3   WebKit
...
7   libsystem_blocks     _Block_release + 236   ← handler released uncalled

WKUIController

OS Version:     iOS 26.3 (23D127)
Hardware Model: iPhone15,4

Exception Type:     EXC_CRASH (SIGABRT)
Termination Reason: SIGNAL 6

NSInternalInconsistencyException:
Completion handler passed to -[HotwireNative.WKUIController
webView:runJavaScriptConfirmPanelWithMessage:initiatedByFrame:completionHandler:] was not called

Thread 0 Crashed:
0   CoreFoundation       objc_exception_throw
1   libobjc.A.dylib      objc_exception_throw + 88
2   CoreFoundation
3   WebKit
...
7   libsystem_blocks     _Block_release + 236   ← handler released uncalled
8   clubcorner
9   libswiftCore.dylib

The fix

The pattern is the same in all cases: guard on delegate and call the handler with a safe default if it's nil. For the auth challenge that's .performDefaultHandling, for the confirm panel false (mirrors the user cancelling), and for the alert panel just calling through. I also proactively fixed runJavaScriptAlertPanelWithMessage since it has the same pattern, even though we haven't seen it crash yet.

@noah44846

noah44846 commented Mar 11, 2026

Copy link
Copy Markdown
Author

Follow-up: We observed another crash where a user tapped a push notification while a JavaScript confirm dialog was visible. In our app, tapping a notification can replace the active navigator, which causes the previous one and its presented alert to disappear without any action being triggered — so the completion handler is never called.

One option we considered would be to store a reference to the pending completion handler on WKUIController and call it in deinit when the navigator is torn down. That would cover this specific case, but it wouldn't handle situations where the alert is dismissed while the navigator stays alive.

A more defensive fix would be a small private UIAlertController subclass that calls the completion handler with a default value in viewDidDisappear if no action was taken — covering all dismissal scenarios regardless of cause. 7e5945a is just a how we solved the Issue internally.

@joemasilotti

Copy link
Copy Markdown
Member

Thanks for this! Can you explain the flow in your app, or perhaps share code, where this occurs:

if Session is torn down mid-cold-boot (e.g. during a navigator replacement)

I'm not sure I follow how that could be triggered but would like to know more.

@noah44846
noah44846 force-pushed the fix/webkit-completion-handler-nil-delegate branch from 7e5945a to 9f1a3a0 Compare March 23, 2026 11:30
@noah44846

noah44846 commented Mar 23, 2026

Copy link
Copy Markdown
Author

Thanks for this! Can you explain the flow in your app, or perhaps share code, where this occurs:

if Session is torn down mid-cold-boot (e.g. during a navigator replacement)

I'm not sure I follow how that could be triggered but would like to know more.

Thanks for the question! I actually just spent some time trying to reproduce this and trace the exact flow, and realized it's much trickier than expected. Here's the full picture:

The didReceiveAuthenticationChallenge fix is purely defensive. We couldn't reliably reproduce the crash — it happened once across a few thousand daily active users, and the timing window is extremely narrow.

From the crash report (stats provided by Rollbar):

active_time_since_launch: 0.816
background_time_since_launch: 1.02
sessions_since_launch: 3

sessions_since_launch is Rollbar's count of foreground/background transitions since the app was launched — so 3 means the app went through multiple foreground/background cycles in under 2 seconds total, which is consistent with the app being opened via a push notification.

The app had been active for less than a second, which points to the crash happening almost immediately after launch. In our app, we create a new Navigator for different screen modes (e.g. login vs. tabbed main app). If a notification arrives while the initial cold boot visit is still loading, we end up replacing the navigator — which deallocates the Session while the TLS handshake is still in-flight in WebKit's network process. When the didReceiveAuthenticationChallenge callback is eventually delivered, the weak delegate is nil and the completion handler is never called.

After digging into it, I'm fairly confident this is related to our specific implementation — we're tearing down a navigator at the wrong time. But the consequence (a hard crash from WebKit) seems disproportionate, and it's a tricky race to guard against from the app side since the timing depends on the network process. The fix just calls .performDefaultHandling when the delegate is nil, which can't change behavior for any case where the delegate is alive. That said, the delegate nil guards in both didReceiveAuthenticationChallenge and the WKUIController methods are defensive for our use case — of course it's entirely up to you whether you think they're appropriate fixes.

The runJavaScriptConfirmPanelWithMessage fix is a different story — this one addresses 4 confirmed crashes and is relevant in any situation where the alert gets dismissed without any UIAlertAction being triggered. From the crash reports:

Crash active_time_since_launch sessions_since_launch application_active
1 254s 3 1
2 70s 2 1
3 152s 5 0 (backgrounded)
4 351s 3 1

These all happened after the user had been active for minutes with multiple foreground/background transitions. The pattern in our case is: user taps a button that triggers a JS confirm() dialog, then a push notification arrives and they tap it, which replaces the navigator — the alert gets dismissed without any UIAlertAction firing, so the completion handler is never called. But this could happen in any scenario where something externally dismisses the alert (programmatic navigation, view controller replacement, etc.) — it's not specific to our implementation.

We initially tried calling the completion handler in viewDidDisappear, but discovered that it fires before the UIAlertAction handler when a user taps a button normally — meaning every confirm would return false regardless of what the user chose. Instead, we went with private UIAlertController subclasses (SafeAlertController / SafeConfirmAlertController) that call the completion handler with a safe default in deinit. The action closures capture [weak alert] and nil out the callback after invocation, so deinit only acts as a fallback when no action was triggered. This covers all external dismissal scenarios without interfering with normal user interaction.

The new commit is at 9f1a3a0.

@joemasilotti

Copy link
Copy Markdown
Member

Thanks! I think we want the first change in Hotwire Native. But I'm not convinced the second should be upstreamed. Said another way, I want to see the guard delegate in there but not anything else. You can subclass WKUIController in your application code if need be.

Is it possible for you to tease out the original commit so I can test and hopefully merge?

@noah44846
noah44846 force-pushed the fix/webkit-completion-handler-nil-delegate branch from 9f1a3a0 to 7f198fd Compare May 20, 2026 11:28
Guard against nil delegate in ColdBootVisit's authentication challenge
handler and WKUIController's alert/confirm panel handlers. When the
delegate is deallocated before WebKit fires the callback, the completion
handler was silently dropped, causing an NSInternalInconsistencyException
crash on the main thread.
@noah44846

Copy link
Copy Markdown
Author

@joemasilotti hey!

I agree subclassing UIAlertController isn't the right approach upstream so I rolled back the last commit.

I filed #239 with a clean repro for the separate JS-dialog crash (push-notification deeplink → presenter torn down while confirm() alert is up → completion handler dropped → next WebKit interaction crashes). Includes a demo branch you can run.

Leaving this PR scoped to the didReceiveAuthenticationChallenge nil-guard so it can merge cleanly — happy to move discussion of the dialog fix to the new issue :)

@svara

svara commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@noah44846 Thanks for the clear write up and reproduction steps. The .performDefaultHandling is the right default (it matches what NavigatorDelegate's default extension already does).

One gap before merge: the completion handler is forwarded through three weak delegate hops, and this PR only guards the leaf. The same crash can still fire at the two upstream hops:

ColdBootVisitSession ✅ guarded here
Session.visit(_:didReceiveAuthenticationChallenge:)Navigator Session.swift:221
Navigator.session(_:didReceiveAuthenticationChallenge:) → app delegate Navigator.swift:224
Both are weak. The Navigator hop is the most reachable: delegate ?? navigatorDelegate only runs at init, so once an app-assigned delegate deallocates, delegate is nil, same crash as your report.

Could you apply the same guard to both?

The dialog crash is a bit more complex, please open a separate issue for that.

@svara svara self-assigned this Jul 1, 2026
The completion handler is forwarded through three weak-delegate hops. The
leaf (ColdBootVisit) already falls back to .performDefaultHandling when its
delegate is nil, but Session and Navigator still forwarded through an
optional delegate, silently dropping the handler if that link was nil and
crashing WebKit with NSInternalInconsistencyException.

Guard both upstream hops with the same .performDefaultHandling fallback,
consistent with the NavigatorDelegate default implementation.

@svara svara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @noah44846!
I went ahead and fixed the two upstream hops too, so the whole delegate chain is covered now.

@svara
svara merged commit 6689b1f into hotwired:main Jul 2, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants