Fix: Unhandled completion handlers cause crashes when delegate is deallocated#233
Fix: Unhandled completion handlers cause crashes when delegate is deallocated#233noah44846 wants to merge 2 commits intohotwired:mainfrom
Conversation
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.
|
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. |
|
Thanks for this! Can you explain the flow in your app, or perhaps share code, where this occurs:
I'm not sure I follow how that could be triggered but would like to know more. |
7e5945a to
9f1a3a0
Compare
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 From the crash report (stats provided by Rollbar):
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 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 The
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 We initially tried calling the completion handler in The new commit is at 9f1a3a0. |
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/WKUIDelegatemethods 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 raisesNSInternalInconsistencyException— crashing the entire app.Affected methods
ColdBootVisit—didReceiveAuthenticationChallengeColdBootVisit.delegateis a weak reference toSession. WhileSessionstrongly owns theWKWebView, but from what I gathered WebKit's network process can keep the web view alive independently during an in-flight request. IfSessionis torn down mid-cold-boot (e.g. during a navigator replacement),ColdBootVisitcan 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.
WKUIController—runJavaScriptConfirmPanelWithMessageandrunJavaScriptAlertPanelWithMessageWKUIControlleris strongly owned byNavigatorviawebkitUIDelegate, butWKWebView.uiDelegateholds it weakly. IfNavigatoris torn down while WebKit is simultaneously invoking one of these methods (WebKit temporarily retains the delegate during the call),WKUIController.delegateis nil, the alert is never presented, its actions never fire, and the handler is never called.Crash reports
ColdBootVisitWKUIControllerThe fix
The pattern is the same in all cases: guard on
delegateand call the handler with a safe default if it's nil. For the auth challenge that's.performDefaultHandling, for the confirm panelfalse(mirrors the user cancelling), and for the alert panel just calling through. I also proactively fixedrunJavaScriptAlertPanelWithMessagesince it has the same pattern, even though we haven't seen it crash yet.