Skip to content

Commit e59cf89

Browse files
kkoopakkoopa
committed
Merge pull request #524 from gagern/UnwrapHolder
Pass Holder instead of This to ObjectWrap::Unwrap
2 parents 367e82a + 13a9931 commit e59cf89

File tree

9 files changed

+53
-26
lines changed

9 files changed

+53
-26
lines changed

‎doc/object_wrappers.md‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,18 @@ class ObjectWrap {
5454
5555
See the Node documentation on [Wrapping C++ Objects](https://nodejs.org/api/addons.html#addons_wrapping_c_objects) for more details.
5656
57+
### This vs. Holder
58+
59+
When calling `Unwrap`, it is important that the argument is indeed some JavaScript object which got wrapped by a `Wrap` call for this class or any derived class.
60+
The `Signature` installed by [`Nan::SetPrototypeMethod()`](methods.md#api_nan_set_prototype_method) does ensure that `info.Holder()` is just such an instance.
61+
In Node 0.12 and later, `info.This()` will also be of such a type, since otherwise the invocation will get rejected.
62+
However, in Node 0.10 and before it was possible to invoke a method on a JavaScript object which just had the extension type in its prototype chain.
63+
In such a situation, calling `Unwrap` on `info.This()` will likely lead to a failed assertion causing a crash, but could lead to even more serious corruption.
64+
65+
On the other hand, calling `Unwrap` in an [accessor](methods.md#api_nan_set_accessor) should not use `Holder()` if the accessor is defined on the prototype.
66+
So either define your accessors on the instance template,
67+
or use `This()` after verifying that it is indeed a valid object.
68+
5769
### Examples
5870
5971
#### Basic
@@ -93,12 +105,12 @@ class MyObject : public Nan::ObjectWrap {
93105
}
94106
95107
static NAN_METHOD(GetHandle) {
96-
MyObject* obj = Nan::ObjectWrap::Unwrap<MyObject>(info.This());
108+
MyObject* obj = Nan::ObjectWrap::Unwrap<MyObject>(info.Holder());
97109
info.GetReturnValue().Set(obj->handle());
98110
}
99111
100112
static NAN_METHOD(GetValue) {
101-
MyObject* obj = Nan::ObjectWrap::Unwrap<MyObject>(info.This());
113+
MyObject* obj = Nan::ObjectWrap::Unwrap<MyObject>(info.Holder());
102114
info.GetReturnValue().Set(obj->value_);
103115
}
104116
@@ -168,7 +180,7 @@ class MyFactoryObject : public Nan::ObjectWrap {
168180
}
169181

170182
static NAN_METHOD(GetValue) {
171-
MyFactoryObject* obj = ObjectWrap::Unwrap<MyFactoryObject>(info.This());
183+
MyFactoryObject* obj = ObjectWrap::Unwrap<MyFactoryObject>(info.Holder());
172184
info.GetReturnValue().Set(obj->value_);
173185
}
174186

‎test/cpp/accessors.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,13 @@ NAN_MODULE_INIT(SetterGetter::Init) {
4848
tpl->SetClassName(Nan::New<v8::String>("SetterGetter").ToLocalChecked());
4949
tpl->InstanceTemplate()->SetInternalFieldCount(1);
5050
SetPrototypeMethod(tpl, "log", SetterGetter::Log);
51-
v8::Local<v8::ObjectTemplate> proto = tpl->PrototypeTemplate();
51+
v8::Local<v8::ObjectTemplate> itpl = tpl->InstanceTemplate();
5252
SetAccessor(
53-
proto
53+
itpl
5454
, Nan::New("prop1").ToLocalChecked()
5555
, SetterGetter::GetProp1);
5656
SetAccessor(
57-
proto
57+
itpl
5858
, Nan::New<v8::String>("prop2").ToLocalChecked()
5959
, SetterGetter::GetProp2
6060
, SetterGetter::SetProp2
@@ -88,7 +88,7 @@ NAN_METHOD(SetterGetter::New) {
8888

8989
NAN_GETTER(SetterGetter::GetProp1) {
9090
SetterGetter* settergetter =
91-
ObjectWrap::Unwrap<SetterGetter>(info.This());
91+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
9292
assert(strlen(settergetter->log) < sizeof (settergetter->log));
9393
strncat(
9494
settergetter->log
@@ -110,7 +110,7 @@ NAN_GETTER(SetterGetter::GetProp1) {
110110

111111
NAN_GETTER(SetterGetter::GetProp2) {
112112
SetterGetter* settergetter =
113-
ObjectWrap::Unwrap<SetterGetter>(info.This());
113+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
114114
assert(strlen(settergetter->log) < sizeof (settergetter->log));
115115
strncat(
116116
settergetter->log
@@ -132,7 +132,7 @@ NAN_GETTER(SetterGetter::GetProp2) {
132132

133133
NAN_SETTER(SetterGetter::SetProp2) {
134134
SetterGetter* settergetter =
135-
ObjectWrap::Unwrap<SetterGetter>(info.This());
135+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
136136
strncpy(
137137
settergetter->prop2
138138
, *Utf8String(value)
@@ -157,7 +157,7 @@ NAN_SETTER(SetterGetter::SetProp2) {
157157

158158
NAN_METHOD(SetterGetter::Log) {
159159
SetterGetter* settergetter =
160-
ObjectWrap::Unwrap<SetterGetter>(info.This());
160+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
161161

162162
info.GetReturnValue().Set(Nan::New(settergetter->log).ToLocalChecked());
163163
}

‎test/cpp/accessors2.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ NAN_METHOD(SetterGetter::New) {
8686

8787
NAN_GETTER(SetterGetter::GetProp1) {
8888
SetterGetter* settergetter =
89-
ObjectWrap::Unwrap<SetterGetter>(info.This());
89+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
9090
assert(strlen(settergetter->log) < sizeof (settergetter->log));
9191
strncat(
9292
settergetter->log
@@ -108,7 +108,7 @@ NAN_GETTER(SetterGetter::GetProp1) {
108108

109109
NAN_GETTER(SetterGetter::GetProp2) {
110110
SetterGetter* settergetter =
111-
ObjectWrap::Unwrap<SetterGetter>(info.This());
111+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
112112
assert(strlen(settergetter->log) < sizeof (settergetter->log));
113113
strncat(
114114
settergetter->log
@@ -130,7 +130,7 @@ NAN_GETTER(SetterGetter::GetProp2) {
130130

131131
NAN_SETTER(SetterGetter::SetProp2) {
132132
SetterGetter* settergetter =
133-
ObjectWrap::Unwrap<SetterGetter>(info.This());
133+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
134134
strncpy(
135135
settergetter->prop2
136136
, *v8::String::Utf8Value(value)
@@ -155,7 +155,7 @@ NAN_SETTER(SetterGetter::SetProp2) {
155155

156156
NAN_METHOD(SetterGetter::Log) {
157157
SetterGetter* settergetter =
158-
ObjectWrap::Unwrap<SetterGetter>(info.This());
158+
ObjectWrap::Unwrap<SetterGetter>(info.Holder());
159159

160160
info.GetReturnValue().Set(Nan::New(settergetter->log).ToLocalChecked());
161161
}

‎test/cpp/indexedinterceptors.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ NAN_METHOD(IndexedInterceptor::New) {
7272

7373
NAN_INDEX_GETTER(IndexedInterceptor::PropertyGetter) {
7474
IndexedInterceptor* interceptor =
75-
ObjectWrap::Unwrap<IndexedInterceptor>(info.This());
75+
ObjectWrap::Unwrap<IndexedInterceptor>(info.Holder());
7676
if (index == 0) {
7777
info.GetReturnValue().Set(Nan::New(interceptor->buf).ToLocalChecked());
7878
} else {
@@ -82,7 +82,7 @@ NAN_INDEX_GETTER(IndexedInterceptor::PropertyGetter) {
8282

8383
NAN_INDEX_SETTER(IndexedInterceptor::PropertySetter) {
8484
IndexedInterceptor* interceptor =
85-
ObjectWrap::Unwrap<IndexedInterceptor>(info.This());
85+
ObjectWrap::Unwrap<IndexedInterceptor>(info.Holder());
8686
if (index == 0) {
8787
std::strncpy(
8888
interceptor->buf
@@ -102,7 +102,7 @@ NAN_INDEX_ENUMERATOR(IndexedInterceptor::PropertyEnumerator) {
102102

103103
NAN_INDEX_DELETER(IndexedInterceptor::PropertyDeleter) {
104104
IndexedInterceptor* interceptor =
105-
ObjectWrap::Unwrap<IndexedInterceptor>(info.This());
105+
ObjectWrap::Unwrap<IndexedInterceptor>(info.Holder());
106106
std::strncpy(interceptor->buf, "goober", sizeof (interceptor->buf));
107107
info.GetReturnValue().Set(True());
108108
}

‎test/cpp/namedinterceptors.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ NAN_METHOD(NamedInterceptor::New) {
7272

7373
NAN_PROPERTY_GETTER(NamedInterceptor::PropertyGetter) {
7474
NamedInterceptor* interceptor =
75-
ObjectWrap::Unwrap<NamedInterceptor>(info.This());
75+
ObjectWrap::Unwrap<NamedInterceptor>(info.Holder());
7676
if (!std::strcmp(*v8::String::Utf8Value(property), "prop")) {
7777
info.GetReturnValue().Set(Nan::New(interceptor->buf).ToLocalChecked());
7878
} else {
@@ -82,7 +82,7 @@ NAN_PROPERTY_GETTER(NamedInterceptor::PropertyGetter) {
8282

8383
NAN_PROPERTY_SETTER(NamedInterceptor::PropertySetter) {
8484
NamedInterceptor* interceptor =
85-
ObjectWrap::Unwrap<NamedInterceptor>(info.This());
85+
ObjectWrap::Unwrap<NamedInterceptor>(info.Holder());
8686
if (!std::strcmp(*v8::String::Utf8Value(property), "prop")) {
8787
std::strncpy(
8888
interceptor->buf
@@ -102,7 +102,7 @@ NAN_PROPERTY_ENUMERATOR(NamedInterceptor::PropertyEnumerator) {
102102

103103
NAN_PROPERTY_DELETER(NamedInterceptor::PropertyDeleter) {
104104
NamedInterceptor* interceptor =
105-
ObjectWrap::Unwrap<NamedInterceptor>(info.This());
105+
ObjectWrap::Unwrap<NamedInterceptor>(info.Holder());
106106
std::strncpy(interceptor->buf, "goober", sizeof (interceptor->buf));
107107
info.GetReturnValue().Set(True());
108108
}

‎test/cpp/objectwraphandle.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ class MyObject : public ObjectWrap {
4545
}
4646

4747
static NAN_METHOD(GetHandle) {
48-
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.This());
48+
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.Holder());
4949
info.GetReturnValue().Set(obj->handle());
5050
}
5151

5252
static NAN_METHOD(GetValue) {
53-
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.This());
53+
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.Holder());
5454
info.GetReturnValue().Set(obj->value_);
5555
}
5656

‎test/cpp/wrappedobjectfactory.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class InnerObject : public ObjectWrap {
4949
}
5050

5151
static NAN_METHOD(GetValue) {
52-
InnerObject* obj = ObjectWrap::Unwrap<InnerObject>(info.This());
52+
InnerObject* obj = ObjectWrap::Unwrap<InnerObject>(info.Holder());
5353
info.GetReturnValue().Set(obj->value_);
5454
}
5555

@@ -102,7 +102,7 @@ class MyObject : public ObjectWrap {
102102
}
103103

104104
static NAN_METHOD(GetValue) {
105-
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.This());
105+
MyObject* obj = ObjectWrap::Unwrap<MyObject>(info.Holder());
106106
info.GetReturnValue().Set(obj->value_);
107107
}
108108

‎test/js/accessors-test.js‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const test = require('tap').test
1111
, bindings = require('bindings')({ module_root: testRoot, bindings: 'accessors' });
1212

1313
test('accessors', function (t) {
14-
t.plan(4)
14+
t.plan(7)
1515
var settergetter = bindings.create()
1616
t.equal(settergetter.prop1, 'this is property 1')
1717
t.ok(settergetter.prop2 === '')
@@ -24,4 +24,9 @@ test('accessors', function (t) {
2424
'Prop2:SETTER(setting a value)\n' +
2525
'Prop2:GETTER(setting a value)\n'
2626
)
27+
var derived = Object.create(settergetter)
28+
t.equal(derived.prop1, 'this is property 1')
29+
derived.prop2 = 'setting a new value'
30+
t.equal(derived.prop2, 'setting a new value')
31+
t.equal(settergetter.prop2, 'setting a new value')
2732
})

‎test/js/objectwraphandle-test.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const test = require('tap').test
1111
, bindings = require('bindings')({ module_root: testRoot, bindings: 'objectwraphandle' });
1212

1313
test('objectwraphandle', function (t) {
14-
t.plan(5);
14+
t.plan(7);
1515

1616
t.type(bindings.MyObject, 'function');
1717

@@ -21,4 +21,14 @@ test('objectwraphandle', function (t) {
2121
t.type(obj.getValue, 'function');
2222
t.type(obj.getHandle(), 'object');
2323
t.type(obj.getValue(), 'number');
24+
25+
var derived = Object.create(obj);
26+
t.type(derived, bindings.MyObject);
27+
try {
28+
// In Node 0.10 this call is valid:
29+
t.equal(derived.getValue(), 10);
30+
} catch (err) {
31+
// In Node >= 0.12 it throws instead:
32+
t.type(err, TypeError);
33+
}
2434
});

0 commit comments

Comments
 (0)