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

Is it possible to determine the type of an NAPI::Object in a non-global context? #764

Closed
jschlight opened this issue Jul 13, 2020 · 7 comments

Comments

@jschlight
Copy link
Contributor

This question is related to an interesting issue raised on the node-sqlite3 project:

TryGhost/node-sqlite3#1355

The JavaScript code from the issue looks like:

const vm = require('vm');
const sqlite3 = require('sqlite3').verbose();
const db = new sqlite3.Database(':memory:');

const contextObject = { db, console };

db.each('select ?', new Date(), console.log);
vm.runInNewContext("db.each('select ?', [new Date()], console.log);", contextObject);

The pertinent code using node-addon-api looks like:

bool OtherInstanceOf(Napi::Object source, const char* object_type) {
    if (strncmp(object_type, "Date", 4) == 0) {
        return source.InstanceOf(source.Env().Global().Get("Date").As<Function>());
    } else if (strncmp(object_type, "RegExp", 6) == 0) {
        return source.InstanceOf(source.Env().Global().Get("RegExp").As<Function>());
    }
    return false;
}

The code to determine if source is a Date object is failing apparently because source has been allocated outside the global context.

The author of the node-sqlite3 issue has identified the IsDate method as a way to address this issue. But is there another more general approach that would work to identify type of an NAPI::Object in a non-global context?

@mhdawson
Copy link
Member

Object.InstanceOf ends up calling napi_instanceof which has the following implementation

napi_status napi_instanceof(napi_env env,
                            napi_value object,
                            napi_value constructor,
                            bool* result) {
  NAPI_PREAMBLE(env);
  CHECK_ARG(env, object);
  CHECK_ARG(env, result);

  *result = false;

  v8::Local<v8::Object> ctor;
  v8::Local<v8::Context> context = env->context();

  CHECK_TO_OBJECT(env, context, ctor, constructor);

  if (!ctor->IsFunction()) {
    napi_throw_type_error(env,
                          "ERR_NAPI_CONS_FUNCTION",
                          "Constructor must be a function");

    return napi_set_last_error(env, napi_function_expected);
  }

  napi_status status = napi_generic_failure;

  v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(object);
  auto maybe_result = val->InstanceOf(context, ctor);
  CHECK_MAYBE_NOTHING(env, maybe_result, status);
  *result = maybe_result.FromJust();
  return GET_RETURN_STATUS(env);
}

I assume the problem is that mismatch between the context that was active when the env was created and the context active when the call is made.

@gabrielschulhof, @addaleax some methods call v8::Local<v8::Context> context = env->isolate->GetCurrentContext(); instead of v8::Localv8::Context context = env->context();` do you remember off hand why we would use one in some places and the other for other cases?

I'm wondering if for napi_instanceof we should be using env->isolate->GetCurrentContext(); but then that lead to the question if why/when we would use env->context().

@addaleax
Copy link
Member

I assume the problem is that mismatch between the context that was active when the env was created and the context active when the call is made.

Yes. The instanceof operator checks whether the .prototype property of the right-hand side appears in the prototype chain of the left-hand side. Each vm Context comes with its own set of builtins, including Date and Date.prototype. As a consequence, the Date.prototype from one Context does not show up in the prototype chain of a Date instance from another context, and instanceof returns false. In particular, this is not specific to N-API.

do you remember off hand why we would use one in some places and the other for other cases?

I'm wondering if for napi_instanceof we should be using env->isolate->GetCurrentContext(); but then that lead to the question if why/when we would use env->context().

Currently, this is irrelevant because Node.js is not actually supporting multi-Context addons. Once we do, using env->isolate->GetCurrentContext() (or storing the context on the env itself) would be the right thing in a lot of situations. However, that mismatch is not related to this issue, passing either context would result in the same return value.

(For the most part, passing a context to V8 functions is relevant for cases in which a new Error object is being thrown – e.g. because the right-hand side isn’t a constructor.)

The author of the node-sqlite3 issue has identified the IsDate method as a way to address this issue. But is there another more general approach that would work to identify type of an NAPI::Object in a non-global context?

napi_typeof and the napi_is_... family are the only native brand checks that I know of. However, the relevant question is what you are actually wanting to find out about the object – if you want napi_get_date_value(), then yes, napi_is_date() will give you the right answer. But if you want to use some other API, e.g. date.getUTCDay(), then you’d best check for the presence of that method (since it can be overriden or removed, and napi_is_date does not provide that information).

@mhdawson
Copy link
Member

Thanks for the clarification on contexts. Would you agree that the 2 places we env->isolate->GetCurrentContext() should be updated to use env->context() to be consistent with the rest of the places we use the context?

@addaleax
Copy link
Member

Would you agree that the 2 places we env->isolate->GetCurrentContext() should be updated to use env->context() to be consistent with the rest of the places we use the context?

I think that would be fine, yes.

@gabrielschulhof
Copy link
Contributor

In summary, it sounds like proper support for native add-ons to correctly support vm.runInNewContext() needs to be built from the core on up.

@mhdawson
Copy link
Member

@jschlight I think this has been answered, is it ok to close ?

mhdawson added a commit to mhdawson/io.js that referenced this issue Nov 10, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>
mhdawson added a commit to nodejs/node that referenced this issue Nov 17, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #36068
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
codebytere pushed a commit to nodejs/node that referenced this issue Nov 22, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #36068
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@jschlight
Copy link
Contributor Author

Yes @mhdawson Thanks for the ping.

BethGriggs pushed a commit to nodejs/node that referenced this issue Dec 10, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #36068
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this issue Dec 10, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #36068
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this issue Dec 15, 2020
Refs: nodejs/node-addon-api#764
Improve the consistency of how we get a context
when needed. We generally used env->context() in N-API
but there were are few exceptions that this PR addresses.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #36068
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
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

4 participants