Fix LogBox parsing for cross-runtime jsi::JSErrors - #55994
Conversation
|
Hi @Naim08! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
There was a problem hiding this comment.
Pull request overview
Fixes LogBox error display for tasks that throw jsi::JSError originating from a different Hermes runtime by avoiding cross-runtime jsi::Value usage and reconstructing an Error object in the current runtime before reporting.
Changes:
- Add
reportError()helper to Legacy/Modern RuntimeScheduler implementations to reconstruct and forward errors safely in the current runtime. - Update scheduler error handling call sites to use
reportError()forjsi::JSError. - Extend test scaffolding and add new tests to validate cross-runtime error forwarding and message preservation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/StubErrorUtils.h | Capture last reported error message from ErrorUtils.reportFatalError for assertions. |
| packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp | Add tests covering cross-runtime jsi::JSError handling and message preservation. |
| packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.h | Declare reportError() helper and document cross-runtime error rationale. |
| packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp | Implement reportError() and route jsi::JSError handling through it. |
| packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.{h,cpp} | Add/implement reportError() and route jsi::JSError handling through it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| int reportFatalCallCount_; | ||
| std::string lastReportedMessage_; |
There was a problem hiding this comment.
StubErrorUtils leaves reportFatalCallCount_ uninitialized. Because the object is created with the implicit default constructor, the first reportFatalCallCount_++ is undefined behavior and can make the tests flaky. Initialize this member (and lastReportedMessage_) with in-class initializers or an explicit constructor.
| int reportFatalCallCount_; | |
| std::string lastReportedMessage_; | |
| int reportFatalCallCount_{0}; | |
| std::string lastReportedMessage_{}; |
| [this]( | ||
| jsi::Runtime &runtime, const jsi::Value &, const jsi::Value *arguments, size_t) noexcept -> jsi::Value { | ||
| jsi::Runtime &runtime, const jsi::Value &, const jsi::Value *arguments, size_t count) noexcept -> jsi::Value { | ||
| reportFatalCallCount_++; | ||
| if (count > 0 && arguments[0].isObject()) { | ||
| auto obj = arguments[0].getObject(runtime); | ||
| auto msgVal = obj.getProperty(runtime, "message"); | ||
| if (msgVal.isString()) { | ||
| lastReportedMessage_ = msgVal.getString(runtime).utf8(runtime); |
There was a problem hiding this comment.
The host function lambda for reportFatalError is declared noexcept, but the newly added JSI operations (getObject, getProperty, getString) can throw jsi::JSIException. If any of those throw, the process will std::terminate(). Remove noexcept here or wrap the property access in a try/catch to keep the stub safe.
| reportFatalCallCount_++; | ||
| if (count > 0 && arguments[0].isObject()) { | ||
| auto obj = arguments[0].getObject(runtime); | ||
| auto msgVal = obj.getProperty(runtime, "message"); | ||
| if (msgVal.isString()) { | ||
| lastReportedMessage_ = msgVal.getString(runtime).utf8(runtime); | ||
| } | ||
| } |
There was a problem hiding this comment.
lastReportedMessage_ is only updated when a valid message string is found; otherwise it retains its previous value. Consider clearing it at the start of each reportFatalError call (or explicitly setting it to an empty string when parsing fails) to avoid stale assertions if multiple errors are reported.
| error.getStack()); | ||
| onTaskError_(runtime, localError); | ||
| } catch (...) { | ||
| jsi::JSError fallbackError(runtime, error.getMessage()); |
There was a problem hiding this comment.
In the fallback path you construct jsi::JSError with only error.getMessage(), which drops the original stack trace even though error.getStack() is available as a safe C++ string. Consider using the JSError(Runtime&, message, stack) constructor here so LogBox still gets a stack when reconstruction fails.
| jsi::JSError fallbackError(runtime, error.getMessage()); | |
| jsi::JSError fallbackError( | |
| runtime, | |
| error.getMessage(), | |
| error.getStack()); |
| error.getStack()); | ||
| onTaskError_(runtime, localError); | ||
| } catch (...) { | ||
| jsi::JSError fallbackError(runtime, error.getMessage()); |
There was a problem hiding this comment.
The fallback path reconstructs a jsi::JSError with only the message, which discards error.getStack() even though it is already available and safe. Consider using the JSError(Runtime&, message, stack) constructor in the fallback to preserve the original stack trace when Error reconstruction fails.
| jsi::JSError fallbackError(runtime, error.getMessage()); | |
| jsi::JSError fallbackError( | |
| runtime, | |
| error.getMessage(), | |
| error.getStack()); |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Summary
When a
jsi::JSErroris thrown from a background Hermes runtime and caught in the RuntimeScheduler, the error'sjsi::Valuebelongs to a differentjsi::Runtime. Forwarding this cross-runtime value toonTaskError_is undefined behavior and causes LogBox to display "Unknown" instead of the actual error message.This PR introduces a
reportError()helper in bothRuntimeScheduler_LegacyandRuntimeScheduler_Modernthat:Errorinstance in the current runtime using the safe C++ string accessors (getMessage()/getStack())jsi::JSErrorif reconstruction failsChangelog:
[GENERAL] [FIXED] - Fix LogBox showing "Unknown" for errors originating from background Hermes runtimes
Test plan:
handlingCrossRuntimeErrortest: throws ajsi::JSErrorfrom a second Hermes runtime and verifies the error message is correctly forwardedhandlingErrorPreservesMessagetest: verifies same-runtime errors still work correctly with the new code pathStubErrorUtilsto capture and expose the last reported error message for assertion🤖 Generated with Claude Code