Skip to content

Commit 365e81a

Browse files
committed
Another attempt to fix issue #60
`MarkIndependent()` in persistent handle class has been deprecated since recent V8 versions (ca. >= 6.0), so `IsIndependent()` returns always true. This deprecation breaks the mechanism of marking externally referecned wrapped C++ object those shall not be deleted. To fix it, an additional internal `v8::Object` field has been added (total fields count becomes 3) to store a pointer to a wrapped object destructor function. Such an internal pointer field must be aligned by 2, but unfortunately, Visual C++ can generates an unaligned pointer to destroy function. At the current time the single destructor function per wrapped class is stored in the `object_registry`, so thar 3rd internal fields in a wrapped object is used as a simple marker wether to call the object destructor or not.
1 parent 6983d5d commit 365e81a

File tree

2 files changed

+24
-25
lines changed

2 files changed

+24
-25
lines changed

test/test_class.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -207,10 +207,10 @@ void test_class_()
207207
check_eq("Y count after GC", Y::instance_count,
208208
1 + 2 * use_shared_ptr); // y1 + (y2 + y3 when use_shared_ptr)
209209

210-
v8pp::class_<Y, Traits>::destroy(isolate);
211-
check_eq("Y count after destroy", Y::instance_count,
212-
1 + 2 * use_shared_ptr); // y1 + (y2 + y3 when use_shared_ptr)
210+
y1_obj = v8pp::class_<Y, Traits>::reference_external(context.isolate(), y1);
213211

212+
check_eq("Y count before class_<Y>::destroy", Y::instance_count,
213+
1 + 2 * use_shared_ptr); // y1 + (y2 + y3 when use_shared_ptr)
214214
v8pp::class_<Y, Traits>::destroy(isolate);
215215
check_eq("Y count after class_<Y>::destroy", Y::instance_count,
216216
1 + 2 * use_shared_ptr); // y1 + (y2 + y3 when use_shared_ptr)

v8pp/class.hpp

+21-22
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,11 @@ class object_registry final : public class_info
7979
func_.Reset(isolate, func);
8080
js_func_.Reset(isolate, js_func);
8181

82-
// each JavaScript instance has 2 internal fields:
82+
// each JavaScript instance has 3 internal fields:
8383
// 0 - pointer to a wrapped C++ object
8484
// 1 - pointer to this object_registry
85-
func->InstanceTemplate()->SetInternalFieldCount(2);
85+
// 2 - pointer to the object destructor
86+
func->InstanceTemplate()->SetInternalFieldCount(3);
8687
func->Inherit(js_func);
8788
}
8889

@@ -105,7 +106,7 @@ class object_registry final : public class_info
105106

106107
~object_registry()
107108
{
108-
remove_objects(true);
109+
remove_objects();
109110
}
110111

111112
v8::Isolate* isolate() { return isolate_; }
@@ -166,24 +167,24 @@ class object_registry final : public class_info
166167
return false;
167168
}
168169

169-
void remove_object(object_id const& obj, bool call_dtor)
170+
void remove_object(object_id const& obj)
170171
{
171172
auto it = objects_.find(Traits::key(obj));
172173
assert(it != objects_.end() && "no object");
173174
if (it != objects_.end())
174175
{
175176
v8::HandleScope scope(isolate_);
176-
reset_object(*it, call_dtor);
177+
reset_object(*it);
177178
objects_.erase(it);
178179
}
179180
}
180181

181-
void remove_objects(bool call_dtor)
182+
void remove_objects()
182183
{
183184
v8::HandleScope scope(isolate_);
184185
for (auto& object : objects_)
185186
{
186-
reset_object(object, call_dtor);
187+
reset_object(object);
187188
}
188189
objects_.clear();
189190
}
@@ -236,20 +237,15 @@ class object_registry final : public class_info
236237

237238
obj->SetAlignedPointerInInternalField(0, Traits::pointer_id(object));
238239
obj->SetAlignedPointerInInternalField(1, this);
240+
obj->SetAlignedPointerInInternalField(2, call_dtor? /*dtor_*/this : nullptr);
239241

240242
v8::UniquePersistent<v8::Object> pobj(isolate_, obj);
241-
pobj.SetWeak(call_dtor? this : nullptr,
242-
[](v8::WeakCallbackInfo<object_registry> const& data)
243+
pobj.SetWeak(this, [](v8::WeakCallbackInfo<object_registry> const& data)
243244
{
244-
bool const call_dtor = (data.GetParameter() != nullptr);
245245
object_id object = data.GetInternalField(0);
246246
object_registry* this_ = static_cast<object_registry*>(data.GetInternalField(1));
247-
this_->remove_object(object, call_dtor);
247+
this_->remove_object(object);
248248
}, v8::WeakCallbackType::kInternalFields);
249-
if (!call_dtor)
250-
{
251-
pobj.MarkIndependent();
252-
}
253249
objects_.emplace(object, std::move(pobj));
254250

255251
return scope.Escape(obj);
@@ -272,7 +268,7 @@ class object_registry final : public class_info
272268
while (value->IsObject())
273269
{
274270
v8::Handle<v8::Object> obj = value->ToObject();
275-
if (obj->InternalFieldCount() == 2)
271+
if (obj->InternalFieldCount() == 3)
276272
{
277273
object_id id = obj->GetAlignedPointerFromInternalField(0);
278274
if (id)
@@ -295,19 +291,22 @@ class object_registry final : public class_info
295291
}
296292

297293
private:
298-
void reset_object(std::pair<pointer_type const, v8::UniquePersistent<v8::Object>>& object, bool call_dtor)
294+
void reset_object(std::pair<pointer_type const, v8::UniquePersistent<v8::Object>>& object)
299295
{
296+
bool call_dtor = true;
300297
if (!object.second.IsNearDeath())
301298
{
302299
v8::Local<v8::Object> obj = to_local(isolate_, object.second);
303-
assert(obj->InternalFieldCount() == 2);
300+
assert(obj->InternalFieldCount() == 3);
304301
assert(obj->GetAlignedPointerFromInternalField(0) == Traits::pointer_id(object.first));
305302
assert(obj->GetAlignedPointerFromInternalField(1) == this);
303+
call_dtor = (obj->GetAlignedPointerFromInternalField(2) != nullptr);
306304
// remove internal fields to disable unwrapping for this V8 Object
307305
obj->SetAlignedPointerInInternalField(0, nullptr);
308306
obj->SetAlignedPointerInInternalField(1, nullptr);
307+
obj->SetAlignedPointerInInternalField(2, nullptr);
309308
}
310-
if (call_dtor && !object.second.IsIndependent())
309+
if (call_dtor)
311310
{
312311
dtor_(isolate_, object.first);
313312
}
@@ -612,7 +611,7 @@ class class_
612611
static void unreference_external(v8::Isolate* isolate, object_pointer_type const& ext)
613612
{
614613
using namespace detail;
615-
return classes::find<Traits>(isolate, type_id<T>()).remove_object(Traits::pointer_id(ext), false);
614+
return classes::find<Traits>(isolate, type_id<T>()).remove_object(Traits::pointer_id(ext));
616615
}
617616

618617
/// As reference_external but delete memory for C++ object
@@ -654,14 +653,14 @@ class class_
654653
static void destroy_object(v8::Isolate* isolate, object_pointer_type const& obj)
655654
{
656655
using namespace detail;
657-
classes::find<Traits>(isolate, type_id<T>()).remove_object(Traits::pointer_id(obj), true);
656+
classes::find<Traits>(isolate, type_id<T>()).remove_object(Traits::pointer_id(obj));
658657
}
659658

660659
/// Destroy all wrapped C++ objects of this class
661660
static void destroy_objects(v8::Isolate* isolate)
662661
{
663662
using namespace detail;
664-
classes::find<Traits>(isolate, type_id<T>()).remove_objects(true);
663+
classes::find<Traits>(isolate, type_id<T>()).remove_objects();
665664
}
666665

667666
/// Destroy all wrapped C++ objects and this binding class_

0 commit comments

Comments
 (0)