fix: close nine crash and concurrency paths found after the TestFlight reports - #1015
Merged
Conversation
On iPad UIKit presents `UIActivityViewController` as a popover, and a popover carrying neither `sourceView` nor `barButtonItem` throws `NSInvalidArgumentException` from `presentationTransitionWillBegin`. A TestFlight user on 9.0.0 (28) crashed there when tapping "share address"; iPhone never reaches it because the sheet is modal. The anchor goes into a `dw_presentActivityViewController` helper instead of the call site, because the support-log share had the same gap. Both callers are driven from SwiftUI and have no sender view to point at, so the helper anchors an arrow-less popover at the centre of the presenting controller's view by default, and takes an explicit view or rect when one exists.
Found by auditing the codebase after two TestFlight crash reports. Every one of them is a trap or an abort, so none can be caught by the `do`/`catch` the surrounding code already has. Explore database (`ExplorePointOfUse.init(row:)`), two of them. The merchant `type` column was resolved through `Merchant.Type(rawValue:)`, the one enum in that file without a catch-all, and the resulting `nil` was passed as a non-optional initializer argument — so a value the backend adds server-side, such as a new merchant category, aborts the app for everyone on the next database sync. `type` is now optional on `Merchant` and `Atm`: unknown stays unknown rather than being mapped onto an existing case. Separately, no text column in that schema is declared NOT NULL, and SQLite.swift's subscript for a non-optional `Expression` is `try! get(column)`, which aborts on NULL and on a column a synced schema no longer carries. `name`, `territory`, `type`, `source`, `merchantId`, `savingsPercentage` and `active` now read through `try? row.get`, which is what this initializer already did for `phone`, `logoLocation` and `coverImage`. NTP parsing (`TimeUtils`). The reply was indexed at offsets 40-43 with no length check. A datagram connection delivers whatever arrived — `minimumIncompleteLength` is not a floor for UDP — so a runt or rewritten packet, as a captive portal intercepting 123/udp produces, arrives with no error and traps. The packet is copied into an array so the offsets are positions within it rather than absolute `Data` indices, and short replies are discarded. Coinbase accounts (`AccountRepository.all`). `balance.amount` arrives as a string in the JSON response and was force-unwrapped through `Decimal(string:)`; anything unparseable now counts as no balance instead of taking down the accounts screen. CrowdNode navigation (`CrowdNodeWebViewController`). `replaceLast(3)` after a successful online-account link assumed the Getting Started route's stack depth. Entering from the home shortcut, where the portal is the navigator's root, leaves two controllers, and `removeLast(3)` traps. Both routes end at the portal with nothing behind it, so the call site sets the stack directly. `replaceLast` also clamps `n` to the stack depth, which closes the same mismatch in `NewAccountViewController`, reachable when `getRootVC()` opens on the new-account screen.
The same audit turned up four dangerous patterns whose reachability took longer to establish. All four ship in the current scheme. Token refresh. `CTXSpendTokenService` and `CBSecureTokenService` each stored the in-flight refresh in a plain property on a non-isolated class and then force-unwrapped it. Concurrent callers exist for both: `CTXSpendAPI` funnels every 401 into the first, and every Coinbase request reaches the second through `refreshTokenIfNeeded`. Two callers can each see no task in flight, start their own, and clobber the stored reference; a competing `defer` clearing it between the write and the force-unwrap is a crash. `PiggyCardsTokenService` already solved this with a private actor, so that actor is promoted to internal and reused by all three rather than copied. It now takes the service label for its log line and a throwing `Void` closure instead of the `Bool` contract that was specific to PiggyCards; the write-only `isRefreshing` flag is gone. Locale separator (`String.attributedAmountForLocalCurrency`). `locale.decimalSeparator!` sits on the amount-entry path, which `AmountInputControl.reloadData` runs on every keystroke. It now falls back to "." exactly as the sibling helper 48 lines above it does. Formatter singletons (`Tools`). `_fiatFormatter` and `_decimalFormatter` were built and replaced without synchronization, while `_cachedFormatters` beside them is lock-protected. `CoinJoinMixingTxSet.fiatAmount` reaches them from the background queue `HomeViewModel.reloadTxDataSource` runs on, concurrently with main- thread formatting everywhere else. They take a second lock rather than the existing one, because the `fiatFormatter` getter calls `fiatFormatter(currencyCode:)`, which takes the existing lock, and `NSLock` is not recursive. Migration versions (`DatabaseConnection`). The duplicate-version check was an `assert`, compiled out under `ENABLE_NS_ASSERTIONS = NO` in Release and TestFlight — the builds where a duplicate would actually ship. `schema_migrations.version` is UNIQUE, so the insert fails there regardless, but as an opaque constraint error. Throwing instead names the cause in the log `AppDelegate` already writes.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Comment |
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
3 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
Two crash reports came in from TestFlight 9.0.0 (builds 25 and 28):
NSInvalidArgumentExceptionfrom-[UIPopoverPresentationController presentationTransitionWillBegin];SIGABRTinside the Rust SDK — that one is fixed upstream in fix(dash-spv): resume on the invariant start_download asserts rust-dashcore#955 and is not part of this PR.Auditing the codebase for the same defect classes turned up five more crash paths reachable from ordinary use, plus four dangerous patterns whose reachability took longer to establish. None of them can be caught by a
do/catch: every one is a trap or an abort, not a thrown error.What was done?
Three commits, readable independently.
fix(payments)— the reported iPad crash. On iPadUIActivityViewControlleris always presented as a popover, and a popover with neithersourceViewnorbarButtonItemthrows when the presentation begins. The anchor lives in a newdw_presentActivityViewControllerhelper rather than at the call site, because the support-log share had the same gap; both callers come from SwiftUI and have no sender view, so the helper anchors an arrow-less popover at the centre by default and accepts an explicit view or rect when one exists.fix— five crash paths.ExplorePointOfUse.init(row:)Merchant.Type(rawValue:)is the one enum in the file without a catch-all; itsnilwas passed as a non-optional argument. A merchant category added server-side aborts the app for everyone on the next database sync.typeis now optional onMerchantandAtm— unknown stays unknown instead of being mapped onto an existing case.ExplorePointOfUse.init(row:)NOT NULL, and SQLite.swift's subscript for a non-optionalExpressionistry! get(column)— it aborts onNULLand on a column a synced schema no longer carries. The affected columns now read throughtry? row.get, as this initializer already did forphone,logoLocationandcoverImage.TimeUtilsminimumIncompleteLengthis not a floor for UDP, so a runt or rewritten datagram (a captive portal intercepting 123/udp) arrives with no error and traps. Reachable fromHomeView.onAppearon every visit.AccountRepository.allbalance.amountfrom the Coinbase JSON was force-unwrapped throughDecimal(string:); unparseable now counts as no balance instead of taking down the accounts screen.CrowdNodeWebViewControllerreplaceLast(3)assumed the Getting Started route's stack depth; entering from the home shortcut leaves two controllers andremoveLast(3)traps. The call site sets the stack directly, andreplaceLastclampsn, which also closes the same mismatch inNewAccountViewController.fix— token refresh and three latent traps.CTXSpendTokenServiceandCBSecureTokenServicestored the in-flight refreshTaskin a plain property on a non-isolated class and force-unwrapped it, with real concurrent callers on both sides (CTXSpendAPIfunnels every 401 into the first; every Coinbase request reaches the second throughrefreshTokenIfNeeded).PiggyCardsTokenServicealready solved this with a private actor, so that actor is promoted to internal and reused by all three rather than copied. Also:locale.decimalSeparator!on the amount-entry path now falls back to"."as the sibling helper in the same file does; the two formatter singletons inToolstake a lock, as the cache beside them already does; and the duplicate-migration-version check throws instead of asserting, so it survivesENABLE_NS_ASSERTIONS = NOin Release and TestFlight and names the cause in the log.How Has This Been Tested?
xcodebuild -workspace DashWallet.xcworkspace -scheme dashpay -sdk iphonesimulator -destination 'generic/platform=iOS Simulator' ARCHS=arm64 build— succeeds.URL(string:)behaviour behind a sixth reported candidate was checked against the current Foundation parser and did not reproduce, so that one is deliberately not changed.Worth a reviewer's eye: the
Merchant.type/Atm.typeoptionality ripples into eight comparison sites (m.type == .onlineand friends). They all keep compiling because comparing an optional against a case is still valid, and a row with an unknown type now reads as "not online" — the behaviour on merchants we can't classify is a product call, not a mechanical one.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only