test: fix unreliable assumption in js-native-api/test_cannot_run_js

[joyeecheung]

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that.

[targos]

LGTM as a hotfix, but the test will basically become useless once V8 12.2 lands, as it will always return napi_ok then.

[joyeecheung]

but the test will basically become useless once V8 12.2 lands, as it will always return napi_ok then.

That's not certain, it really depends on what the heap looks like when the addon is loaded. Any subtle change to the internals can delay the GC to happen after the check callbacks are called in the event loop (or not happen at all), and then it'll be napi_cannot_run_js again (and become a flaky nightmare). The fix just makes sure that it works for all cases (no matter GC kicks in before or after check callbacks, or if it doesn't happen at all)

[richardlau]

If this fixes the test, perhaps remove the flaky designation in

node/test/root.status

Line 2 in f28ccd3

js-native-api/test_cannot_run_js/test: PASS,FLAKY

?

[joyeecheung]

If this fixes the test, perhaps remove the flaky designation in

Updated the status file - was the designation even working in the first place? It still got caught in the CI it seems, and locally the test runner still reports it. (I think it should've been marked as PASS,CRASH instead?)

[richardlau]

If this fixes the test, perhaps remove the flaky designation in

Updated the status file - was the designation even working in the first place? It still got caught in the CI it seems, and locally the test runner still reports it. (I think it should've been marked as PASS,CRASH instead?)

Yes, it was working -- e.g. https://ci.nodejs.org/job/node-test-commit-ibmi/nodes=ibmi73-ppc64/1466/ (from #48180 (comment)) is yellow due to this test crashing and not red.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57467/

[richardlau]

It looks like the addon is failing to compile on Windows:
e.g. https://ci.nodejs.org/job/node-test-binary-windows-native-suites/21625/nodes=win10-vs2019-COMPILED_BY-vs2022/console

17:10:21 Failed to build addon in c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js:
17:10:21 Building the projects in this solution one at a time. To enable parallel build, please add the "-m" switch.

17:10:21   test_cannot_run_js.c

17:10:21 c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\test_cannot_run_js.c(28,1): error C2121: '#': invalid character: possibly the result of a macro expansion [c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\build\test_cannot_run_js.vcxproj]

[StefanStojanovic]

It looks like the addon is failing to compile on Windows

I tried this locally and can confirm the behavior. What fixed the compilation for me was this. @joyeecheung if you think it's good, feel free to apply it.

diff --git a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
index 925a7117d06..19cc7ae9058 100644
--- a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -21,12 +21,13 @@ static void Finalize(napi_env env, void* data, void* hint) {
   // or during environment shutdown (where napi_cannot_run_js or
   // napi_pending_exception is returned). This is not deterministic from
   // the point of view of the addon.
-  NODE_API_NOGC_ASSERT_RETURN_VOID(
 #ifdef NAPI_EXPERIMENTAL
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_cannot_run_js || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_cannot_run_js");
 #else
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_pending_exception || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_pending_exception");

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57508/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57511/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57516/

[nodejs-github-bot]

Landed in 30c9181