Fix "Not implemented" exception on JSC #6028

tjzel · 2024-05-17T10:49:40Z

Summary

In some implementations of JSC, at least react-native-macos here native state methods are not implemented and they are throwing errors. This PR adds logic that will go around this issue, by probing the hasNativeState method to determine whether it should use that.

Fixes #5974

Test plan

MacOS example builds now.
Check if all "not implemented" methods in the link are safely accessed.

tomekzaw

Why can't we just call hasNativeState in try/catch block?

tjzel · 2024-05-20T08:57:06Z

@tomekzaw It's for performance. Why would we surround it with the try-catch on every call, since we will know everything after the first call.

tomekzaw · 2024-05-20T21:57:49Z

@tjzel I wouldn't worry much about performance in this case, C++ try/catch blocks are quite fast.

SInce we know that some versions react-native-macos don't support jsi::NativeState when using JSC, why can't we just use #ifdefs instead?

Also, the names nativeStateAccess, AccessProbe, Safe and Unsafe are a bit overwhelming, how about simply supportsNativeState with Yes and No instead?

Finally, I don't like the fact that some unrelated files have been formatted, can we move them to a separate PR?

mrousavy · 2024-05-22T12:01:34Z

Sorry about that, I added jsi::NativeState here.. I didn't test on MacOS / JSC.

tomekzaw · 2024-05-22T13:00:01Z

@mrousavy No worries

tjzel · 2024-05-27T10:27:56Z

@tomekzaw I read an interesting article about try/catch blocks performance from which it seems that try blocks are fast but the catch ones are expensive. Since these calls would constantly throw when nativeState is not implemented, I'd rather keep the current approach.

Regarding #ifdef I don't know how would we do that. I don't think there's any flag that would tell us if nativeState is implemented on a given engine.

I'll revert formatting and make the names more approachable.

piaskowyk · 2024-05-29T12:48:25Z

Common/cpp/SharedItems/Shareables.cpp

+void ShareableObject::runWithNativeStateProbe(
+    std::function<void()> &&block) {
+  try {
+    block();


Why we need to pass an lambda here instead of just execute obj.setNativeState(rt, nativeState_);?

We have more than just setNativeState function handled by it.

piaskowyk · 2024-05-29T12:48:28Z

Common/cpp/SharedItems/Shareables.cpp

+  try {
+    block();
+    isNativeStateImplemented_ = IsNativeStateImplemented::Yes;
+  } catch (...) {


Does this 'Not implemented' exception have a specific type that we can implicitly catch here?

I don't want to search through all of current implementations of it 🫣

piaskowyk

Based on the information from here - PR to React Native core, the native state has been available on JSC since version 0.74. Hence, it appears that we can simply create a straightforward compilation flag instead of runtime checks. What do you think?

Bumps [axios](https://github.com/axios/axios) from 1.4.0 to 1.6.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p> <blockquote> <h2>Release v1.6.0</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>CSRF:</strong> fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>) (<a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0">96ee232</a>)</li> <li><strong>dns:</strong> fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>) (<a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8">5aaff53</a>)</li> <li><strong>types:</strong> fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>) (<a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09">a1c8ad0</a>)</li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code> ⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+449/-114 ([software-mansion#6032](axios/axios#6032) [software-mansion#6021](axios/axios#6021) [software-mansion#6011](axios/axios#6011) [software-mansion#5932](axios/axios#5932) [software-mansion#5931](axios/axios#5931) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/valentin-panov" title="+4/-4 ([software-mansion#6028](axios/axios#6028) )">Valentin Panov</a></li> <li> <a href="https://github.com/therealrinku" title="+1/-1 ([software-mansion#5889](axios/axios#5889) )">Rinku Chaudhari</a></li> </ul> <h2>Release v1.5.1</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapters:</strong> improved adapters loading logic to have clear error messages; (<a href="https://redirect.github.com/axios/axios/issues/5919">#5919</a>) (<a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859">e410779</a>)</li> <li><strong>formdata:</strong> fixed automatic addition of the <code>Content-Type</code> header for FormData in non-browser environments; (<a href="https://redirect.github.com/axios/axios/issues/5917">#5917</a>) (<a href="https://github.com/axios/axios/commit/bc9af51b1886d1b3529617702f2a21a6c0ed5d92">bc9af51</a>)</li> <li><strong>headers:</strong> allow <code>content-encoding</code> header to handle case-insensitive values (<a href="https://redirect.github.com/axios/axios/issues/5890">#5890</a>) (<a href="https://redirect.github.com/axios/axios/issues/5892">#5892</a>) (<a href="https://github.com/axios/axios/commit/4c89f25196525e90a6e75eda9cb31ae0a2e18acd">4c89f25</a>)</li> <li><strong>types:</strong> removed duplicated code (<a href="https://github.com/axios/axios/commit/9e6205630e1c9cf863adf141c0edb9e6d8d4b149">9e62056</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+89/-18 ([software-mansion#5919](axios/axios#5919) [software-mansion#5917](axios/axios#5917) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/DavidJDallas" title="+11/-5 ()">David Dallas</a></li> <li> <a href="https://github.com/fb-sean" title="+2/-8 ()">Sean Sattler</a></li> <li> <a href="https://github.com/0o001" title="+4/-4 ()">Mustafa Ateş Uzun</a></li> <li> <a href="https://github.com/sfc-gh-pmotacki" title="+2/-1 ([software-mansion#5892](axios/axios#5892) )">Przemyslaw Motacki</a></li> <li> <a href="https://github.com/Cadienvan" title="+1/-1 ()">Michael Di Prisco</a></li> </ul> <h2>Release v1.5.0</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapter:</strong> make adapter loading error more clear by using platform-specific adapters explicitly (<a href="https://redirect.github.com/axios/axios/issues/5837">#5837</a>) (<a href="https://github.com/axios/axios/commit/9a414bb6c81796a95c6c7fe668637825458e8b6d">9a414bb</a>)</li> <li><strong>dns:</strong> fixed <code>cacheable-lookup</code> integration; (<a href="https://redirect.github.com/axios/axios/issues/5836">#5836</a>) (<a href="https://github.com/axios/axios/commit/b3e327dcc9277bdce34c7ef57beedf644b00d628">b3e327d</a>)</li> <li><strong>headers:</strong> added support for setting header names that overlap with class methods; (<a href="https://redirect.github.com/axios/axios/issues/5831">#5831</a>) (<a href="https://github.com/axios/axios/commit/d8b4ca0ea5f2f05efa4edfe1e7684593f9f68273">d8b4ca0</a>)</li> <li><strong>headers:</strong> fixed common Content-Type header merging; (<a href="https://redirect.github.com/axios/axios/issues/5832">#5832</a>) (<a href="https://github.com/axios/axios/commit/8fda2766b1e6bcb72c3fabc146223083ef13ce17">8fda276</a>)</li> </ul> <h3>Features</h3>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p> <blockquote> <h1><a href="https://github.com/axios/axios/compare/v1.5.1...v1.6.0">1.6.0</a> (2023-10-26)</h1> <h3>Bug Fixes</h3> <ul> <li><strong>CSRF:</strong> fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>) (<a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0">96ee232</a>)</li> <li><strong>dns:</strong> fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>) (<a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8">5aaff53</a>)</li> <li><strong>types:</strong> fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>) (<a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09">a1c8ad0</a>)</li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code> ⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+449/-114 ([software-mansion#6032](axios/axios#6032) [software-mansion#6021](axios/axios#6021) [software-mansion#6011](axios/axios#6011) [software-mansion#5932](axios/axios#5932) [software-mansion#5931](axios/axios#5931) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/valentin-panov" title="+4/-4 ([software-mansion#6028](axios/axios#6028) )">Valentin Panov</a></li> <li> <a href="https://github.com/therealrinku" title="+1/-1 ([software-mansion#5889](axios/axios#5889) )">Rinku Chaudhari</a></li> </ul> <h2><a href="https://github.com/axios/axios/compare/v1.5.0...v1.5.1">1.5.1</a> (2023-09-26)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapters:</strong> improved adapters loading logic to have clear error messages; (<a href="https://redirect.github.com/axios/axios/issues/5919">#5919</a>) (<a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859">e410779</a>)</li> <li><strong>formdata:</strong> fixed automatic addition of the <code>Content-Type</code> header for FormData in non-browser environments; (<a href="https://redirect.github.com/axios/axios/issues/5917">#5917</a>) (<a href="https://github.com/axios/axios/commit/bc9af51b1886d1b3529617702f2a21a6c0ed5d92">bc9af51</a>)</li> <li><strong>headers:</strong> allow <code>content-encoding</code> header to handle case-insensitive values (<a href="https://redirect.github.com/axios/axios/issues/5890">#5890</a>) (<a href="https://redirect.github.com/axios/axios/issues/5892">#5892</a>) (<a href="https://github.com/axios/axios/commit/4c89f25196525e90a6e75eda9cb31ae0a2e18acd">4c89f25</a>)</li> <li><strong>types:</strong> removed duplicated code (<a href="https://github.com/axios/axios/commit/9e6205630e1c9cf863adf141c0edb9e6d8d4b149">9e62056</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+89/-18 ([software-mansion#5919](axios/axios#5919) [software-mansion#5917](axios/axios#5917) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/DavidJDallas" title="+11/-5 ()">David Dallas</a></li> <li> <a href="https://github.com/fb-sean" title="+2/-8 ()">Sean Sattler</a></li> <li> <a href="https://github.com/0o001" title="+4/-4 ()">Mustafa Ateş Uzun</a></li> <li> <a href="https://github.com/sfc-gh-pmotacki" title="+2/-1 ([software-mansion#5892](axios/axios#5892) )">Przemyslaw Motacki</a></li> <li> <a href="https://github.com/Cadienvan" title="+1/-1 ()">Michael Di Prisco</a></li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code> ⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h1><a href="https://github.com/axios/axios/compare/v1.4.0...v1.5.0">1.5.0</a> (2023-08-26)</h1>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/axios/axios/commit/f7adacdbaa569281253c8cfc623ad3f4dc909c60"><code>f7adacd</code></a> chore(release): v1.6.0 (<a href="https://redirect.github.com/axios/axios/issues/6031">#6031</a>)</li> <li><a href="https://github.com/axios/axios/commit/9917e67cbb6c157382863bad8c741de58e3f3c2b"><code>9917e67</code></a> chore(ci): fix release-it arg; (<a href="https://redirect.github.com/axios/axios/issues/6032">#6032</a>)</li> <li><a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0"><code>96ee232</code></a> fix(CSRF): fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>)</li> <li><a href="https://github.com/axios/axios/commit/7d45ab2e2ad6e59f5475e39afd4b286b1f393fc0"><code>7d45ab2</code></a> chore(tests): fixed tests to pass in node v19 and v20 with <code>keep-alive</code> enabl...</li> <li><a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8"><code>5aaff53</code></a> fix(dns): fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>)</li> <li><a href="https://github.com/axios/axios/commit/a48a63ad823fc20e5a6a705f05f09842ca49f48c"><code>a48a63a</code></a> chore(docs): added AxiosHeaders docs; (<a href="https://redirect.github.com/axios/axios/issues/5932">#5932</a>)</li> <li><a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09"><code>a1c8ad0</code></a> fix(types): fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>)</li> <li><a href="https://github.com/axios/axios/commit/2ac731d60545ba5c4202c25fd2e732ddd8297d82"><code>2ac731d</code></a> chore(docs): update readme.md (<a href="https://redirect.github.com/axios/axios/issues/5889">#5889</a>)</li> <li><a href="https://github.com/axios/axios/commit/88fb52b5fad7aabab0532e7ad086c5f1b0178905"><code>88fb52b</code></a> chore(release): v1.5.1 (<a href="https://redirect.github.com/axios/axios/issues/5920">#5920</a>)</li> <li><a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859"><code>e410779</code></a> fix(adapters): improved adapters loading logic to have clear error messages; ...</li> <li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/v1.4.0...v1.6.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=1.4.0&new-version=1.6.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/software-mansion/react-native-reanimated/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

tjzel · 2024-06-19T08:56:23Z

Well I guess we can do it that way for now, if the problem persists we will resort to runtime checks.

tjzel · 2024-06-19T12:04:03Z

Superseded by #6137

Supersedes 🚽 #6028 Fixes #5974

tjzel added 2 commits May 17, 2024 12:33

init

adec927

set native state

bbcb11f

tjzel requested review from tomekzaw and piaskowyk May 17, 2024 10:49

tjzel added 2 commits May 17, 2024 12:56

use lambda

285670b

fix type

bd7fbcd

tomekzaw reviewed May 17, 2024

View reviewed changes

tjzel added 3 commits May 27, 2024 13:56

Merge branch 'main' into @tjzel/macos/fix-JSC-error

69b72db

review changes

ddc46d1

renamve v2

fff280c

piaskowyk reviewed May 29, 2024

View reviewed changes

piaskowyk reviewed Jun 13, 2024

View reviewed changes

tjzel mentioned this pull request Jun 19, 2024

Fix "Not implemented" exception with macros #6137

Merged

tjzel closed this Jun 19, 2024

github-merge-queue bot pushed a commit that referenced this pull request Jul 1, 2024

Fix "Not implemented" exception with macros (#6137)

90fe8d3

Supersedes 🚽 #6028 Fixes #5974

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Not implemented" exception on JSC #6028

Fix "Not implemented" exception on JSC #6028

tjzel commented May 17, 2024 •

edited by patrycjakalinska

Loading

tomekzaw left a comment

tjzel commented May 20, 2024

tomekzaw commented May 20, 2024 •

edited

Loading

mrousavy commented May 22, 2024

tomekzaw commented May 22, 2024

tjzel commented May 27, 2024

piaskowyk May 29, 2024

tjzel Jun 10, 2024

piaskowyk May 29, 2024

tjzel Jun 10, 2024 •

edited

Loading

piaskowyk left a comment

tjzel commented Jun 19, 2024

tjzel commented Jun 19, 2024

Fix "Not implemented" exception on JSC #6028

Fix "Not implemented" exception on JSC #6028

Conversation

tjzel commented May 17, 2024 • edited by patrycjakalinska Loading

Summary

Test plan

tomekzaw left a comment

Choose a reason for hiding this comment

tjzel commented May 20, 2024

tomekzaw commented May 20, 2024 • edited Loading

mrousavy commented May 22, 2024

tomekzaw commented May 22, 2024

tjzel commented May 27, 2024

piaskowyk May 29, 2024

Choose a reason for hiding this comment

tjzel Jun 10, 2024

Choose a reason for hiding this comment

piaskowyk May 29, 2024

Choose a reason for hiding this comment

tjzel Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

piaskowyk left a comment

Choose a reason for hiding this comment

tjzel commented Jun 19, 2024

tjzel commented Jun 19, 2024

tjzel commented May 17, 2024 •

edited by patrycjakalinska

Loading

tomekzaw commented May 20, 2024 •

edited

Loading

tjzel Jun 10, 2024 •

edited

Loading