Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
This was referenced Sep 25, 2026
JSError(rt, message) built its value by looking up and calling the global Error function, which predates IRuntime::createError. Use rt.createError instead, as the JSError::createXError factories already do. Runtimes without a native implementation behave as before, since the default Runtime::createError makes the same global call. Runtimes with a native implementation skip the global lookup and JS call. RuntimeDecorator and WithRuntimeDecorator did not override the createError family, so decorated runtimes (e.g. the thread-safe runtime and TracingRuntime) fell back to the default global calls instead of the wrapped runtime's implementation. Forward them. JSErrorDoesNotInfinitelyRecurse now runs through a decorator that keeps the default createError, so it still covers the fallback when creating the Error fails.
mrousavy
force-pushed
the
jsi-jserror-create-error
branch
from
September 26, 2026 13:16
e050793 to
f3a9eab
Compare
lavenzg
requested changes
Oct 7, 2026
| // implementation, which calls the global Error constructor, so that removing | ||
| // it below makes creating the Error fail even if the runtime has a native | ||
| // createError. | ||
| class RD : public RuntimeDecorator<Runtime, Runtime> { |
Contributor
There was a problem hiding this comment.
We need to do the same for test CreateJSErrorPropagatesOOMExceptionTest.
lavenzg
pushed a commit
to lavenzg/react-native
that referenced
this pull request
Oct 7, 2026
…eact#58936) Summary: `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396bd4 on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (facebook/hermes#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. X-link: facebook/hermes#2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg
Run CreateJSErrorPropagatesOOMExceptionTest through a decorator that calls Runtime::createError, so its replacement global Error constructor still triggers OOM after JSError starts using the native createError API. Verified that the test fails before this change and passes afterward with HERMESVM_EXCEPTION_ON_OOM=ON. All 362 API tests pass in ASan+Debug.
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
…2205) Summary: X-link: react/react-native#58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396b on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. Pull Request resolved: #2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
…2205) Summary: X-link: react/react-native#58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396b on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. Pull Request resolved: #2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
meta-codesync Bot
pushed a commit
to react/react-native
that referenced
this pull request
Oct 8, 2026
…58936) Summary: Pull Request resolved: #58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396bd4 on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (facebook/hermes#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. X-link: facebook/hermes#2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
…2205) Summary: X-link: react/react-native#58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396b on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. Pull Request resolved: #2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
JSError(rt, message)still creates its value by looking upglobalThis.Errorand calling it, which predatesIRuntime::createError. It now usesrt.createError(...)like theJSError::createXErrorfactories, with the same try/catch fallback. The defaultRuntime::createErrormakes the same global call, so runtimes without a native implementation behave as before. On Hermes everyJSErrorthrown from native code skips a global lookup and a JS call, andmessage,stackandwhat()come out the same.RuntimeDecoratorandWithRuntimeDecoratordidn't forward thecreateErrorfamily, so decorated runtimes (the thread-safe runtime,TracingRuntime) quietly fell back to the global constructor calls. They forward toplain()now, throughAroundforWithRuntimeDecorator, and they're public like injsi::Runtime.One visible difference on Hermes, same as the other
createXErrorAPIs: if user code replaced or deletedglobalThis.Error,JSError(rt, message)no longer calls the replacement and creates a realErroranyway.Test Plan
Two new tests in testlib.cpp.
JSErrorMessageTestchecks the value is anErrorwith the right message and a string stack, and that a countingglobalThis.Errorreplacement gets called exactly as often asrt.createErrorcalls it, so it holds for native and default implementations.DecoratorCreateErrorTestchecks that all sevencreateXErrorcalls, andJSError, reach the wrapped runtime once through both decorators, and thatAroundruns once.JSErrorDoesNotInfinitelyRecurseandCreateJSErrorPropagatesOOMExceptionTestrun through decorators that use the defaultRuntime::createError. This keeps the missing-constructor fallback and OOM propagation covered, since both tests depend on changingglobalThis.Error.APITests(ASan + Debug, macOS arm64,HERMESVM_EXCEPTION_ON_OOM=ON): 362/362 pass, including SynthTrace and all three OOM tests. Built with-O1andHERMES_ENABLE_WERROR=ON; the existing GoogleTestuninitialized-const-pointerwarning is demoted for AppleClang 21.Before the OOM test adaptation,
CreateJSErrorPropagatesOOMExceptionTestfails because it throwsJSErrorinstead ofJSOutOfMemoryError. It passes with the decorator. The earlier checks with thejsi.cppanddecorator.hchanges reverted also made the new JSI tests fail.