fix(stability): close crash paths found in the 2026-08 crash audit - #910
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Static audit of DashWallet/Sources plus device diagnostics from two users
(testnet iOS 26.5.2 and mainnet iOS 27.0, both 9.0.0 build 13).
Nil / force-unwrap crashes on ordinary paths:
- `stopNetworkMonitoring()` force-unwrapped `reachabilityObserver`, which is
only assigned by `startNetworkMonitoring()`. Six adopters pair start in
`viewDidLoad` with stop in `deinit`, so a controller released before its
view loaded crashed. Now guarded and nilled out.
- Untyped `catch` blocks force-cast to `Coinbase.Error` / `CrowdNode.Error`;
any `URLError` / `CancellationError` from a flaky connection crashed the
Coinbase transfer and CrowdNode flows. Now `as?` with a fallback case.
- `fatalError` on decoded Coinbase amounts, exchange rates, account icon URLs
and Uphold amounts — all server- or user-supplied. They now return a
placeholder or a typed error.
- `CBAccount.send` / `UpholdAmountModel` treated an unbound SDK wallet as
unreachable; it is reachable during network and wallet switches.
- Gift-card amounts went through `UInt64(Double)`, which traps on negative or
non-finite input. Now range-checked via `Decimal`.
Home transaction list:
- `txItems` was read on the view model's worker queue while being written on
main. Every access is now on main, and `TransactionGroup` became a struct so
the array no longer runs ARC on shared class instances.
- Group indexes captured off-main were applied a main-queue turn later without
revalidation, trapping when a full reload replaced the array in between.
Launch and teardown:
- `[DWEnvironment sharedInstance]` materializes DashSync and reconciles the
whole legacy transaction set on main — 1-4s in the diagnostics, against the
launch watchdog budget. The DashSync-touching tail is deferred by one
main-queue turn. Every chain consumer moved with it: deferring
`DWEnvironment` alone let `DWPhoneWCSessionManager`'s background read win the
race and hit DSChain's main-thread assertion.
- `DWCaptureSessionManager` never tore down: teardown was scheduled only when
the session was already running, so the capture session, its device input
and three serial queues leaked for the process lifetime. Teardown is now
unconditional, clears both output delegates, and runs on main so the session
state is single-threaded. A nil capture device (simulator, or a device that
will not vend the camera) is handled instead of raising.
Other:
- `NumberFormatter.inputString(from:and:)` fell through `assertionFailure` to
`fatalError` in Release when the currency symbol could not be located in the
formatted string — locale-dependent, on the amount-keyboard hot path. It now
returns nil, and the separator index is taken after the string it indexes is
rebuilt.
- `ExplorePointOfUseListViewController` inserted rows from pager offsets rather
than the data source, raising NSInternalInconsistencyException when a filter
or search changed the model mid-request.
- `DatabaseConnection` built its store URL with `URL(string:)` on a filesystem
path and passed `absoluteString` to SQLite; `migrateIfNeeded` now reports a
failed open instead of unwrapping nil.
- The three `DispatchSemaphore` bridges into `Task { @mainactor }` refuse to
run on the main thread rather than deadlocking.
- Removed the `CJTEST` debug tags from shipping log lines (guardrail #6).
Not verified by a build: the SwiftDashSDK package in ../platform is on a
feature branch and does not compile, so the app target was never reached.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romchornyi
force-pushed
the
fix/crash-audit
branch
from
August 3, 2026 17:08
614fd56 to
29c4822
Compare
5 tasks
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
2 participants
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.
Issue being fixed or feature implemented
Users reported frequent crashes on 9.0.0 (13). A static audit of
DashWallet/Sources(684 Swift + 189 ObjC files) was cross-referenced against device diagnostics from two users:Device A's crashed run left one
Starting full transaction reloadwith no matchingFull reload complete(17 vs 16 over the file; the clean run is balanced 9/9), while the Rust/tokio side kept logging for a further 12 s — i.e. the app's Swift side stopped on its own insideHomeViewModel.reloadTxDataSource().Device B died while idle with the Rust side stopping simultaneously, so it is a second, resource-shaped signature rather than the same bug.
What was done?
Nil / force-unwrap on ordinary paths
stopNetworkMonitoring()force-unwrappedreachabilityObserver, only ever assigned bystartNetworkMonitoring(). Six adopters pair start inviewDidLoadwith stop indeinit, so a controller released before its view loaded crashed.catchblocks force-cast toCoinbase.Error/CrowdNode.Error— anyURLError/CancellationErrorfrom a flaky connection crashed the Coinbase transfer and CrowdNode flows.fatalErroron decoded Coinbase amounts, exchange rates, account icon URLs and Uphold amounts (all server- or user-supplied), and on an unbound SDK wallet inCBAccount.send/UpholdAmountModel— reachable during network and wallet switches.UInt64(Double), which traps on negative or non-finite input.Home transaction list (device A's crash)
txItemswas read on the view model's worker queue while being written on main. Every access is now on main, andTransactionGroupbecame a struct so the array no longer runs ARC on shared class instances.QR scanner teardown
DWCaptureSessionManagernever tore down — across all six device logs"Preview has been stopped"appears 4×,"Tearing down…"0×, because teardown was scheduled only when the session was already running. The capture session, its device input and three serial queues leaked for the process lifetime. Teardown is now unconditional, clears both output delegates, and runs on main so the session state is single-threaded; a nil capture device (simulator, or a device that will not vend the camera) is handled instead of raisingNSInvalidArgumentException.Other
NumberFormatter.inputString(from:and:)fell throughassertionFailuretofatalErrorin Release when the currency symbol could not be located in the formatted string — locale-dependent, on the amount-keyboard hot path.ExplorePointOfUseListViewControllerinserted rows from pager offsets rather than the data source, raisingNSInternalInconsistencyExceptionwhen a filter or search changed the model mid-request.DatabaseConnectionbuilt its store URL withURL(string:)on a filesystem path and passedabsoluteStringto SQLite.DispatchSemaphorebridges intoTask { @MainActor }now refuse to run on the main thread rather than deadlocking.CJTESTdebug tags from shipping log lines (guardrail Fix bitcode support in bls-signatures-pod #6).Dropped during rebase
The audit also found that
[DWEnvironment sharedInstance]materialized DashSync and reconciled the whole legacy transaction set on main duringdidFinishLaunching— 1–4 s in the diagnostics (~15kDSAccountlog lines per launch, 99.5 % of device B's log volume), against the launch watchdog budget.chore(dashsync): complete final unlinkon the base branch removed that call entirely, so the fix is no longer needed and was dropped when rebasing onto it.How Has This Been Tested?
Not built. The SwiftDashSDK package in
../platformis onfeat/platform-wallet-dashconnect-ffiand fails withvalue of type 'EventHandlerCallbacks' has no member 'release_fn', so the app target was never reached. Needs adashpayarm64-sim build against thev4.1-devpin plus a testnet smoke of Home, Coinbase transfer, CrowdNode, the QR scanner and the amount keyboard before merge.Verification so far is static: every changed call site was read against its callers, and the new enum cases (
Coinbase.Error.unknownError,CrowdNode.Error.messageStatus,UpholdRequestTransferModelState.fail) were confirmed to exist. The rebase onto the post-unlink base was resolved by hand:SwiftDashSDKWalletWiper.deleteWalletsFromSDKwas rewritten upstream to take[Network: Set<Data>], and the main-thread guard was reapplied to the new signature.Breaking Changes
None.
TransactionGroupchanges fromclasstostruct, but it is internal to the home list and no call site relied on reference semantics.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code