Napi::TypeError::New is not an instanceof TypeError

[rynz]

Consider:

Napi::Value Rwar(const Napi::CallbackInfo& info) {
  Napi::Env env = info.Env();
  Napi::TypeError::New(env, "Wrong number of arguments")
      .ThrowAsJavaScriptException();
  return env.Null();
}

test('throws wrong number of arguments', () => {
  expect.assertions(1)
  try {
    addon.rwar()
  } catch (err) {
    expect(err).toBeInstanceOf(TypeError)
  }
})

For whatever reason I am getting:

● throws wrong number of arguments

    expect(received).toBeInstanceOf(expected)

    Expected constructor: TypeError
    Received constructor: TypeError

       6 |     addon.rwar()
       7 |   } catch (err) {
    >  8 |     expect(err).toBeInstanceOf(TypeError)
         |                 ^
       9 |   }
      10 | })
      11 | 

Whenever I check err instanceof Error or err instanceof TypeError both return false. Is there something I am doing wrong, or is this a bug?

[mhdawson]

I don't see anything obviously wrong with the node-addon-api or N-API code in core, looks like they should create a TypeError. What kind of object are you receiving and any more info in it that might help figure out what is going on?

[rynz]

@mhdawson I've created an example rynz/node-addon-api-exception-example

That's the thing, it is receiving an Error/TypeError and the prototype chain looks okay. I've added a test to compare N-API vs Javascript debug.

  node-addon-api
    ✕ throws wrong number of arguments (2 ms)
  javascript
    ✓ throws wrong number of arguments (1 ms)

  ● node-addon-api › throws wrong number of arguments

    expect(received).toBeInstanceOf(expected)

    Expected constructor: TypeError
    Received constructor: TypeError

       8 |     } catch (err) {
       9 |       debugger
    > 10 |       expect(err).toBeInstanceOf(TypeError)
         |                   ^
      11 |     }
      12 |   })
      13 | })

[mhdawson]

Thanks for putting together the example, I'll try to take a look next week.

[NickNaso]

Hi everybody,
today I did some attempts using the example prepared by @rynz.
I created a simple index.js file like reported below:

'use strict'

const addon = require('./lib/rwar')
const assert = require('assert')

try {
  addon.rwar()
} catch (err) {
  assert.strictEqual(typeof TypeError == 'function', true)
  assert.strictEqual(err instanceof TypeError, true)
  assert.strictEqual(err instanceof Error, true)
  assert.strictEqual(typeof err.constructor === 'function' && err.constructor == TypeError, true)
  assert.strictEqual(typeof err.constructor === 'function' && err.constructor == Error, false)
}

I tried to replicate the checks that jest do on toBeInstanceOf and they produced the expected results.

[mhdawson]

I was also just looking at this. Using plain old instanceof gives the expected answer:

const addon = require('../rwar')

try {
  addon.rwar()
} catch (err) {
  console.log(err);
  console.log(err instanceof TypeError);
}

-sh-4.2$ node test.js
TypeError: Wrong number of arguments
at Object. (/home/mhdawson/test/node-addon-api-exception-example/lib/tests/test.js:4:9)
at Module._compile (internal/modules/cjs/loader.js:1217:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1238:10)
at Module.load (internal/modules/cjs/loader.js:1066:32)
at Function.Module._load (internal/modules/cjs/loader.js:954:14)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
at internal/main/run_main_module.js:17:47
true

[NickNaso]

I'm trying to execute the some tests with other test suite like:

• node-tap:

'use strict'

const tap = require('tap')
const addon = require('./lib/rwar')

const test = tap.test

test('test 1', (t) => {
  try {
    addon.rwar()
  } catch (err) {
    t.match(err, TypeError)
  }
  t.done()
})

test('test 2', (t) => {
  const fn = function() {
    return addon.rwar()
  }
  t.throws(fn, TypeError)
  t.done()
})

• jasmine

'use strict'

const addon = require('./lib/rwar')
describe('Test addon', function () {
  it('Schould be TypeError', function() {
    try {
      addon.hello()
    } catch (err) {
      expect(err).toBeInstanceOf(Error)
      expect(err).toBeInstanceOf(TypeError)
    }
  })
})

All continue to work as expected. Do you know if with jest we need some particular configuration?

[mhdawson]

I'm wondering if jest does some instrumentation of TypeError that's getting in the way.

[NickNaso]

Hi everybody,
I provided to open an issue on jest repo hope that hey give us some suggestion about. The issue is jestjs/jest#10189

[mhdawson]

@NickNaso great idea, I think we need somebody with knowledge of jest to comment.

[mhdawson]

The root cause may be related to the same issue in #764. The effects of JEST using different contexts.

[mhdawson]

Does seems related to JEST using contexts. This test recreates the behavior reported:

const addon = require('../rwar')
const vm = require('vm');

try {
  addon.rwar();
} catch (err) {
  console.log('Regular context:' +  (err instanceof TypeError));
}

try {
  const contextObject = { };
  vm.runInNewContext("addon.rwar()", contextObject);
} catch (err) {
  console.log('New context:' + (err instanceof TypeError));
}

[mhdawson]

@rynz you can read a bit more on the details in #764 but this seems to be an expected result of Node.js (not specific to N-API) not supporting multi-Context addons and the use of vm.runInNewContext by Jest.

[rynz]

@mhdawson thank you. What is the use-case of multi-context with node.js?

[mhdawson]

In terms of "multi-context" I think "context" might be a bit overloaded. I do know that there is/was work to let add-ons work nicely with Worker Threads such that you have ways to easily store data which is not global but instead specific to the worker thread from which the add-on is called. Sometimes I think I've seen this referred to as multi-context or something similar as well. That's one use case.

In terms of the question you raised in this issue, the use case is doing things like calling in vm.runInNewContext and getting what you expect if it had been JavaScript code versus native code.

Not sure this helped to clarify. @gabrielschulhof do you have a better explanation?

[mhdawson]

Some discussion in the N-API meeting today and its a bit less clear as I may not have gotten a good test case. Will try to update the test case a bit later today.

[mhdawson]

Ok so the test case I had above was missing the right version (which I had checked as well) to replicate. So the move complete test case is:

[mhdawson]

const addon = require('../rwar')
const vm = require('vm');

try {
  addon.rwar();
} catch (err) {
  console.log('Regular context:' +  (err instanceof TypeError));
}

try {
  const contextObject = { };
  vm.runInNewContext("addon.rwar()", contextObject);
} catch (err) {
  console.log('New context:' + (err instanceof TypeError));
}

const contextObject = { console };
vm.runInNewContext('try {addon.rwar() } catch (err) {console.log("New context:" + (err instanceof TypeError))}', contextObject);

[mhdawson]

And more complete with an explanation of the expected results for each case. Still same conclusion that the native addon not being aware of the context when run under runInNewContext is related to the reported issue.

const addon = require('../rwar')
const vm = require('vm');
const contextObject = { console };

/////////////////////////
// plain JavaScript
/////////////////////////
console.log('Plain JavaScript');

// expect err to be instnace of TypeError as only one context
try {
  throw new TypeError();
} catch (err) {
  console.log('Regular context:' +  (err instanceof TypeError));
}

// Don't expect err to be instance of TypeError as it was
// created in a different context from the context in which the
// check was made
try {
  vm.runInNewContext("throw new TypeError()", contextObject);
} catch (err) {
  console.log('New context:' + (err instanceof TypeError));
}

// expect err to be instance of TypeError as check is in
// same context as where err wsa created
vm.runInNewContext('try {throw new TypeError()} catch (err) {console.log("New context:" + (err instanceof TypeError))}', contextObject);

/////////////////////////
// Addons
/////////////////////////
console.log();
console.log('Addons');

// expect err to be instnace of TypeError as only one context
try {
  addon.rwar();
} catch (err) {
  console.log('Regular context:' +  (err instanceof TypeError));
}

// Don't expect err to be instance of TypeError as it was
// created in a different context from the context in which the
// check was made
try {
  vm.runInNewContext("addon.rwar()", contextObject);
} catch (err) {
  console.log('New context:' + (err instanceof TypeError));
}

// expect err to be instance of TypeError as check is in
// same context as where err wsa created
vm.runInNewContext('try {addon.rwar() } catch (err) {console.log("New context:" + (err instanceof TypeError))}', contextObject);

with the output being:

bash-4.2$ node test.js
Plain JavaScript
Regular context:true
New context:false
New context:true

Addons
Regular context:true
New context:false
New context:false

where the last one recreates the different between plain JavaScript and an exception thrown in a native, checked in JavaScript while running under runInNewContext

[mhdawson]

@gabrielschulhof updated test case as discussed in the N-API team meeting today.

From some additional experimental is seems like for addons the context used to create the error when a native method is run under vm.runInNewContext is not the context passed in, nor the context outside of the vm.runInNewContext

For example

const copyOfTypeError = TypeError;
const contextObject = { console, TypeError, copyOfTypeError };
vm.runInNewContext('try {addon.rwar() } catch (err) {console.log("New context:" + (err instanceof copyOfTypeError))}', contextObject);

Still results in false, where as doing the same for plan JavaScript reports true.

[rynz]

Fantastic work @mhdawson!

[gabrielschulhof]

@mhdawson I also did some testing, and it looks like the addon always produces objects descended from the built-ins present in the context of its creation:

#include <node_api.h>
#include "../../js-native-api/common.h"

static napi_value newError(napi_env env, napi_callback_info info) {
  size_t argc = 1;
  napi_value message, error;
  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &message, NULL, NULL));
  NAPI_CALL(env, napi_create_error(env, NULL, message, &error));
  return error;
}

NAPI_MODULE_INIT() {
  napi_property_descriptor methods[] = {
    DECLARE_NAPI_PROPERTY("newError", newError),
  };

  NAPI_CALL(env, napi_define_properties(
      env, exports, sizeof(methods) / sizeof(methods[0]), methods));

  return exports;
}

'use strict';

const binding = require(`./build/${common.buildType}/test_vm_context`);
const vm = require('vm');

const jsError = new Error('JS Error');
const nativeError = binding.newError('Native Error');

console.log('js:');
console.log('  created in: normal context, checked in: normal context: ' +
  (jsError instanceof Error));
console.log('  created in: normal context, checked in: vm context:     ' +
  vm.runInNewContext('jsError instanceof Error', { jsError }));
console.log('  created in: vm context,     checked in: normal context: ' +
  (vm.runInNewContext('new Error(\'JS Error\');') instanceof Error));
console.log('  created in: vm context,     checked in: vm context:     ' +
  vm.runInNewContext('(new Error(\'JS Error\')) instanceof Error;'));

console.log('');

console.log('native:');
console.log('  created in: normal context, checked in: normal context: ' +
  (nativeError instanceof Error));
console.log('  created in: normal context, checked in: vm context:     ' +
  vm.runInNewContext('nativeError instanceof Error', { nativeError }));
console.log('  created in: vm context,     checked in: normal context: ' +
  (vm.runInNewContext('binding.newError(\'nativeError\');', { binding })
    instanceof Error));
console.log('  created in: vm context,     checked in: vm context:     ' +
  vm.runInNewContext('binding.newError(\'nativeError\') instanceof Error;',
    { binding }));

and the output was:

js:
  created in: normal context, checked in: normal context: true
  created in: normal context, checked in: vm context:     false
  created in: vm context,     checked in: normal context: false
  created in: vm context,     checked in: vm context:     true

native:
  created in: normal context, checked in: normal context: true
  created in: normal context, checked in: vm context:     false
  created in: vm context,     checked in: normal context: true
  created in: vm context,     checked in: vm context:     false

Notice that I had to pass binding into the vm context. Ideally, I would vm.runInNewContext('require("/path/to/binding").newError("Native Error");'), meaning I would load a copy of the native add-on inside the vm context. require() is currently not defined in the vm context. I even tried passing the require into the vm context, but still, the add-on created instances descended from the built-ins of the outside context.

Basically, NAPI_ADDON_INIT() would have to be called again inside the vm context. This does not currently happen. I guess this is what @addaleax meant.

[gabrielschulhof]

I looked at the V8 documentation and, AFAICT it doesn't even have the facilities for creating a context-sensitive error:

https://v8docs.nodesource.com/node-14.1/da/d6a/classv8_1_1_exception.html

[mhdawson]

@gabrielschulhof

I looked at the V8 documentation and, AFAICT it doesn't even have the facilities for creating a context-sensitive error:

from the limited time I'd taken to look at it, I was thinking the same thing since the creation of TypeError had no way to pass in a context.

[mhdawson]

I did wonder though if there was some other way that the current context used by TypeError() could be set on the isolate or through some other way but had not had time to look at the code for that.

[addaleax]

@mhdawson @gabrielschulhof You could enter another context while creating the error object, if you want, but that’s probably not a good approach to take here, partially because N-API functions are more or less baked in stone and partially because this is not a typical use case (and because it would be better addressed through actually making N-API addons context-sensitive).

What you can do in this situation is passing the TypeError function from a context directly and creating an instance of that using napi_new_instance, that should not be an issue.

[mhdawson]

and because it would be better addressed through actually making N-API addons context-sensitive

+1

I think @gabrielschulhof and myself were just tying to think about how you might do that, as we don't have a good enough understanding of that yet.

[mhdawson]

From the discussion in #764 it sound like this is the expected behaviour for native addons when using vm.runInNewContext() and is not specific to node-addon-api.

@rynz is ok if we close this out?

[mhdawson]

Going to close this out, please let us know if this is not the right thing to do.