Handle scope validation tests

[jasongin]

Fixes #121

• Add unit tests for handle scope validations, using a mocked runtime so that the tests can run without libnode. [1]
  • Test rules for nesting different types of value scopes.
  • Test attempts to access values from disposed scope or from different thread.
  • Test attempts to access references from a different thread.
  • The checks should also cover attempts to access values/references from a disposed environment because they rely on the thread-local current scope that will be null (or different) after the environment root scope is closed. There are no explicit tests for this though.
• Refactor and fix handle checks in JSValue, JSValueScope, and JSReference to pass the tests.
• When making API calls related to a JSValue instance, always use the value's Scope member rather than the thread-local "current" scope. This allows for validation that the current and target environments (threads) are the same.
• Add new exception types JSValueScopeClosedException (subclass of ObjectDisposedException) and JSInvalidScopeException (subclass of InvalidOperationException) to provide detailed error info when a handle scope problem is detected.
• Pass a GC handle to the JSRuntimeContext via the hint parameter of all finalizers (which required some minor refactoring where the hint was used for other things). Previously the finalizers were getting the context via the thread-local scope, which caused errors when finalizers ran after the root scope was closed. Also the finalizers were calling napi_get_instance_data (many times) which should be avoided. Finalizers now do not make any JS calls and do not rely on any thread-statics.
• Fix a similar issue with JSReference.Dispose(), to handle disposing references after the root scope was closed.
• Fix a bug in generator tool response file parsing of an empty quoted value (""). This broke builds targeting .NET 4 where the --packs argument value is empty. (I don't know how this passed testing before.)

I compared benchmark results before and after the changes - there was no measurable impact from the additional validations.

[1] Currently I get a crash when trying to create more than one "environment" using the new embedding APIs. I will check whether that is fixed in a newer revision of that PR; I'm using a build of libnode from a much older PR revision.

[vmoroz]

I am concerned that PR removes all safety checks from JSValue.
After this change the Handle is never checked which will cause a lot of security risks of accessing released memory.
Am I reading this change wrong?

[jasongin]

I am concerned that PR removes all safety checks from JSValue. After this change the Handle is never checked which will cause a lot of security risks of accessing released memory. Am I reading this change wrong?

The checks are not removed, but moved to the JSValueScope to napi_env conversion. We could have them in both places but I thought that was redundant since you have to get a napi_env to do anything with a handle. I can explain in more detail when I have time later.

[vmoroz]

I am concerned that PR removes all safety checks from JSValue. After this change the Handle is never checked which will cause a lot of security risks of accessing released memory. Am I reading this change wrong?

The checks are not removed, but moved to the JSValueScope to napi_env conversion. We could have them in both places but I thought that was redundant since you have to get a napi_env to do anything with a handle. I can explain in more detail when I have time later.

The main goal of JSValue is to safely wrap napi_value. Replacing it for the napi_env check from JSValueContext is not equivalent. For the functions that use a single napi_value, I agree, there is no difference. But if a function uses two or more napi_value, then we effectively check only one of them, while others are used unchecked. It is dangerous if these values are from different scopes.
If we want to get rid of redundant napi_value/napi_env checks, then we should rather avoid checking for napi_env because it will be covered by napi_value check, but not the other way around.

[jasongin]

if a function uses two or more napi_value

Yes, that is a good point about additional napi_value args.

If we want to get rid of redundant napi_value/napi_env checks, then we should rather avoid checking for napi_env because it will be covered by napi_value check, but not the other way around.

For static functions that take only a napi_env and no value handles we still have to validate the thread access to the env via the current scope.

So we'll check in both places, and it will a little redundant sometimes, but it is only a boolean plus an nint comparison so it's not a big cost. From benchmarks of scenarios that have heavy interaction with handles, it looks like maybe 1-2% performance difference, though the benchmarks don't quite have that level of precision. Regardless I think is worth it for the added safety.

I suppose we could add a JSValueScope.UncheckedEnv handle property that could be used when we know the scope is already being checked via the value.

[vmoroz]

LGTM

[jasongin]

In the update I also added consistent public (checked) Handle and EnvironmentHandle properties; the (napi_value), (napi_env), and (napi_ref) operators now just call those. That follows framework API design guidelines that any operators should have non-operator equivalent properties or methods. And I added an internal UncheckedEnvironmentHandle property that can be used to bypass redundant checks when the scope / env handle will already be checked via value handle access in the same call.

[vmoroz]

        throw new InvalidOperationException("Parent scope must not be null.");

Should we use the JSInvalidScopeException in this function?

---

Refers to: src/NodeApi/JSValueScope.cs:390 in c4cc578. [](commit_id = c4cc578, deletion_comment = False)

[jasongin]

        throw new InvalidOperationException("Parent scope must not be null.");

Should we use the JSInvalidScopeException in this function?

JSInvalidScopeException was renamed to JSThreadAccessException at your suggestion. So then it didn't make sense to use for the parent scope checks here because they are not about threading. (JSInvalidScopeException.cs was mistakenly still included in the dif.)