EXC_BAD_ACCESS with Node 8.3, works with Node 6.11

[mcollina]

I've ported https://github.com/mcollina/native-hdr-histogram/tree/napi to node-addon-api. The tests passes with Node 6.11, but they fail with the following error on Node 8.3.0-rc.0:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xffffffff00000000)
    frame #0: 0x0000000100a58df0 node`(anonymous namespace)::v8impl::FunctionCallbackWrapper::SetReturnValue(napi_value__*) + 16
node`(anonymous namespace)::v8impl::FunctionCallbackWrapper::SetReturnValue:
->  0x100a58df0 <+16>: movq   (%rsi), %rcx
    0x100a58df3 <+19>: movq   %rcx, 0x18(%rax)
    0x100a58df7 <+23>: popq   %rbp
    0x100a58df8 <+24>: retq
(llnode) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xffffffff00000000)
  * frame #0: 0x0000000100a58df0 node`(anonymous namespace)::v8impl::FunctionCallbackWrapper::SetReturnValue(napi_value__*) + 16
    frame #1: 0x0000000100a5227f node`(anonymous namespace)::v8impl::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) + 307
    frame #2: 0x0000000100216122 node`v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) + 466
    frame #3: 0x000000010027761c node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 572
    frame #4: 0x0000000100277028 node`v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) + 232
    frame #5: 0x00003308176840dd
    frame #6: 0x0000330817689154
    frame #7: 0x000033081777596e
    frame #8: 0x0000330817692f12
    frame #9: 0x0000330817773fa1
    frame #10: 0x0000330817692f12
    frame #11: 0x00003308177738aa
    frame #12: 0x0000330817692f12
    frame #13: 0x0000330817686fbb
    frame #14: 0x000033081777373c
    frame #15: 0x0000330817692f12
    frame #16: 0x00003308177740f2
    frame #17: 0x0000330817692f12
    frame #18: 0x0000330817772fdb
    frame #19: 0x0000330817692f12
    frame #20: 0x0000330817773fa1
    frame #21: 0x0000330817692f12
    frame #22: 0x0000330817686fbb
    frame #23: 0x0000330817773a25
    frame #24: 0x0000330817692f12
    frame #25: 0x00003308177738aa
    frame #26: 0x0000330817692f12
    frame #27: 0x0000330817686fbb
    frame #28: 0x0000330817774242
    frame #29: 0x0000330817692f12
    frame #30: 0x0000330817772cc8
    frame #31: 0x0000330817692f12
    frame #32: 0x00003308177738aa
    frame #33: 0x0000330817692f12
    frame #34: 0x00003308177738aa
    frame #35: 0x0000330817692f12
    frame #36: 0x000033081777373c
    frame #37: 0x0000330817692f12
    frame #38: 0x0000330817774242
    frame #39: 0x0000330817692f12
    frame #40: 0x0000330817772cc8
    frame #41: 0x0000330817692f12
    frame #42: 0x0000330817793c29
    frame #43: 0x0000330817692f12
    frame #44: 0x0000330817773fa1
    frame #45: 0x0000330817692f12
    frame #46: 0x0000330817690650
    frame #47: 0x0000330817764ead
    frame #48: 0x00000001005c5dfd node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::Object>, v8::internal::Execution::MessageHandling) + 701
    frame #49: 0x00000001005c5a9b node`v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 171
    frame #50: 0x00000001001faf29 node`v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 441
    frame #51: 0x0000000100a4a7cf node`node::LoadEnvironment(node::Environment*) + 591
    frame #52: 0x0000000100a51832 node`node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) + 426
    frame #53: 0x0000000100a4c88f node`node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) + 462
    frame #54: 0x0000000100a4be83 node`node::Start(int, char**) + 333
    frame #55: 0x0000000100001134 node`start + 52

to reproduce, run node --napi-modules test.js after downloading and building the linked branch: https://github.com/mcollina/native-hdr-histogram/tree/napi

[kfarnung]

Thanks @mcollina for the report. I tried Windows and Ubuntu, but I'm not seeing that crash. I don't have a Mac handy right now to verify with, but I'll try to track one down.

[gabrielschulhof]

I did this:

• nvm install v8.3.0
• npm install
• node --napi-modules test.js

So far, it's stuck on "# Subtest: create an histogram arguments checks\nok 1 - expected to throw"

[gabrielschulhof]

This is on "macOS Sierra Version 10.12.2"

[gabrielschulhof]

Still stuck ... I'm going to interrupt it.

[mcollina]

Yes, and if you run that with lldb, you will get the error above.

[gabrielschulhof]

Aaah, OK. Will do.

[gabrielschulhof]

Reproduced.

[gabrielschulhof]

Built 8.3.0 with debug symbols and it runs successfully LOL

[gabrielschulhof]

That is, inside lldb.

[gabrielschulhof]

So it only happens with the official version.

[gabrielschulhof]

@jasongin the instability may arise from the fact that the constructor for the HdrHistogramWrap class throws an error during its construction. I've noticed that the C++ wrapper doesn't have any tests for ObjectWrap. We should add a test that implements an object wrapping such that the constructor throws a JS error.

[jasongin]

I've noticed that the C++ wrapper doesn't have any tests for ObjectWrap.

Yes, I agree test coverage is needed. It just hasn't been a top priority for me so far because ObjectWrap was tested fairly well already via the ported modules that use it (like canvas).

[addaleax]

@gabrielschulhof What you’re saying makes a lot of sense. I think this would be the fix for it:

diff --git a/napi-inl.h b/napi-inl.h
index 4573963422d9..bf64376072c6 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -2428,12 +2428,10 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
   }
 
   T* instance;
-  napi_value wrapper;
-  details::WrapCallback([&] {
+  napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
     instance = new T(callbackInfo);
-    wrapper = callbackInfo.This();
-    return nullptr;
+    return callbackInfo.This();
   });
 
   napi_ref ref;

The code should look like that anyway, I think. In its current state, wrapper would always be unitialized if the constructor threw.