Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nan tests hit DCHECKs on latest node #858

Open
MarshallOfSound opened this issue May 29, 2019 · 2 comments
Open

nan tests hit DCHECKs on latest node #858

MarshallOfSound opened this issue May 29, 2019 · 2 comments

Comments

@MarshallOfSound
Copy link
Member

MarshallOfSound commented May 29, 2019

I discovered this while running the nan test suite against Electron on our CI (we run tests with DCHECKs enabled). Just to cover my bases I ran the same nan tests against a Debug build of node and the same DCHECKs were hit.

#
# Fatal error in ../deps/v8/src/api-inl.h, line 126
# Debug check failed: allow_empty_handle || that != nullptr.
#
#
#
#FailureMessage Object: 0x7ffeefbfc2a0not ok test/js/nannew-test.js ......................... 22/23
    Command: "/Users/sattard/projects/nodejs/node/out/Debug/node nannew-test.js"
    TAP version 13
    ok 1 ../cpp/nannew.cpp:61: New<Array>()->Length() == 0
    ok 2 ../cpp/nannew.cpp:62: New<Array>(7)->Length() == 7
    ok 3 ../cpp/nannew.cpp:63: assertType<Array>(New<Array>(7))
    ok 4 ../cpp/nannew.cpp:73: New<Boolean>(true)->Value() == true
    ok 5 ../cpp/nannew.cpp:74: New<Boolean>(false)->Value() == false
    ok 6 ../cpp/nannew.cpp:75: assertType<Boolean>( New<Boolean>(true))
    ok 7 ../cpp/nannew.cpp:77: New(true)->Value() == true
    ok 8 ../cpp/nannew.cpp:78: New(false)->Value() == false
    ok 9 ../cpp/nannew.cpp:79: assertType<Boolean>( New(true))
    ok 10 ../cpp/nannew.cpp:94: assertType<BooleanObject>( New<BooleanObject>(true))
    ok 11 ../cpp/nannew.cpp:95: New<BooleanObject>(true)->ValueOf() == true
    ok 12 ../cpp/nannew.cpp:96: New<BooleanObject>(false)->ValueOf() == false
    ok 13 ../cpp/nannew.cpp:106: assertType<Context>( New<Context>())
    ok 14 ../cpp/nannew.cpp:108: assertType<Context>( New<Context>(&extensions))
    ok 15 ../cpp/nannew.cpp:111: assertType<Context>( New<Context>(static_cast<ExtensionConfiguration *>(__null) , Local<ObjectTemplate>()))
    ok 16 ../cpp/nannew.cpp:113: assertType<Context>( New<Context>(&extensions, Local<ObjectTemplate>()))
    ok 17 ../cpp/nannew.cpp:116: assertType<Context>( New<Context>(&extensions , Local<ObjectTemplate>(), Local<Value>()))
    ok 18 ../cpp/nannew.cpp:127: assertType<Date>( New<Date>(static_cast<double>(time(__null))).ToLocalChecked())
    ok 19 ../cpp/nannew.cpp:139: New<External>(&ttt)->Value() == &ttt
    ok 20 ../cpp/nannew.cpp:140: assertType<External>(New<External>(&ttt))
    ok 21 ../cpp/nannew.cpp:149: assertType<Function>(New<Function>(testFunction))
    ok 22 ../cpp/nannew.cpp:151: assertType<Function>(New<Function>(testFunction, data))
    not ok 23 test/js/nannew-test.js
      ---
        exit:    ~
        signal:  SIGILL
        stderr:  |
          #
          # Fatal error in ../deps/v8/src/api-inl.h, line 126
          # Debug check failed: allow_empty_handle || that != nullptr.
          #
          #
          #
          #FailureMessage Object: 0x7ffeefbfc2a0
        command: "/Users/sattard/projects/nodejs/node/out/Debug/node nannew-test.js"
      ...

    1..23
    # tests 23
    # pass  22
    # fail  1

The same tests pass in Release builds (as DCHECKs are disabled) but I think some time should be spent figuring out what is going wrong here as V8 clearly isn't happy with what is happening 🤔

To reproduce with a local build of node with a directory structure of

nodejs
  / node
  / nan
# Build node + headers
cd nodejs/node
./configure --debug
make -j4
make tar-headers
tar -xzf node-v13.0.0-headers.tar.gz

cd ../nan
# Build nan test modules with right headers
npm_config_nodedir=$(pwd)/../node/node-v13.0.0 npx node-gyp rebuild --directory test
# Run nan tests with built version of node
../node/out/Debug/node node_modules/.bin/tap test/js/*-test.js

Note that although the example above uses HEAD of node (v13) I have reproed with v12 as well (which is what Electron currently ships)

@kkoopa
Copy link
Collaborator

kkoopa commented May 30, 2019

Based on the progression of test cases, I would guess it is https://github.com/nodejs/nan/blob/master/test/cpp/nannew.cpp#L161 that triggers it, but I do not see why.

@kkoopa
Copy link
Collaborator

kkoopa commented May 30, 2019

Might no logger be allowed to create empty handles, at least not of FunctionTemplate. Utils::OpenHandle defaults to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants