vitorsousa pushed a commit to branch master. http://git.enlightenment.org/core/efl.git/commit/?id=afb66463937a8d08e2bf1d538641a9520ba457d6
commit afb66463937a8d08e2bf1d538641a9520ba457d6 Author: Vitor Sousa <[email protected]> Date: Wed Mar 16 19:46:22 2016 -0300 efl js: Add clean up callbacks to deallocate memory used in v8::External Add several garbage collector callbacks for cleaning allocated C and C++ data used inside v8:External objects. Fix eo_unref of already freed object in eo_js_construct_from_eo.hh. Ensure all structs are allocated with malloc. Add test for garbage collection. Had to created .sh script because shebang clause do not support multiple arguments. --- src/Makefile_Eolian_Js.am | 4 +- src/bindings/js/eina_js/eina_js_compatibility.hh | 5 +- src/bindings/js/eina_js/eina_js_container.cc | 16 +++- src/bindings/js/eina_js/eina_js_container.hh | 5 ++ .../js/eina_js/eina_js_get_value_from_c.hh | 89 +++++++++++++++++----- src/bindings/js/eina_js/eina_js_value.cc | 3 + src/bindings/js/eo_js/eo_js_construct_from_eo.hh | 1 + src/bindings/js/eo_js/eo_js_event.hh | 1 + src/bindings/js/eo_js/eo_js_struct.hh | 6 +- src/tests/eolian_js/eolian_js_suite.js | 16 ++++ src/tests/eolian_js/eolian_js_suite.sh | 4 + 11 files changed, 123 insertions(+), 27 deletions(-) diff --git a/src/Makefile_Eolian_Js.am b/src/Makefile_Eolian_Js.am index 32cf893..b15fa87 100644 --- a/src/Makefile_Eolian_Js.am +++ b/src/Makefile_Eolian_Js.am @@ -25,11 +25,11 @@ include Makefile_Eolian_Js_Helper.am if EFL_ENABLE_TESTS if HAVE_NODEJS -TESTS += tests/eolian_js/eolian_js_suite.js +TESTS += tests/eolian_js/eolian_js_suite.sh check_LTLIBRARIES += tests/eolian_js/libeolian_js_suite.la -tests/eolian_js/eolian_js_suite.js: tests/eolian_js/eolian_js_suite_mod.node +tests/eolian_js/eolian_js_suite.sh: tests/eolian_js/eolian_js_suite_mod.node tests/eolian_js/eolian_js_suite_mod.node: tests/eolian_js/libeolian_js_suite.la $(AM_V_CP)$(CP) $(top_builddir)/src/tests/eolian_js/.libs/libeolian_js_suite.so $(top_builddir)/src/tests/eolian_js/eolian_js_suite_mod.node diff --git a/src/bindings/js/eina_js/eina_js_compatibility.hh b/src/bindings/js/eina_js/eina_js_compatibility.hh index fb36e4e..7a46714 100644 --- a/src/bindings/js/eina_js/eina_js_compatibility.hh +++ b/src/bindings/js/eina_js/eina_js_compatibility.hh @@ -927,8 +927,9 @@ compatibility_return_type cast_function(compatibility_callback_info_type args) char* class_name = *str; auto ctor = ::efl::eina::js::get_class_constructor(class_name); - return compatibility_return - (new_v8_external_instance(ctor, ::eo_ref(eo), isolate), args); + auto obj = new_v8_external_instance(ctor, ::eo_ref(eo), isolate); + efl::eina::js::make_weak(isolate, obj, [eo]{ ::eo_unref(eo); }); + return compatibility_return(obj, args); } else { diff --git a/src/bindings/js/eina_js/eina_js_container.cc b/src/bindings/js/eina_js/eina_js_container.cc index 60c53e0..82246c5 100644 --- a/src/bindings/js/eina_js/eina_js_container.cc +++ b/src/bindings/js/eina_js/eina_js_container.cc @@ -54,11 +54,14 @@ v8::Local<v8::Value> concat(eina_container_base& lhs, v8::Isolate* isolate, v8:: , &typeinfo_rhs = typeid(rhs); if(!typeinfo_lhs.before(typeinfo_rhs) && !typeinfo_rhs.before(typeinfo_lhs)) { + auto ptr = rhs.concat(lhs); v8::Handle<v8::Value> a[] = - {efl::eina::js::compatibility_new<v8::External>(isolate, rhs.concat(lhs))}; + {efl::eina::js::compatibility_new<v8::External>(isolate, ptr)}; assert(!!*instance_templates[lhs.get_container_type()].handle()); v8::Local<v8::Object> result = instance_templates[lhs.get_container_type()].handle()->NewInstance(1, a); + if (ptr) + efl::eina::js::make_weak(isolate, result, [ptr]{ delete ptr; }); return result; } else @@ -95,9 +98,12 @@ v8::Local<v8::Value> slice(eina_container_base& self, v8::Isolate* isolate, v8:: else return v8::Undefined(isolate); - v8::Handle<v8::Value> a[] = {efl::eina::js::compatibility_new<v8::External>(isolate, self.slice(i, j))}; + auto ptr = self.slice(i, j); + v8::Handle<v8::Value> a[] = {efl::eina::js::compatibility_new<v8::External>(isolate, ptr)}; v8::Local<v8::Object> result = instance_templates[self.get_container_type()].handle() ->NewInstance(1, a); + if (ptr) + efl::eina::js::make_weak(isolate, result, [ptr]{ delete ptr; }); return result; } @@ -154,6 +160,7 @@ compatibility_return_type new_eina_list_internal(compatibility_callback_info_typ eina_container_base* p = new eina_list<int>; compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); } else { @@ -182,6 +189,7 @@ compatibility_return_type new_eina_list(compatibility_callback_info_type args) eina_container_base* p = new eina_list<int>; compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); return compatibility_return(); } else if (args.Length() == 1 && args[0]->IsString()) @@ -195,6 +203,7 @@ compatibility_return_type new_eina_list(compatibility_callback_info_type args) } compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); return compatibility_return(); } } @@ -212,6 +221,7 @@ compatibility_return_type new_eina_array_internal(compatibility_callback_info_ty eina_container_base* p = new eina_array<int>; compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); } else { @@ -240,6 +250,7 @@ compatibility_return_type new_eina_array(compatibility_callback_info_type args) eina_container_base* p = new eina_array<int>; compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); return compatibility_return(); } else if (args.Length() == 1 && args[0]->IsString()) @@ -253,6 +264,7 @@ compatibility_return_type new_eina_array(compatibility_callback_info_type args) } compatibility_set_pointer_internal_field (args.This(), 0, dynamic_cast<void*>(p)); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ delete p; }); return compatibility_return(); } } diff --git a/src/bindings/js/eina_js/eina_js_container.hh b/src/bindings/js/eina_js/eina_js_container.hh index 21343fd..62fe22c 100644 --- a/src/bindings/js/eina_js/eina_js_container.hh +++ b/src/bindings/js/eina_js/eina_js_container.hh @@ -273,6 +273,11 @@ struct eina_container_common : eina_container_type_specific<C, typename C::value (eina::js::compatibility_new<v8::String>(isolate, "Indexed attribution was not implemented."))); return v8::Undefined(isolate); } + + typename C::native_handle_type release_native_handle() + { + return _container.release_native_handle(); + } C _container; typedef C container_type; }; diff --git a/src/bindings/js/eina_js/eina_js_get_value_from_c.hh b/src/bindings/js/eina_js/eina_js_get_value_from_c.hh index 204c497..653a96e 100644 --- a/src/bindings/js/eina_js/eina_js_get_value_from_c.hh +++ b/src/bindings/js/eina_js/eina_js_get_value_from_c.hh @@ -143,7 +143,9 @@ inline v8::Local<v8::Value> get_value_from_c(Eo* v, v8::Isolate* isolate, const char* class_name) { auto ctor = ::efl::eina::js::get_class_constructor(class_name); - return new_v8_external_instance(ctor, v, isolate); + auto obj = new_v8_external_instance(ctor, ::eo_ref(v), isolate); + efl::eina::js::make_weak(isolate, obj, [v]{ ::eo_unref(v); }); + return obj; } inline v8::Local<v8::Value> @@ -151,23 +153,32 @@ get_value_from_c(const Eo* v, v8::Isolate* isolate, const char* class_name) { // TODO: implement const objects? auto ctor = ::efl::eina::js::get_class_constructor(class_name); - return new_v8_external_instance(ctor, const_cast<Eo*>(v), isolate); + auto obj = new_v8_external_instance(ctor, ::eo_ref(v), isolate); + efl::eina::js::make_weak(isolate, obj, [v]{ ::eo_unref(v); }); + return obj; } template <typename T> inline v8::Local<v8::Value> -get_value_from_c(struct_ptr_tag<T> v, v8::Isolate* isolate, const char* class_name) +get_value_from_c(struct_ptr_tag<T*> v, v8::Isolate* isolate, const char* class_name) { - // TODO: implement const structs? + using nonconst_type = typename std::remove_const<T>::type; + bool own = false; // TODO: handle ownership + auto ptr = const_cast<nonconst_type*>(v.value); auto ctor = ::efl::eina::js::get_class_constructor(class_name); - return new_v8_external_instance(ctor, const_cast<typename std::remove_const<T>::type>(v.value), isolate); + auto obj = new_v8_external_instance(ctor, ptr, isolate); + if (own) + efl::eina::js::make_weak(isolate, obj, [ptr]{ free(ptr); }); + return obj; } template <typename T> inline v8::Local<v8::Value> get_value_from_c(struct_tag<T> v, v8::Isolate* isolate, const char* class_name) { - return get_value_from_c(struct_ptr_tag<T*>{new T(v.value)}, isolate, class_name); + T* s = static_cast<T*>(malloc(sizeof(T))); + *s = v.value; + return get_value_from_c(struct_ptr_tag<T*>{s}, isolate, class_name); } template <typename T, typename K> @@ -175,8 +186,16 @@ inline v8::Local<v8::Value> get_value_from_c(efl::eina::js::complex_tag<Eina_Accessor *, T, K> v, v8::Isolate* isolate, const char*) { using wrapped_type = typename container_wrapper<T>::type; + bool own = false; // TODO: handle ownership auto a = new ::efl::eina::accessor<wrapped_type>{v.value}; - return export_accessor<T>(*a , isolate, K::class_name()); + auto obj = export_accessor<T>(*a , isolate, K::class_name()); + efl::eina::js::make_weak(isolate, obj, [a, own] + { + if (!own) + a->release_native_handle(); + delete a; + }); + return obj; } template <typename T, typename K> @@ -191,18 +210,34 @@ template <typename T, typename K> inline v8::Local<v8::Value> get_value_from_c(efl::eina::js::complex_tag<Eina_Array *, T, K> v, v8::Isolate* isolate, const char*) { - // TODO: use unique_ptr for eina_array to avoid leak ? - auto o = new ::efl::eina::js::range_eina_array<T, K>(v.value); + bool own = false; // TODO: handle ownership + auto o = new ::efl::eina::js::eina_array<T, K>(v.value); auto ctor = get_array_instance_template(); - return new_v8_external_instance(ctor, o, isolate); + auto obj = new_v8_external_instance(ctor, o, isolate); + efl::eina::js::make_weak(isolate, obj, [o, own] + { + if (!own) + o->release_native_handle(); + delete o; + }); + return obj; } template <typename T, typename K> inline v8::Local<v8::Value> -get_value_from_c(efl::eina::js::complex_tag<const Eina_Array *, T, K> v, v8::Isolate* isolate, const char* class_name) +get_value_from_c(efl::eina::js::complex_tag<const Eina_Array *, T, K> v, v8::Isolate* isolate, const char*) { - // TODO: implement const array? - return get_value_from_c(efl::eina::js::complex_tag<Eina_Array *, T, K>{const_cast<Eina_Array*>(v.value)}, isolate, class_name); + bool own = false; // TODO: handle ownership + auto o = new ::efl::eina::js::range_eina_array<T, K>(const_cast<Eina_Array*>(v.value)); + auto ctor = get_array_instance_template(); + auto obj = new_v8_external_instance(ctor, o, isolate); + efl::eina::js::make_weak(isolate, obj, [o, own] + { + if (!own) + o->release_native_handle(); + delete o; + }); + return obj; } template <typename T, typename K> @@ -245,18 +280,34 @@ template <typename T, typename K> inline v8::Local<v8::Value> get_value_from_c(efl::eina::js::complex_tag<Eina_List *, T, K> v, v8::Isolate* isolate, const char*) { - // TODO: ensure eina_list ownership ??? - auto o = new ::efl::eina::js::range_eina_list<T, K>(v.value); + bool own = false; // TODO: handle ownership + auto o = new ::efl::eina::js::eina_list<T, K>(v.value); auto ctor = get_list_instance_template(); - return new_v8_external_instance(ctor, o, isolate); + auto obj = new_v8_external_instance(ctor, o, isolate); + efl::eina::js::make_weak(isolate, obj, [o, own] + { + if (!own) + o->release_native_handle(); + delete o; + }); + return obj; } template <typename T, typename K> inline v8::Local<v8::Value> -get_value_from_c(efl::eina::js::complex_tag<const Eina_List *, T, K> v, v8::Isolate* isolate, const char* class_name) +get_value_from_c(efl::eina::js::complex_tag<const Eina_List *, T, K> v, v8::Isolate* isolate, const char*) { - // TODO: implement const list? - return get_value_from_c(efl::eina::js::complex_tag<Eina_List *, T, K>{const_cast<Eina_List*>(v.value)}, isolate, class_name); + bool own = false; // TODO: handle ownership + auto o = new ::efl::eina::js::range_eina_list<T, K>(const_cast<Eina_List*>(v.value)); + auto ctor = get_list_instance_template(); + auto obj = new_v8_external_instance(ctor, o, isolate); + efl::eina::js::make_weak(isolate, obj, [o, own] + { + if (!own) + o->release_native_handle(); + delete o; + }); + return obj; } diff --git a/src/bindings/js/eina_js/eina_js_value.cc b/src/bindings/js/eina_js/eina_js_value.cc index 5b5b822..c7377ae 100644 --- a/src/bindings/js/eina_js/eina_js_value.cc +++ b/src/bindings/js/eina_js/eina_js_value.cc @@ -54,6 +54,9 @@ compatibility_return_type eina_value_constructor(compatibility_callback_info_typ std::unique_ptr<value> ptr(new value(value_cast<value>(args[0]))); compatibility_set_pointer_internal_field(args.This(), 0, ptr.get()); + auto v = ptr.get(); + if (v) + efl::eina::js::make_weak(isolate, args.This(), [v]{ delete v; }); ptr.release(); } catch(const std::bad_cast &e) { v8::Local<v8::Object> je = compatibility_new<v8::Object>(isolate); diff --git a/src/bindings/js/eo_js/eo_js_construct_from_eo.hh b/src/bindings/js/eo_js/eo_js_construct_from_eo.hh index 287aaa7..8008d16 100644 --- a/src/bindings/js/eo_js/eo_js_construct_from_eo.hh +++ b/src/bindings/js/eo_js/eo_js_construct_from_eo.hh @@ -20,6 +20,7 @@ inline eina::js::compatibility_return_type construct_from_eo(eina::js::compatibi { Eo* eo = static_cast<Eo*>(v8::External::Cast(*args[0])->Value()); args.This()->SetInternalField(0, args[0]); + ::eo_ref(eo); efl::eina::js::make_weak(args.GetIsolate(), args.This(), [eo] { eo_unref(eo); }); return eina::js::compatibility_return(); } diff --git a/src/bindings/js/eo_js/eo_js_event.hh b/src/bindings/js/eo_js/eo_js_event.hh index 280aeac..b589e5d 100644 --- a/src/bindings/js/eo_js/eo_js_event.hh +++ b/src/bindings/js/eo_js/eo_js_event.hh @@ -100,6 +100,7 @@ inline eina::js::compatibility_return_type on_event(eina::js::compatibility_call event_callback_information* i = new event_callback_information {event, {isolate, eina::js::compatibility_cast<v8::Function>(f)}}; eo_event_callback_add(eo, event->event, event->event_callback, i); + efl::eina::js::make_weak(isolate, self, [i]{ delete i; }); } else { diff --git a/src/bindings/js/eo_js/eo_js_struct.hh b/src/bindings/js/eo_js/eo_js_struct.hh index 731f9d0..eba0c74 100644 --- a/src/bindings/js/eo_js/eo_js_struct.hh +++ b/src/bindings/js/eo_js/eo_js_struct.hh @@ -42,8 +42,10 @@ eina::js::compatibility_return_type new_struct(eina::js::compatibility_callback_ if(args.Length() == 0) { - S* p = new S{}; - eina::js::compatibility_set_pointer_internal_field(args.This(), 0, static_cast<void*>(p)); + void* p = std::malloc(sizeof(S)); + std::memset(p, 0, sizeof(S)); + eina::js::compatibility_set_pointer_internal_field(args.This(), 0, p); + efl::eina::js::make_weak(args.GetIsolate(), args.This(), [p]{ free(p); }); } else { diff --git a/src/tests/eolian_js/eolian_js_suite.js b/src/tests/eolian_js/eolian_js_suite.js index 66e5596..2c45eb3 100755 --- a/src/tests/eolian_js/eolian_js_suite.js +++ b/src/tests/eolian_js/eolian_js_suite.js @@ -795,6 +795,22 @@ startTest("method_iterator_of_structs", function() { // assert(a[0] === 42); // }); +// Garbage Collection // +startTest("gc_object", function() { + var freed = false; + (function() { + var obj = new TestObject(null); + obj.on("del", function() { + printInfo('Object destructed') + freed = true; + }); + }()); + printInfo('going to garbage collect'); + global.gc(); + printInfo('is object destructed?'); + assert(freed); +}); + startTest("new Constructor_Method_Class", function() { var obj = new ConstructorMethodClass(null, 5, 10.0); assert(obj); diff --git a/src/tests/eolian_js/eolian_js_suite.sh b/src/tests/eolian_js/eolian_js_suite.sh new file mode 100755 index 0000000..6b78a50 --- /dev/null +++ b/src/tests/eolian_js/eolian_js_suite.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +BASEDIR=$(dirname $0) +/usr/bin/env node --expose-gc $BASEDIR/eolian_js_suite.js --
