src: Avoid calling into C++ with a null this

[chearon]

These tests all crash the process because instance is null when exceptions are off. If ObjectWrap methods try to access anything on this, it's bad news. I don't think the method should get called at all in this case since there's a pending JS exception anyways.

Context: I recently ported node-canvas to node-addon-api and we have tests for this. It was a pain in v8/NAN, and we could remove a lot of code with these changes. I had a positive experience working with node-addon-api, great work everyone!

[gabrielschulhof]

Looking good. Just a few minor comments.

[chearon]

Thanks for the review! Think I got everything except for the one I commented on.

[mhdawson]

LGTM

[mhdawson]

Landed in 858942c

[kraenhansen]

I'm posting here to keep the context of the PR and get some early feedback on my thoughts.
I'll happily create a new issue if I'm not overlooking something obvious.

I believe the instance checks in InstanceMethodCallbackWrapper and InstanceGetterCallbackWrapper should throw instead of returning undefined.

To me, it seems that the tests are relying on the ObjectWrap<T>::Unwrap to throw if failed. And while napi_unwrap's implementation indeed fails if the "local value" is not an object, this is not the case for an Addon declaring its methods via InstanceWrap<T>::InstanceMethod: When the Addon's Unwrap function is called

node-addon-api/napi-inl.h

Line 4576 in 7c3226f

T* instance = T::Unwrap(callbackInfo.This().As<Object>());

it simply defers to GetInstanceData

node-addon-api/napi-inl.h

Lines 6795 to 6798 in 7c3226f

	template <typename T>
	inline T* Addon<T>::Unwrap(Object wrapper) {
	return wrapper.Env().GetInstanceData<T>();
	}

which does report a fatal error (not throwing) if the status indicates a failure

node-addon-api/napi-inl.h

Lines 742 to 744 in 7c3226f

	napi_status status = napi_get_instance_data(_env, &data);
	NAPI_FATAL_IF_FAILED(
	status, "BasicEnv::GetInstanceData", "invalid arguments");

but the call to napi_get_instance_data will simply pass through nullptr values: https://github.com/nodejs/node/blob/804d41f9c73c03e670d8e788d14a2237c8c2a057/src/js_native_api_v8.cc#L3527

---

This results in some rather strange behavior where if an addon is instantiated via simple object construction instead of calling it's Init function, the native implementation of methods are never called and return values are simply undefined.

This is an example of the HelloAddon being constructed:

Napi::Env env{_env};
Napi::HandleScope {env};

Napi::Object exports = Napi::Object::New(env);
HelloAddon addon{env, exports};

// Log the properties of the exports object
for (auto prop: exports.GetPropertyNames()) {
  printf("exports.%s = %s\n",
         prop.second.operator Napi::Value().As<Napi::String>().Utf8Value().c_str(),
         exports.Get(prop.second).ToString().Utf8Value().c_str()
         );
}

// Test
auto result = exports.Get("hello").As<Napi::Function>().Call(exports, {});
printf("Result of hello function: %s\n", result.ToString().Utf8Value().c_str());
XCTAssertFalse(result.IsEmpty());
XCTAssertTrue(result.IsString());

This last assertion fails since result is undefined and the following is printed to stdout during execution:

exports.hello = function hello() { [native code] }
Result of hello function: undefined