Fix crash on Cmd-G in the HTML output window - #44
Merged
Conversation
Cmd-G and Cmd-Shift-G in the HTML output window killed TextMate with EXC_BAD_ACCESS at address 0x10, on the main thread, with no TextMate frames below main: the fault lands on the async IPC reply from the WebContent process, long after the call site returned. -findNext: and -findPrevious: passed completionHandler:nil to -[WKWebView findString:withConfiguration:completionHandler:]. That parameter is implicitly nonnull — WKWebView.h:510 declares `configuration` as nullable and the handler not, inside NS_ASSUME_NONNULL_BEGIN/END — and WebKit invokes it without a null check, so 0x10 is the invoke slot of a nil Block_layout. Passing nil is a caller-side contract violation; there is nothing to fix in WebKit. The OakNotEmptyString guard above each call is what made the crash delayed rather than immediate. A non-empty string takes the async path; an empty one would fault synchronously, because the !string.length early return invokes the handler just as unguarded. -performFindOperation: already passed a real block, so the find dialog was never affected. Only the two keyboard shortcuts. An empty block restores the behaviour these two actions had under WebKit1, where -searchFor:direction:caseSensitive:wrap: returned a BOOL that findNext: also discarded. Reporting the result the way performFindOperation: does — so that a failed Cmd-G beeps — is a separate question, deliberately left out of a crash fix. Verified with a minimal WKWebView harness rather than by inspection: calling findString: with a nil handler exits 139 on SIGSEGV, and with an empty block exits 0. Under lldb the nil case faults in CallableWrapper<-[WKWebView findString:withConfiguration:completionHandler:] ::$_24, void, bool>::call(bool) + 56, above makeAsyncReplyCompletionHandler, AuxiliaryProcessProxy::sendMessage and IPC::Connection::dispatchMessage — matching the reported crash frame for frame. Claude-Session: https://claude.ai/code/session_01Gi3HR9ioH4wfrn4BWvJUDA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cmd-G/Cmd-Shift-Gin the HTML output window killed TextMate:No TextMate frames below
main— the fault lands on the async IPC reply from the WebContent process, long after the call site returned, which is why the stack alone doesn't name the culprit.Cause
-findNext:and-findPrevious:passedcompletionHandler:nil:SDK
WKWebView.h:510declaresconfigurationasnullableand the handler not, insideNS_ASSUME_NONNULL_BEGIN/END(lines 34/768), so the handler is implicitly nonnull. WebKit invokes it without a null check, and0x10is theinvokeslot of a nilBlock_layout(isa8 +flags4 +reserved4). Passing nil is a caller-side contract violation; there is nothing to fix in WebKit.The
OakNotEmptyStringguard above each call is what made this delayed rather than immediate — a non-empty string takes the async path. An empty one would fault synchronously, because the!string.lengthearly return invokes the handler just as unguarded.-performFindOperation:already passed a real block, so the find dialog was never affected. Only the two keyboard shortcuts.Both
v2.1.4-undead(the build that crashed) andv2.1.5-undeadcontain these two lines.The compiler was already saying so
Building the unfixed code emits, at exactly the two call sites:
Verification
A minimal WKWebView harness, rather than inspection:
nil^(WKFindResult*){ }Under lldb the nil case gives
EXC_BAD_ACCESS (code=1, address=0x10)inCallableWrapper<-[WKWebView findString:withConfiguration:completionHandler:]::$_24, void, bool>::call(bool) + 56, abovemakeAsyncReplyCompletionHandler,AuxiliaryProcessProxy::sendMessageandIPC::Connection::dispatchMessage— matching the reported crash frame for frame. The lambda ordinal differs ($_24vs$_28) only because the test ran on macOS 26.5 against a 26.3 dump.Clean build of the tree; full CTest run unchanged.
Why an empty block
It restores what these two actions did under WebKit1, where
-searchFor:direction:caseSensitive:wrap:returned aBOOLthatfindNext:also discarded. Reporting the result the wayperformFindOperation:does — so a failedCmd-Gbeeps instead of doing nothing visible — is a real improvement but a separate question, deliberately kept out of a crash fix.Follow-ups, not in this PR
-Wnonnullfatal. It found this bug for free, and after this change exactly one site in the tree stands in the way. That is more durable protection than a test, which would need GUI infrastructure this repo doesn't have.Frameworks/text/src/types.h:119is that site, and it's a latent bug rather than noise:range_t(0)has no matchingpos_tconstructor, so it resolves torange_t(std::string const&)and constructsstd::stringfrom a null pointer, which is undefined behaviour. Currently unreachable —selection_t(std::string const&)'s do-while always pushes one range, soempty()is never true there. It should almost certainly berange_t().