fix(bindings): restore the eight iOS methods React Native could not reach - #416
Merged
Conversation
…each `RCT_EXTERN_METHOD` does not declare a Swift method, it records a selector React Native resolves against the class at module load. One it cannot find is dropped with a log line and the JS method is simply absent. Neither compiler sees both halves of the bridge, and `OfflineProtocolModule.swift` is the one bridge source no CI job compiles, so three separate drifts shipped: - `wipePersistedState` kept the pre-rename `userId:` label and had been uncallable since 0.21.0, so logging out could not erase the account it had just signed out of. - `setBatteryState`, `getIsCharging`, `updateRelayConfig` and `getRelayConfig` were written in Swift, Kotlin and TypeScript and never declared in the shim, so since 0.22.0 every relay setting an application passed to `create()` was discarded on iOS behind a `console.warn`. - `dataListSpaces`, `dataFlushAll` and `dataWipeAll` took a labelled first parameter, which Swift exports as `dataListSpacesWithResolver:` rather than `dataListSpaces:`, and stopped resolving in 0.23.0. These are fixed in Swift rather than in the shim: React Native names the JS method after the selector text before its first colon, so writing the `With` form in the shim would rename the JS method instead of repairing it. Android was never affected; its dispatch is by method name and position. `react_native_ios_objc_shim_and_swift_agree_on_every_selector` now reads both bridge halves plus `src/index.ts` and compares them as sets, in all three directions. The set of Swift methods it holds to this is derived rather than listed: an `@objc` method is one React Native exports exactly when it takes the promise pair, so a new bridge method is inside the invariant the moment it is written.
…selectors The selector fix in the parent commit restored eight methods React Native could not reach. Extending its guard to compare parameter types found seven more that resolved and then ran on the wrong bits. `RCT_EXTERN_METHOD` stringifies its argument. React Native parses that text at module load, picks the `RCTConvert` converter from the type it reads there, and picks the calling convention from the Swift parameter's runtime encoding, then calls the one through a function pointer cast to the other. Pair `nonnull NSNumber *` with a Swift `Int` and `+[RCTConvert NSNumber:]` returns an object pointer that is then read as a 64-bit integer, so the method runs on the pointer bits of a tagged `NSNumber` and never on the number. Pair it with a `Double` and an integer register is read as a floating-point one. The selector still resolves, the method still runs, and nothing is logged. The type table in `BRIDGE_MAINTENANCE.md` had mapped `Int` to `nonnull NSNumber *` since v0.3.3, the release that also introduced the first method to follow it, so every method that took a number followed it too: - `sendMessage`, `sendMessageRich`, `sendPresenceUpdate`: every priority and status fell through to the `default:` arm, so an app's choice was discarded; - `setBatteryLevel`, `setBatteryState`: the level clamped to a bound rather than recording the battery, which is an input to relay eligibility; - `processFileChunk`, `blePeerDiscovered`: the value reached a narrowing conversion that traps, aborting the application. Fixed in the shim rather than in Swift: the selector is unchanged either way, and this direction leaves all seven Swift bodies alone. The two conversions that now receive real values are made safe for them, since out-of-range input from JavaScript is a caller mistake and belongs in a rejected promise rather than in an abort. The guard gains a third direction, comparing the ABI class of every parameter behind a shared selector. Both classifiers are exhaustive over the vocabulary the bridge uses and refuse anything else rather than guessing, so a new type has to be classified deliberately: a wrong equivalence here is invisible in exactly the way the guard exists to prevent. Verified: the type direction was written first and run against the unfixed tree, where it named all nine mismatched parameters. Two mutations, each restored and checksum-verified: reverting one type fires the ABI direction naming it, and an unclassified type fires the refusal. `cargo fmt`, `clippy`, `cargo test --workspace` and rustdoc are clean; the full hand-written iOS source set typechecks (exit 0) under the `BRIDGE_MAINTENANCE.md` recipe, itself negative-controlled first, and `swift test` passes 252 tests.
The selector guard reads three files and compares them in three directions. Two of those directions assert up front that their parser actually found something, on the grounds that two empty sets agree with each other perfectly and prove absolutely nothing. The third direction doesn't, and it is the one most likely to need it. The TypeScript scan hunts for a single identifier string, `OfflineProtocolNativeModule.`. Rename that binding, destructure it, or move the package to a TurboModule spec, and the scan matches nothing at all. No matches means no findings, no findings means the assertion passes, and a guard that passes because it read an empty set is worse than no guard, because by then you trust it. So count the calls and require a plausible number of them, the same way the other two directions do. There are 167 today. Checked the same way the rest of this guard was: renaming the identifier in index.ts makes it fail with "only found 0 native-module calls in bindings/react-native/src/index.ts", which is the sentence you want to be reading at that point.
…byte `UInt8(_:)` traps. It does not return nil, it does not throw, it does not truncate: it kills the process. Thirteen conversions in the iOS bridge used it on numbers that came straight from JavaScript. Twelve of them turn an array argument into bytes, so any element outside 0...255 aborts the app. That covers a BLE fragment, a Wi-Fi Direct or internet frame, an MLS ciphertext, a Welcome, a key package and a file chunk, which means most of them are reachable by a peer sending us something malformed rather than only by our own caller being careless. The thirteenth is `initialTtl`, typed as an unbounded `number` in TypeScript, so an application passing 300 to `create()` takes the app down on iOS while Android quietly truncates it and starts. Nobody notices until someone's field device starts crashing on boot and you get to explain why. This is not great. Route the arrays through a helper that throws instead, landing in the rejection every one of these call sites already had, and clamp `initialTtl` the way every other numeric config field in that file is already clamped. An out-of-range value from JavaScript is a caller mistake, and a caller mistake belongs in a rejected promise, not in a crash report. Worth being clear that these were never hiding behind the argument ABI bug fixed earlier on this branch. Array arguments cross as `NSArray *` against `[NSNumber]`, which has agreed since the UniFFI migration, so every one of these has been live in every release that shipped the method. Pin it with a guard that fails on any `UInt8` conversion in that file whose argument doesn't carry its own bound. It has to read text rather than count call sites, because the thing that makes a conversion safe is local to where it's written. The scalar narrowings in `processFileChunk` are bounded by a `guard` several lines above instead, which no textual rule can see, so they stay held by that clause and by review. While at it, document the one case that can't be fixed here. `forwardMessage` takes an optional priority, React Native forces every `NSNumber` argument to non-null because Android can't express a nullable number, and it drops the call before the Swift method is entered. Neither the resolver nor the rejecter runs, so the promise never settles and the caller waits forever. No spelling of the declaration fixes that one; it needs a contract change in three languages. See #417 for the contract change that would actually fix it.
The previous commit swept this bridge for conversions that trap on JavaScript input, fixed thirteen of them, and added a guard that fails on any `UInt8(...)` whose argument is not bounded where it is written. It missed two, and the guard could not have caught them, because they are not byte conversions. Both DORS config paths clamp `historyWindowSize` with `max(1, min(100, Int(raw)))`, which reads as bounded and is not: the narrowing runs *first*. `raw` is a `UInt64` straight off `NSNumber.uint64Value`, and a negative JavaScript number lands there as `UInt64.max`, so the `Int` conversion traps and aborts the process before the clamp around it ever runs. A `historyWindowSize` of -1 kills the app through `create()`, and again through `updateDorsConfig`. So clamp in the domain the value arrives in. `historyWindow` is only ever consumed as a `UInt64`, so dropping the round-trip through `Int` deletes the trapping conversion outright instead of making a pointless one safe, and the two `UInt64(historyWindow)` call sites become identity conversions and go with it. The symlink-farm typecheck agrees it now infers as `UInt64`; the negative control proved the harness by complaining about exactly that type. No new guard for this one. Whether `Int(raw)` is safe depends on what `raw` already is, which the text cannot say, and this file is full of legitimate widenings like `Int(hops)` that a textual rule would flag. Review and the checklist hold it instead, and both bridge docs now say that a clamp wrapped *around* a narrowing conversion does not count. While at it, two comments on the new guards were simply wrong. The byte guard's empty-argument skip claimed to exempt `[UInt8](...)`, which never reaches it at all because the `]` stops it matching the search; what it actually exempts is the zero-argument `UInt8()`. And the ABI classifier claimed `Int?` boxes into an `NSNumber`. It cannot: `@objc` refuses an optional value-type parameter outright, so only reference types ever reach that branch.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What was broken
RCT_EXTERN_METHODdoes not declare a Swift method. It records a selector string that React Native resolves against the class at module load, inparseExportedMethods. A selector no Swift method implements is dropped there behind anRCTLogWarnand the JS method is simply absent.Neither half's compiler sees the other: the
.mcompiles standalone against the macro, andOfflineProtocolModule.swiftis the one bridge source no CI job compiles at all (it needs real React headers). So three separate drifts shipped, costing eight methods across 0.21.0 to 0.24.0:wipePersistedStateuserId:vsprofile:)undefined is not a functionsetBatteryState,getIsCharging,updateRelayConfig,getRelayConfigcreate()swallows the failure in aconsole.warndataListSpaces,dataFlushAll,dataWipeAllThe reported issue was
wipePersistedStateonly. The other seven were found by the guard added here.Android was never affected. Its dispatch is by method name and position; Kotlin parameter names never participate, and the Kotlin side was correct throughout.
Behaviour change worth calling out to app teams
updateRelayConfig/getRelayConfigbeing absent meant that since 0.22.0 every relay setting an application passed tocreate()was discarded on iOS, behind aconsole.warn. Apps that setallowRelay,minBatteryForRelayorrelayPrioritywill see those settings take effect on iOS for the first time on the release carrying this fix. Same for the battery feed, which had no iOS writer.Why the three data methods are fixed in Swift, not in the shim
Swift exports
f(resolver:rejecter:)asfWithResolver:rejecter:, notf:rejecter:(verified empirically againstswiftc, not from memory). React Native names the JS method after the selector text before its first colon, so spellingdataListSpacesWithResolver:in the shim would have renamed the JS method rather than repairing it. Dropping the label in Swift is the only fix that restoresdataListSpacesas a callable JS name, and it matches the other 162 methods.The guard
react_native_ios_objc_shim_and_swift_agree_on_every_selectorreadsOfflineProtocolModule.m,OfflineProtocolModule.swiftandsrc/index.tsand compares them as sets in three directions:This is C5's mechanism applied to a selector table rather than a constant, and it is a Rust guard for C5's stated reason: for sources CI typechecks at most and never runs, a source-reading guard is the only reachable pin.
The set of Swift methods held to this is derived, not listed: an
@objcmethod is one React Native exports exactly when it takes the promise pair. That excludes the twoNotificationCentertargets and RN's ownaddListener/removeListenersoverrides without naming them, and puts a new bridge method inside the invariant the moment it is written.Verification
git checkout, which would have eaten the uncommitted fix):RCT_EXTERN_METHOD→ direction 2 fires naminggetIsCharging:rejecter:;cargo fmt --all -- --check,cargo clippy --workspace -- -D warnings,cargo test --workspace(27 suites),RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps: all clean.BRIDGE_MAINTENANCE.mdsymlink-farm recipe (swiftc -typecheck, exit 0), and that harness was itself negative-controlled with a deliberate bad type before the clean run was trusted.swift testinios/: 252 tests pass.Docs
docs/bridges/swift.mdS1 gains the label-agreement and unlabelled-first-parameter rules;docs/bridges/README.mdC5 gains the selector table as its seventh entry;BRIDGE_MAINTENANCE.mdgains a step and a Common Issues entry with the command to run.Review round 2
Three findings from review, all addressed.
The guard could pass while checking nothing
Directions 1 and 2 each assert their parser found at least 150 methods, on the grounds that two empty sets agree perfectly. Direction 3 had no such floor, and it is the one that needs it most: the TypeScript scan keys off the literal
OfflineProtocolNativeModule., so renaming that binding, destructuring it, or moving to a TurboModule spec would match nothing, find nothing, and pass. It now counts calls and requires 150 (there are 167). Mutation-tested: renaming the identifier fails withonly found 0 native-module calls.Thirteen conversions abort the app instead of rejecting
Review flagged
processFileChunk'sdata.map { UInt8($0.intValue) }. Grepping the class found thirteen, not one:initialTtlcreate(), for any app passing anumberabove 255UInt8(_:)traps: it aborts the process rather than returning something the bridge could reject. Arrays now convert through a throwingjsByteshelper that lands in the rejection every one of these call sites already had;initialTtlis clamped, matching Android, which truncates throughtoUByte()and starts normally where iOS crashed.These were never masked by the ABI bug above. Array arguments cross as
NSArray *against[NSNumber], which has agreed since the UniFFI migration, so each has been live in every release that shipped the method.Pinned by
react_native_ios_bridge_bounds_every_byte_it_builds_from_javascript, which fails on anyUInt8(...)in the module whose argument does not carry its own bound (exactly:,clamping:,min(,uint8Value, a mask or a shift). It reads text because what makes a conversion safe is local to where it is written;processFileChunk's scalar narrowings are bounded by aguardseveral lines above instead, which no textual rule can see. Mutation-tested: restoring one trapping conversion fails naming$0.intValue.forwardMessage is now tracked, not just described
Its optional priority cannot cross the bridge at all: React Native forces every
NSNumberargument to non-null (Android cannot express a nullable number), so a null one is refused inRCTModuleMethod.mmbeforeinvokeWithTarget:runs. Neither resolver nor rejecter fires and the promise never settles. It is#if RCT_DEBUG-only, so release builds are fine. No declaration fixes it; filed as #417 with the three candidate contract changes, and linked fromBRIDGE_MAINTENANCE.mdanddocs/bridges/swift.md.Verification
Both new guards mutation-tested and restored checksum-verified.
cargo fmt --all -- --check,cargo clippy --workspace -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsall clean. The full hand-written iOS source set typechecks (swiftc -typecheck, exit 0) under theBRIDGE_MAINTENANCE.mdrecipe, negative-controlled first;swift test252 pass;tsc --noEmitclean. Still no UDL change, so no binding regeneration.Review round 3
One finding, plus two comments of my own that were wrong.
The same bug class survived round 2, in a conversion that is not a byte
Round 2 swept the module for trapping conversions and pinned the result with a guard that reads every
UInt8(...). Both DORS config paths were outside that net, because their conversion is anInt:That reads as bounded and is not, because the narrowing runs first.
historyWindowRawis aUInt64, a negative JavaScript number reachesuint64ValueasUInt64.maxby C conversion, andInt(UInt64.max)traps. Socreate({dors: {historyWindowSize: -1}})aborted the app, and so did the same field throughupdateDorsConfig. A value like1e20saturates to the same place.Fixed by clamping in the domain the value arrives in rather than by making the narrowing safe:
historyWindowis only ever consumed as aUInt64, so the round-trip throughIntis deleted outright and the twoUInt64(historyWindow)call sites become identity conversions and go with it. Nothing narrows any more, so there is nothing left to get wrong.No new guard, deliberately. Whether
Int(raw)is safe depends on whatrawalready is, which the text cannot say, and the module is full of legitimate widenings (Int(hops),Int(progress.chunksSent)) that a textual rule would flag. This is theprocessFileChunksituation again: held by review and the checklist. Both bridge docs now carry the rule that a clamp wrapped around a narrowing conversion does not count, which is the part that generalises.I re-swept every other narrowing in the file while I was here. The rest are clean: the DORS
UInt64(...uint64Value)forms are identity conversions of non-trapping accessors, the data-layer calls useUInt32(truncating:),UInt16(finalPort)sits behind an explicit0...65535guard, and the remainingInt(...)sites convert engine outputs rather than JavaScript input.Two comments on the new guards were wrong
[UInt8](...). It never reaches that check at all: the]before the paren meansUInt8(does not match in the first place. What the skip actually exempts is the zero-argumentUInt8().Int?boxes into anNSNumber. It cannot.@objcrefuses a method outright when a parameter is an optional value type, so only reference types ever reach that branch.Neither affected behaviour, but a guard that explains itself wrongly is a guard someone edits wrongly later.
Verification
cargo fmt --all -- --check,cargo clippy --workspace -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsall clean. The full hand-written iOS source set typechecks under theBRIDGE_MAINTENANCE.mdsymlink-farm recipe (swiftc -typecheck, exit 0), negative-controlled first, and the negative control doubles as proof of the fix's key assumption: it failed withcannot convert value of type 'UInt64' to specified type 'String', which is the compiler confirminghistoryWindownow infers asUInt64. The injected error was removed and the file checksum-verified back to its pre-injection state. Still no UDL change, so no binding regeneration.Downstream: OFF-2462 closes once this ships and the companion app bumps its pin.