diff --git a/__tests__/components/MapView.test.js b/__tests__/components/MapView.test.js index e580fe217..76e278856 100644 --- a/__tests__/components/MapView.test.js +++ b/__tests__/components/MapView.test.js @@ -2,6 +2,18 @@ import * as React from 'react'; import { render } from '@testing-library/react-native'; import MapView from '../../src/components/MapView'; +import NativeMapViewModule from '../../src/specs/NativeMapViewModule'; + +// `_runNativeMethod` lives on the class produced by `NativeBridgeComponent`, +// which `MapView` extends - it is not an own property of `MapView.prototype`. +const bridgePrototype = Object.getPrototypeOf(MapView.prototype); + +// Let the microtask queue drain and give node a macrotask turn, which is when +// an unhandled rejection would be reported. +const flushRejections = async () => { + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); +}; describe('MapView', () => { test('renders with testID', () => { @@ -13,4 +25,153 @@ describe('MapView', () => { getByTestId(expectedTestId); }).not.toThrow(); }); + + describe('setHandledMapChangedEvents', () => { + let unhandledRejection; + let warnSpy; + + beforeEach(() => { + unhandledRejection = jest.fn(); + process.on('unhandledRejection', unhandledRejection); + warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + process.off('unhandledRejection', unhandledRejection); + jest.restoreAllMocks(); + }); + + // Regression guard for the fix itself: on a plain render the native ref is + // not resolved yet, so the call is queued rather than sent to the native + // module. The queued branch must still hand back a real promise, otherwise + // attaching a rejection handler to it would throw. + test('queues the call while the native ref is unresolved', () => { + const nativeSpy = jest.spyOn( + NativeMapViewModule, + 'setHandledMapChangedEvents', + ); + const ref = React.createRef(); + + render( {}} />); + + expect(ref.current._nativeRef).toBeUndefined(); + expect(nativeSpy).not.toHaveBeenCalled(); + expect(ref.current._preRefMapMethodQueue).toHaveLength(1); + expect( + ref.current._runNativeMethod('setHandledMapChangedEvents', undefined, [ + [], + ]), + ).toBeInstanceOf(Promise); + }); + + // https://github.com/rnmapbox/maps/issues/3492 - unmounting the map while + // the call is in flight rejects with `Unknown reactTag: `. + test('does not leak an unhandled rejection on mount', async () => { + let error; + jest.spyOn(bridgePrototype, '_runNativeMethod').mockImplementation(() => { + // Built here so the stack points at the real bridge call site. + error = new Error('Unknown reactTag: 123'); + return Promise.reject(error); + }); + + render( {}} />); + await flushRejections(); + + expect(unhandledRejection).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('setHandledMapChangedEvents'), + error, + ); + }); + + test('does not leak an unhandled rejection on update', async () => { + const ref = React.createRef(); + const { rerender } = render( {}} />); + + let error; + const runNativeMethod = jest + .spyOn(bridgePrototype, '_runNativeMethod') + .mockImplementation(() => { + // Built here so the stack points at the real bridge call site. + error = new Error('Unknown reactTag: 123'); + return Promise.reject(error); + }); + + // A new inline handler changes the callback prop identity, which is what + // makes `componentDidUpdate` resend the handled events. + rerender( {}} />); + await flushRejections(); + + expect(runNativeMethod).toHaveBeenCalledWith( + 'setHandledMapChangedEvents', + undefined, + [expect.arrayContaining([expect.any(String)])], + ); + expect(unhandledRejection).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('setHandledMapChangedEvents'), + error, + ); + }); + + // `runNativeMethod` throws synchronously when the view handle has already + // gone, so a bare `.catch()` would not cover it. + test('reports a synchronous failure instead of throwing', async () => { + const error = new Error('Could not find handle for native ref'); + jest + .spyOn(bridgePrototype, '_runNativeMethod') + .mockImplementation(() => { + throw error; + }); + + expect(() => render( {}} />)).not.toThrow(); + await flushRejections(); + + expect(unhandledRejection).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('setHandledMapChangedEvents'), + error, + ); + }); + + // The mount-time call is queued until the native ref lands, so it is + // drained by `_runPendingNativeMethods` - an async method every caller + // invokes fire-and-forget, which leaks the rejection just the same. + test('does not leak an unhandled rejection while draining the queue', async () => { + const ref = React.createRef(); + render( {}} />); + + expect(ref.current._preRefMapMethodQueue).toHaveLength(1); + + let error; + jest.spyOn(bridgePrototype, '_runNativeMethod').mockImplementation(() => { + error = new Error('Unknown reactTag: 123'); + return Promise.reject(error); + }); + + ref.current._setNativeRef({}); + await flushRejections(); + + expect(unhandledRejection).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('setHandledMapChangedEvents'), + error, + ); + }); + + // Non-vacuity: the tests above must not pass merely because every outcome + // is being swallowed - a successful call still resolves and stays quiet. + test('stays quiet when the native call succeeds', async () => { + const runNativeMethod = jest + .spyOn(bridgePrototype, '_runNativeMethod') + .mockImplementation(() => Promise.resolve(undefined)); + + render( {}} />); + await flushRejections(); + + expect(runNativeMethod).toHaveBeenCalledTimes(1); + expect(unhandledRejection).not.toHaveBeenCalled(); + expect(warnSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/components/MapView.tsx b/src/components/MapView.tsx index d7b5bf502..44ff5cc29 100644 --- a/src/components/MapView.tsx +++ b/src/components/MapView.tsx @@ -672,9 +672,11 @@ class MapView extends NativeBridgeComponent( ); } - this._runNativeMethod('setHandledMapChangedEvents', this._nativeRef, [ - events, - ]); + this._runNativeMethodDetached( + 'setHandledMapChangedEvents', + this._nativeRef, + [events], + ); } /** @@ -937,7 +939,7 @@ class MapView extends NativeBridgeComponent( sourceId: string, sourceLayerId: string | null = null, ) { - this._runNative('setSourceVisibility', [ + this._runNativeMethodDetached('setSourceVisibility', this._nativeRef, [ visible, sourceId, sourceLayerId, diff --git a/src/components/NativeBridgeComponent.tsx b/src/components/NativeBridgeComponent.tsx index dbc372f34..2eeecbf7d 100644 --- a/src/components/NativeBridgeComponent.tsx +++ b/src/components/NativeBridgeComponent.tsx @@ -29,18 +29,39 @@ const NativeBridgeComponent = < this._preRefMapMethodQueue = []; } + /** + * Reports a native method that failed with no caller left to handle it. + * + * Rethrowing is not an option here: every caller of the methods below + * discards the promise, so a rethrow would only recreate the unhandled + * rejection this is meant to remove. + */ + _warnNativeMethodFailed(methodName: string, error: unknown) { + console.warn( + `rnmapbox/maps: native method ${methodName} failed - this is expected if the view was detached while the call was in flight:`, + error, + ); + } + async _runPendingNativeMethods(nativeRef: RefType) { if (nativeRef) { while (this._preRefMapMethodQueue.length > 0) { const item = this._preRefMapMethodQueue.pop(); if (item && item.method && item.resolver) { - const res = await this._runNativeMethod( - item.method.name, - nativeRef, - item.method.args, - ); - item.resolver(res); + // Every caller invokes this fire-and-forget, so a rejection here + // would escape as an unhandled rejection. Catching per item also + // keeps one failure from abandoning the rest of the queue. + try { + const res = await this._runNativeMethod( + item.method.name, + nativeRef, + item.method.args, + ); + item.resolver(res); + } catch (error) { + this._warnNativeMethodFailed(item.method.name, error); + } } } } @@ -62,6 +83,31 @@ const NativeBridgeComponent = < return runNativeMethod(this._turboModule, methodName, nativeRef, args); } + + /** + * Runs a native method whose result is intentionally discarded. + * + * Callers that drop the promise on the floor leak an unhandled rejection + * whenever the native view goes away while the call is in flight - a tab + * switch, or any unmount - because the bridge then rejects with + * `Unknown reactTag: `. + */ + _runNativeMethodDetached( + methodName: string, + nativeRef: RefType | undefined, + args: NativeArg[] = [], + ): void { + const onError = (error: unknown) => + this._warnNativeMethodFailed(methodName, error); + + try { + // `runNativeMethod` throws synchronously when the view handle is + // already gone, so the try/catch is not redundant with `.catch`. + this._runNativeMethod(methodName, nativeRef, args).catch(onError); + } catch (error) { + onError(error); + } + } }; export default NativeBridgeComponent; diff --git a/src/components/PointAnnotation.tsx b/src/components/PointAnnotation.tsx index 3c889e042..b6d9582f4 100644 --- a/src/components/PointAnnotation.tsx +++ b/src/components/PointAnnotation.tsx @@ -209,7 +209,7 @@ class PointAnnotation extends NativeBridgeComponent( * Call this for example from Image#onLoad. */ refresh() { - this._runNativeMethod('refresh', this._nativeRef, []); + this._runNativeMethodDetached('refresh', this._nativeRef, []); } _setNativeRef(nativeRef: NativePointAnnotationRef) {