Bridge promises don't handle multiple resolve or reject calls properly

[esprehn]

• [ x] Review the documentation: https://facebook.github.io/react-native
• [ x] Search for existing issues: https://github.com/facebook/react-native/issues
• [ x] Use the latest React Native release: https://github.com/facebook/react-native/releases

Environment

Environment:
OS: macOS High Sierra 10.13.5
Node: 8.9.4
Yarn: 1.5.1
npm: 5.6.0
Watchman: 4.7.0
Xcode: Xcode 9.4.1 Build version 9F2000
Android Studio: 3.1 AI-173.4720617

Packages: (wanted => installed)
react: 16.2.0 => 16.2.0
react-native: 0.54.4 => 0.54.4

Description

PromiseImpl and RCTModuleMethod don't handle promise resolution properly. Resolving a promise a second time is supposed to have no effect per ES semantics, similarly rejecting it a second time, or if it's rejected already, should have no effect.

PromiseImpl.resolve a second time will throw an error:

Exception java.lang.RuntimeException: Illegal callback invocation from native module. This callback type only permits a single invocation from native code.
com.facebook.react.bridge.CallbackImpl.invoke (CallbackImpl.java:30)
com.facebook.react.bridge.PromiseImpl.resolve (PromiseImpl.java:32)

For iOS this doesn't crash in ObjC, instead it sends another message over the bridge, but the callbacks are gone, so it incorrectly warns in a dev build (Callback with id ${cbID}: ${module}.${method}() not found) and is correctly a no-op in a release build (though it is inefficient to send the bridge response).

Reproducible Demo

Create a @RectMethod and do resolve() in the Java twice, or do do resolve twice in an ObjC iOS method.

[esprehn]

This happens in the most recent version too. The code is the same.

[hramos]

@esprehn can you update your post accordingly? 0.54.4 is not the latest as of this writing, and we'd like to know the exact version you tested in.

[esprehn]

@hramos It happens in the most recent version as well. The code is here:

The bridge makes the callbacks here:

react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaMethodWrapper.java

Line 126 in 26684cf

public Promise extractArgument(

which contains the throw statement:

react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CallbackImpl.java

Line 28 in 26684cf

throw new RuntimeException("Illegal callback invocation from native "+

None of the related files have been touched since the relicensing in Feb.

For Android this could be fixed by setting mResolve and mReject to null, for iOS I'd need to study the bridge code more, it doesn't have the same kind of Promise abstraction, instead it seems to just pass two blocks into your method.

[esprehn]

This does actually crash on iOS in a debug build, I misread the code:

react-native/React/Base/RCTModuleMethod.mm

Line 119 in 30b9d81

static BOOL checkCallbackMultipleInvocations(BOOL *didInvoke) {

[react-native-bot]

I am closing this issue because it does not appear to have been verified on the latest release, and there has been no followup in a while.

If you found this thread after encountering the same issue in the latest release, please feel free to create a new issue with up-to-date information by clicking here.

[esprehn]

This bug still exists. @hramos The bot probably shouldn't close issues with pending PRs?

[hramos]

@esprehn the bot won't close issues with a pending PR, but this issue was not labeled as such at the time of closing. Can you update your original post and make sure it mentions the actual version you tested in? Otherwise the bot might close it again for referring to an older version (0.56 is latest at this time).

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

[esprehn]

This issue still exists. Bridge promises don't follow the spec.

[dulmandakh]

This issue if fixed in 0.59. Thank you