n-api: investigate breakage

[refack]

Check to see If this will turn the CI green, that will mean that #12240 breaks on windows
Now the suspect is lines L2186-L2187 in node_api.cc 8fbace1/#12246.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

n-api,test

[TimothyGu]

The failed test 332 is:

not ok 332 addons-napi/test_instanceof/test
  ---
  duration_ms: 0.231
  severity: fail
  stack: |-
    
    assert.js:82
      throw new assert.AssertionError({
      ^
    AssertionError: true === undefined
        at assertTrue (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\addons-napi\test_instanceof\test.js:16:17)
        at eval (eval at testFile (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\addons-napi\test_instanceof\test.js:38:3), <anonymous>:29:1)
        at testFile (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\addons-napi\test_instanceof\test.js:38:3)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\addons-napi\test_instanceof\test.js:42:1)
        at Module._compile (module.js:607:30)
        at Object.Module._extensions..js (module.js:618:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Module.runMain (module.js:643:10)

which is unrelated to test/addons/make-callback-recurse/test.js. It's more likely that this test failure comes from 8fbace1/#12246.

[TimothyGu]

(closed by accident)

[refack]

CI: https://ci.nodejs.org/job/node-test-pull-request/7275/
🤷
Meanwhile I'm building locally so I can repro & debug

[mscdex]

It does seem to be that commit/PR. Here is another run on current master: https://ci.nodejs.org/job/node-test-binary-windows/7558/

I wonder if something got changed after CI was run in that PR.

[gabrielschulhof]

I'd like to see which combination of value/constructor causes the napi_instanceof() assertion to fail. To that end, I have created https://github.com/gabrielschulhof/node/tree/fix-windows-build which introduces only https://github.com/nodejs/node/compare/master...gabrielschulhof:fix-windows-build?expand=1 just to see, but I can't start a CI on that. @refack can you start a Windows-only CI on that?

[gabrielschulhof]

Yaaaaay! It seems I can reproduce the problem locally - albeit on a super-slow Windows VM 🎉

[gabrielschulhof]

Sure enough, if I comment out the line that actually caches Symbol.hasInstance, the test passes 😕

[gabrielschulhof]

Added a bunch of printfs, and I'm StrictEqualsing a freshly retrieved Symbol.hasInstance with the contents of the persistent. Unfortunately my round trip time is like 44 minutes :(

[refack]

Unfortunately my round trip time is like 44 minutes :(

I'm able to repro on my machine. send me your current diff, I believe my round trip will be much shorter.

[gabrielschulhof]

Just pull from https://github.com/gabrielschulhof/node/tree/fix-windows-build

[gabrielschulhof]

@refack Thanks!

[gabrielschulhof]

Wow! It looks like the persistent works if I keep retrieving the symbol, even if I don't use the freshly retrieved value. Now I'm confused.

[gabrielschulhof]

My latest suspicion:

https://github.com/nodejs/node/blob/master/src/node_api.cc#L2188
https://github.com/nodejs/node/blob/master/src/node_api.cc#L2195

In these two places we do if (status != napi_ok) return status; even though we didn't previously make a N-API call. In fact, in the second place we're checking status uninitialized.

[gabrielschulhof]

... and the second place is that runs most often, because we will have the symbol cached after the first time every time. Running my slow turnaround now.

[gabrielschulhof]

I'm surprised that using an uninitialized variable in an if-statement hasn't generated a warning.

[gabrielschulhof]

Would a macro like this:

#define NAPI_CALL(theCall)                \
  do {                                    \
    napi_status status = theCall;         \
    if (status != napi_ok) return status; \
  } while(0)

get optimized down to what we have now without a perf hit? If so, I will replace all this status retrieval and checking.

[gabrielschulhof]

#12283 should fix the problem. @refack could you please run a CI on it?

[refack]

#12283 should fix the problem. @refack could you please run a CI on it?

Ha 😄 I got to the same conclusion (8db5a0b)

[refack]

Made obsolete by #12283