Skip to content

objectwrap: gracefully handle constructor exceptions #600

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.

Sign up for GitHub

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

Closed
gabrielschulhof wants to merge 4 commits into from
Closed

objectwrap: gracefully handle constructor exceptions #600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cf3755
objectwrap: gracefully handle constructor exceptions
Nov 14, 2019
b00638c
remove static cast in constructor
Jan 1, 2020
868b4bd
perform some renames to better reflect semantics
Jan 2, 2020
bd465de
test for a specific exception
Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
perform some renames to better reflect semantics
  • Loading branch information
Gabriel Schulhof committed Jan 7, 2020
commit 868b4bd1f003897e9f086a9adb75182041619964
32 changes: 16 additions & 16 deletions napi-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2702,35 +2702,35 @@ inline Object FunctionReference::New(const std::vector<napi_value>& args) const
// CallbackInfo class
////////////////////////////////////////////////////////////////////////////////

class ObjectWrapCleanup {
class ObjectWrapConstructionContext {
public:
ObjectWrapCleanup(CallbackInfo* info) {
info->_object_wrap_cleanup = this;
ObjectWrapConstructionContext(CallbackInfo* info) {
info->_objectWrapConstructionContext = this;
}

static inline void MarkWrapOK(const CallbackInfo& info) {
if (info._object_wrap_cleanup == nullptr) {
Napi::Error::Fatal("ObjectWrapCleanup::MarkWrapOK",
"_object_wrap_cleanup is NULL");
static inline void SetObjectWrapped(const CallbackInfo& info) {
if (info._objectWrapConstructionContext == nullptr) {
Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped",
"_objectWrapConstructionContext is NULL");
}
info._object_wrap_cleanup->_wrap_worked = true;
info._objectWrapConstructionContext->_objectWrapped = true;
}

inline void RemoveWrap(const CallbackInfo& info) {
if (_wrap_worked) {
inline void Cleanup(const CallbackInfo& info) {
if (_objectWrapped) {
napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr);

// There's already a pending exception if we are at this point, so we have
// no choice but to fatally fail here.
NAPI_FATAL_IF_FAILED(status,
"ObjectWrapCleanup::RemoveWrap",
"ObjectWrapConstructionContext::Cleanup",
"Failed to remove wrap from unsuccessfully "
"constructed ObjectWrap instance");
}
}

private:
bool _wrap_worked = false;
bool _objectWrapped = false;
};

inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info)
Expand Down Expand Up @@ -3140,7 +3140,7 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref);
NAPI_THROW_IF_FAILED_VOID(env, status);

ObjectWrapCleanup::MarkWrapOK(callbackInfo);
ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo);
Reference<Object>* instanceRef = this;
*instanceRef = Reference<Object>(env, ref);
}
Expand Down Expand Up @@ -3716,21 +3716,21 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(

napi_value wrapper = details::WrapCallback([&] {
CallbackInfo callbackInfo(env, info);
ObjectWrapCleanup cleanup(&callbackInfo);
ObjectWrapConstructionContext constructionContext(&callbackInfo);
#ifdef NAPI_CPP_EXCEPTIONS
try {
new T(callbackInfo);
} catch (const Error& e) {
// Re-throw the error after removing the failed wrap.
cleanup.RemoveWrap(callbackInfo);
constructionContext.Cleanup(callbackInfo);
throw e;
}
#else
T* instance = new T(callbackInfo);
if (callbackInfo.Env().IsExceptionPending()) {
// We need to clear the exception so that removing the wrap might work.
Error e = callbackInfo.Env().GetAndClearPendingException();
cleanup.RemoveWrap(callbackInfo);
constructionContext.Cleanup(callbackInfo);
e.ThrowAsJavaScriptException();
delete instance;
}
Expand Down
6 changes: 3 additions & 3 deletions napi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ namespace Napi {
class CallbackInfo;
class TypedArray;
template <typename T> class TypedArrayOf;
class ObjectWrapCleanup;
class ObjectWrapConstructionContext;

typedef TypedArrayOf<int8_t> Int8Array; ///< Typed-array of signed 8-bit integers
typedef TypedArrayOf<uint8_t> Uint8Array; ///< Typed-array of unsigned 8-bit integers
Expand Down Expand Up @@ -1403,7 +1403,7 @@ namespace Napi {

class CallbackInfo {
public:
friend class ObjectWrapCleanup;
friend class ObjectWrapConstructionContext;
CallbackInfo(napi_env env, napi_callback_info info);
~CallbackInfo();

Expand All @@ -1429,7 +1429,7 @@ namespace Napi {
napi_value _staticArgs[6];
napi_value* _dynamicArgs;
void* _data;
ObjectWrapCleanup* _object_wrap_cleanup;
ObjectWrapConstructionContext* _objectWrapConstructionContext;
};

class PropertyDescriptor {
Expand Down
9 changes: 1 addition & 8 deletions test/objectwrap_constructor_exception.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,8 @@ const assert = require('assert');

const test = (binding) => {
const { ConstructorExceptionTest } = binding.objectwrapConstructorException;
let gotException = false;
try {
new ConstructorExceptionTest();
} catch (anException) {
gotException = true;
}
assert.throws(() => (new ConstructorExceptionTest()));
Copy link
Copy Markdown
Member

@mhdawson mhdawson Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be improved to validate that we get the same exception as we expect?

Copy link
Copy Markdown
Contributor Author

@gabrielschulhof gabrielschulhof Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do.

global.gc();

assert.strictEqual(gotException, true);
}

test(require(`./build/${buildType}/binding.node`));
Expand Down