fix(MapView): don't leak unhandled rejections from native bridge calls - #4280
Open
giaBaoJS wants to merge 1 commit into
Open
fix(MapView): don't leak unhandled rejections from native bridge calls#4280giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
`_setHandledMapChangedEvents` discarded the promise returned by `_runNativeMethod`. When the native view is torn down while the call is in flight - navigating away from the map, or any unmount - the bridge rejects with `Unknown reactTag: <n>` and React Native escalates it to a `Possible Unhandled Promise Rejection`. It is called from both `componentDidMount` and `componentDidUpdate`, and the latter re-fires on every render that changes a callback prop identity, so apps using inline handlers report the warning in very high volume. Add `_runNativeMethodDetached` to `NativeBridgeComponent` for calls whose result is intentionally discarded, and route the three fire-and-forget call sites through it: `MapView._setHandledMapChangedEvents`, `MapView.setSourceVisibility` and `PointAnnotation.refresh`. It reports failures through `console.warn` rather than rethrowing - every caller has already returned, so a rethrow would only recreate the unhandled rejection. `runNativeMethod` throws synchronously when the view handle is already gone, which a bare `.catch()` would miss, so both paths are handled. `_runPendingNativeMethods` had the same problem: it is `async` and all three of its callers invoke it fire-and-forget, so a rejection while draining the queued mount-time call escaped the same way. Catching per item also stops one failure from abandoning the rest of the queue. Fixes rnmapbox#3492
giaBaoJS
requested a deployment
to
CI with Mapbox Tokens
August 14, 2026 02:52 — with
GitHub Actions
Waiting
giaBaoJS
requested a deployment
to
CI with Mapbox Tokens
August 14, 2026 02:52 — with
GitHub Actions
Waiting
giaBaoJS
requested a deployment
to
CI with Mapbox Tokens
August 14, 2026 02:52 — with
GitHub Actions
Waiting
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.
Description
Fixes #3492
MapView._setHandledMapChangedEventsdiscards the promise returned by_runNativeMethod(MapView.tsx#L675):_runNativeMethodreturnsPromise<ReturnType>(NativeBridgeComponent.tsx#L49) and no rejection handler is ever attached. It is called from bothcomponentDidMount(MapView.tsx:581) andcomponentDidUpdate(MapView.tsx:598). Tearing the native view down while a call is in flight — navigating away from the map — makes the bridge reject withUnknown reactTag: <n>, which React Native escalates toPossible Unhandled Promise Rejection.The
componentDidUpdatepath re-fires on every render that changes the identity of a callback prop, which is why apps using inline handlers see this at very high volume (the reporter measured Sentry going from ~400 errors/month to maxing out at 10,000/day).#4065 previously attacked the native-side retry timeout and was closed unmerged; it never touched the JS side. This is the JS half.
What changed
Added
_runNativeMethodDetachedtoNativeBridgeComponentfor calls whose result is intentionally discarded, and routed the three fire-and-forget call sites through it:MapView._setHandledMapChangedEvents(MapView.tsx:675)MapView.setSourceVisibility(MapView.tsx:940)PointAnnotation.refresh(PointAnnotation.tsx:212)Those last two are the same defect: both are declared to return
void, so the promise is created and dropped inside the library and callers have no way to attach a handler themselves.Two details worth flagging for review:
1. It also covers a synchronous throw.
runNativeMethodthrows synchronously rather than rejecting when the view handle is already gone (utils/index.ts#L61):That is the same race with a worse outcome — an uncaught exception out of
componentDidMountrather than a warning — and a bare.catch()would never be installed to catch it. Hence thetry/catcharound the.catch().2.
_runPendingNativeMethodsleaked the same way. It isasyncand all three of its callers (MapView.tsx:1202,PointAnnotation.tsx:217,ShapeSource.tsx:175) invoke it fire-and-forget, so a rejection while draining the queued mount-time call escaped identically. I only found this because the first version of the fix left it failing — see the fourth test below. Catching per item also stops one failure from abandoning the rest of the queue.Unconditional catch, or rethrow anything that isn't the unmount race?
I went with unconditional, because "rethrow" is not actually available here:
p.catch(e => { throw e })just produces another rejected promise nobody owns — it would recreate the exact bug for the non-matching case. "Selective rethrow" would mean "selectively keep the bug".Unknown reactTag, which is a React Native internal message, not a stable contract, and differs across platforms and versions. When such a match silently stops matching after an RN upgrade it fails in the bad direction — the rejection storm comes back.So instead of rethrowing, failures are reported through
console.warnwith the method name and the original error. That keeps a genuine wiring failure visible in development while removing the unhandled rejection — and, relevant to the reported symptom,console.warnis not captured by Sentry's default integrations, whereas unhandled rejections are.I used
console.warnrather thanLoggerdeliberately:src/utils/Logger.tsis a bridge for native Mapbox log events (its only message-emitting method is theonLogevent handler, andMapViewuses it solely via.start()/.stop()). Pushing a JS-originated message into that stream would change whatLogger.setLogCallbackconsumers receive.console.warnis what the rest of the codebase — including the six sibling warnings inside_setHandledMapChangedEventsitself — already uses.Happy to dial the log level down, drop it to silent for the known race, or split the extra two call sites into a separate PR if you'd prefer a narrower change.
Checklist
CONTRIBUTING.mdyarn generatein the root folder — it exits 0 and produces no changes here; the fix is internal and no public signature or JSDoc changed./exampleapp./example)Component to reproduce the issue you're fixing
Per
.github/REPRODUCING.mdthe reproducer should fail on the unfixed build and pass with the fix. This bug reproduces as a jest test, so it needs no device, no simulator and no Mapbox token, and it is committed with the fix as a regression guard in__tests__/components/MapView.test.js.The reproduction is non-tautological: on unfixed code the failure is raised by jest itself catching the escaped rejection, with a stack pointing straight at the offending line — not by any
expectin the test.BEFORE — fix reverted, tests kept (4 fail / 3 pass)
The three stacks are the three distinct escape routes:
componentDidMount,componentDidUpdate, and the queue drain.AFTER — fix applied (7 / 7 pass)
Two of the seven tests deliberately pass in both directions, so the suite cannot pass by simply swallowing everything:
stays quiet when the native call succeeds— a successful call still resolves and must produce no warning. Without it, the fix could "pass" by silencing every outcome.queues the call while the native ref is unresolved— asserts that on a plainrender(<MapView/>)the native module is called 0 times, the call is queued, and the queued branch still hands back a realPromise. That is the one plausible way this change could break: if the pre-ref branch returned something non-thenable, attaching.catch()to it would throw.Each test also registers a
process.on('unhandledRejection')spy, so the intent is asserted explicitly as well as caught implicitly by jest.Verification
Against
main@cbf2a2d(v10.3.5), node v24.13.0, yarn 3.6.1:yarn unittestyarn typecheckyarn lintyarn generateNo native change, no public API change, no generated-doc change.