Skip to content

Commit

Permalink
Add virtual destructor to JSError
Browse files Browse the repository at this point in the history
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
  • Loading branch information
javache authored and facebook-github-bot committed May 7, 2020
1 parent 851644c commit e566c7e
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 0 deletions.
6 changes: 6 additions & 0 deletions ReactCommon/jsi/jsi/jsi.cpp
Expand Up @@ -450,5 +450,11 @@ void JSError::setValue(Runtime& rt, Value&& value) {
}
}

JSIException::~JSIException() {}

JSINativeException::~JSINativeException() {}

JSError::~JSError() {}

} // namespace jsi
} // namespace facebook
6 changes: 6 additions & 0 deletions ReactCommon/jsi/jsi/jsi.h
Expand Up @@ -1212,6 +1212,8 @@ class JSI_EXPORT JSIException : public std::exception {
return what_.c_str();
}

virtual ~JSIException();

protected:
std::string what_;
};
Expand All @@ -1221,6 +1223,8 @@ class JSI_EXPORT JSIException : public std::exception {
class JSI_EXPORT JSINativeException : public JSIException {
public:
JSINativeException(std::string what) : JSIException(std::move(what)) {}

virtual ~JSINativeException();
};

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

virtual ~JSError();

const std::string& getStack() const {
return stack_;
}
Expand Down

0 comments on commit e566c7e

Please sign in to comment.