Skip to content

fix(MapView): don't leak unhandled rejections from native bridge calls - #4280

Open
giaBaoJS wants to merge 1 commit into
rnmapbox:mainfrom
giaBaoJS:fix/3492-unhandled-rejection-native-bridge
Open

fix(MapView): don't leak unhandled rejections from native bridge calls#4280
giaBaoJS wants to merge 1 commit into
rnmapbox:mainfrom
giaBaoJS:fix/3492-unhandled-rejection-native-bridge

Conversation

@giaBaoJS

Copy link
Copy Markdown

Description

Fixes #3492

MapView._setHandledMapChangedEvents discards the promise returned by _runNativeMethod (MapView.tsx#L675):

this._runNativeMethod('setHandledMapChangedEvents', this._nativeRef, [
  events,
]);

_runNativeMethod returns Promise<ReturnType> (NativeBridgeComponent.tsx#L49) and no rejection handler is ever attached. It is called from both componentDidMount (MapView.tsx:581) and componentDidUpdate (MapView.tsx:598). Tearing the native view down while a call is in flight — navigating away from the map — makes the bridge reject with Unknown reactTag: <n>, which React Native escalates to Possible Unhandled Promise Rejection.

The componentDidUpdate path 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 _runNativeMethodDetached to NativeBridgeComponent for calls whose result is intentionally discarded, and routed the three fire-and-forget call sites through it:

Call site Trigger
MapView._setHandledMapChangedEvents (MapView.tsx:675) lifecycle — the reported bug
MapView.setSourceVisibility (MapView.tsx:940) public API
PointAnnotation.refresh (PointAnnotation.tsx:212) public API

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. runNativeMethod throws synchronously rather than rejecting when the view handle is already gone (utils/index.ts#L61):

const handle = findNodeHandle(nativeRef);
if (!handle) {
  throw new Error(`Could not find handle for native ref ${module}.${name}`);
}

That is the same race with a worse outcome — an uncaught exception out of componentDidMount rather than a warning — and a bare .catch() would never be installed to catch it. Hence the try/catch around the .catch().

2. _runPendingNativeMethods leaked the same way. It is async and 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:

  • Every one of these call sites has already returned by the time the promise settles. 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".
  • The alternative discriminator would be matching on the string 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.warn with 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.warn is not captured by Sentry's default integrations, whereas unhandled rejections are.

I used console.warn rather than Logger deliberately: src/utils/Logger.ts is a bridge for native Mapbox log events (its only message-emitting method is the onLog event handler, and MapView uses it solely via .start()/.stop()). Pushing a JS-originated message into that stream would change what Logger.setLogCallback consumers receive. console.warn is what the rest of the codebase — including the six sibling warnings inside _setHandledMapChangedEvents itself — 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

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder — it exits 0 and produces no changes here; the fix is internal and no public signature or JSDoc changed.
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Component to reproduce the issue you're fixing

Per .github/REPRODUCING.md the 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 expect in the test.

BEFORE — fix reverted, tests kept (4 fail / 3 pass)
    ✓ renders with testID (9 ms)
    setHandledMapChangedEvents
      ✓ queues the call while the native ref is unresolved (1 ms)
      ✕ does not leak an unhandled rejection on mount (1 ms)
      ✕ does not leak an unhandled rejection on update (3 ms)
      ✕ reports a synchronous failure instead of throwing (11 ms)
      ✕ does not leak an unhandled rejection while draining the queue
      ✓ stays quiet when the native call succeeds

  ● MapView › setHandledMapChangedEvents › does not leak an unhandled rejection on mount

    Unknown reactTag: 123

      71 |       jest.spyOn(bridgePrototype, '_runNativeMethod').mockImplementation(() => {
      72 |         // Built here so the stack points at the real bridge call site.
    > 73 |         error = new Error('Unknown reactTag: 123');
         |                 ^
      74 |         return Promise.reject(error);
      75 |       });

      at MapView.<anonymous> (__tests__/components/MapView.test.js:73:17)
      at MapView._runNativeMethod [as _setHandledMapChangedEvents] (src/components/MapView.tsx:675:10)
      at MapView._setHandledMapChangedEvents [as componentDidMount] (src/components/MapView.tsx:581:10)

  ● MapView › setHandledMapChangedEvents › does not leak an unhandled rejection on update

    Unknown reactTag: 123

      at MapView._runNativeMethod [as _setHandledMapChangedEvents] (src/components/MapView.tsx:675:10)
      at MapView._setHandledMapChangedEvents [as componentDidUpdate] (src/components/MapView.tsx:598:12)

  ● MapView › setHandledMapChangedEvents › does not leak an unhandled rejection while draining the queue

    Unknown reactTag: 123

      at MapView._runNativeMethod [as _runPendingNativeMethods] (src/components/NativeBridgeComponent.tsx:38:36)
      at MapView._runPendingNativeMethods [as _setNativeRef] (src/components/MapView.tsx:1202:13)

Tests:       4 failed, 3 passed, 7 total

The three stacks are the three distinct escape routes: componentDidMount, componentDidUpdate, and the queue drain.

AFTER — fix applied (7 / 7 pass)
PASS __tests__/components/MapView.test.js
  MapView
    ✓ renders with testID (10 ms)
    setHandledMapChangedEvents
      ✓ queues the call while the native ref is unresolved (1 ms)
      ✓ does not leak an unhandled rejection on mount (1 ms)
      ✓ does not leak an unhandled rejection on update (2 ms)
      ✓ reports a synchronous failure instead of throwing
      ✓ does not leak an unhandled rejection while draining the queue (1 ms)
      ✓ stays quiet when the native call succeeds

Test Suites: 1 passed, 1 total
Tests:       7 passed, 7 total

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 plain render(<MapView/>) the native module is called 0 times, the call is queued, and the queued branch still hands back a real Promise. 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:

before after
yarn unittest 22 suites, 112 tests, all pass 22 suites, 118 tests, all pass
yarn typecheck exit 0 exit 0
yarn lint 0 errors, 95 warnings 0 errors, 95 warnings (unchanged)
yarn generate exit 0, no changes exit 0, no changes

No native change, no public API change, no generated-doc change.

`_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
giaBaoJS requested a deployment to CI with Mapbox Tokens August 14, 2026 02:52 — with GitHub Actions Waiting
@giaBaoJS
giaBaoJS requested a deployment to CI with Mapbox Tokens August 14, 2026 02:52 — with GitHub Actions Waiting
@giaBaoJS
giaBaoJS requested a deployment to CI with Mapbox Tokens August 14, 2026 02:52 — with GitHub Actions Waiting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Navigating away from Map causes unhandled promise rejection for "setHandledMapChangedEvents"

1 participant