Skip to content

Commit e566c7e

Browse files
javachefacebook-github-bot
authored andcommitted
Add virtual destructor to JSError
Summary: We consume Hermes through multiple .so's, which means we have multiple (weak) typeinfo definitions of facebook::jsi::JSError. Previously we were using gnustl, which would strcmp typeinfo to decide whether a certain exception handler applies, which meant this didn't cause any major issues. However since this is deprecated, we recently switched to libc++, which does not have this by behaviour (or it does, but behind a flag I'm not sure how to enable). This causes any JS exceptions to fall through from our exception handlers and fatal the app. This problem is actually documented in the common Android NDK problems page: https://android.googlesource.com/platform/ndk/+/master/docs/user/common_problems.md#rtti_exceptions-not-working-across-library-boundaries The suggested solution is to ensure that any exception types have a key function defined (a non-pure, out-of-line virtual function). The simplest one to add is a virtual destructor. This makes the object file that holds the implementation of the destructor export a non-weak typeinfo definition which will at load time override the other weak versions. I'm not sure why we're the first to hit this. RN's JSIExecutor doesn't explicitly reference JSError which probably helps (https://github.com/facebook/react-native/blob/master/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp#L256-L258) and they also don't use unguarded callbacks like we do. Changelog: [Internal] Reviewed By: mhorowitz Differential Revision: D21426524 fbshipit-source-id: 474284ada1ca2810045dc4402c420879447f9308
1 parent 851644c commit e566c7e

File tree

2 files changed

+12
-0
lines changed

2 files changed

+12
-0
lines changed

ReactCommon/jsi/jsi/jsi.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,5 +450,11 @@ void JSError::setValue(Runtime& rt, Value&& value) {
450450
}
451451
}
452452

453+
JSIException::~JSIException() {}
454+
455+
JSINativeException::~JSINativeException() {}
456+
457+
JSError::~JSError() {}
458+
453459
} // namespace jsi
454460
} // namespace facebook

ReactCommon/jsi/jsi/jsi.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1212,6 +1212,8 @@ class JSI_EXPORT JSIException : public std::exception {
12121212
return what_.c_str();
12131213
}
12141214

1215+
virtual ~JSIException();
1216+
12151217
protected:
12161218
std::string what_;
12171219
};
@@ -1221,6 +1223,8 @@ class JSI_EXPORT JSIException : public std::exception {
12211223
class JSI_EXPORT JSINativeException : public JSIException {
12221224
public:
12231225
JSINativeException(std::string what) : JSIException(std::move(what)) {}
1226+
1227+
virtual ~JSINativeException();
12241228
};
12251229

12261230
/// This exception will be thrown by API functions whenever a JS
@@ -1249,6 +1253,8 @@ class JSI_EXPORT JSError : public JSIException {
12491253
/// but necessary to avoid ambiguity with the above.
12501254
JSError(std::string what, Runtime& rt, Value&& value);
12511255

1256+
virtual ~JSError();
1257+
12521258
const std::string& getStack() const {
12531259
return stack_;
12541260
}

0 commit comments

Comments
 (0)