From 4650273c90569211425757c88ae93c0342ddd4e3 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 19:42:03 +0200 Subject: [PATCH 1/6] Shutdown V8 on GSHUTDOWN --- php_v8js_macros.h | 3 +++ v8js.cc | 8 ++++++++ v8js_v8.cc | 4 ++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/php_v8js_macros.h b/php_v8js_macros.h index 9fd430c..cc7aaa4 100644 --- a/php_v8js_macros.h +++ b/php_v8js_macros.h @@ -115,6 +115,9 @@ void v8js_register_accessors(std::vector *accessor_list, v8: /* Module globals */ ZEND_BEGIN_MODULE_GLOBALS(v8js) int v8_initialized; +#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 + v8::Platform *v8_platform; +#endif HashTable *extensions; /* Ini globals */ diff --git a/v8js.cc b/v8js.cc index c622c3e..4ed5bcd 100755 --- a/v8js.cc +++ b/v8js.cc @@ -200,6 +200,14 @@ static PHP_GSHUTDOWN_FUNCTION(v8js) v8js_globals->timer_stack.~deque(); v8js_globals->timer_mutex.~mutex(); #endif + + if (v8js_globals->v8_initialized) { + v8::V8::Dispose(); + v8::V8::ShutdownPlatform(); +#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 + delete v8js_globals->v8_platform; +#endif + } } /* }}} */ diff --git a/v8js_v8.cc b/v8js_v8.cc index 742c559..a202c8a 100644 --- a/v8js_v8.cc +++ b/v8js_v8.cc @@ -42,8 +42,8 @@ void v8js_v8_init(TSRMLS_D) /* {{{ */ } #if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 - v8::Platform* platform = v8::platform::CreateDefaultPlatform(); - v8::V8::InitializePlatform(platform); + V8JSG(v8_platform) = v8::platform::CreateDefaultPlatform(); + v8::V8::InitializePlatform(V8JSG(v8_platform)); #endif /* Set V8 command line flags (must be done before V8::Initialize()!) */ From dd4996cd5653213160d9b671c074b10cacd6df22 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 19:42:42 +0200 Subject: [PATCH 2/6] delete v8::Extension instance on shutdown --- v8js_class.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/v8js_class.cc b/v8js_class.cc index 924de4d..a54c8a7 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -62,6 +62,7 @@ struct v8js_jsext { int deps_count; char *name; char *source; + v8::Extension *extension; }; /* }}} */ @@ -241,6 +242,7 @@ static void v8js_jsext_dtor(v8js_jsext *jsext) /* {{{ */ if (jsext->deps) { v8js_free_ext_strarr(jsext->deps, jsext->deps_count); } + delete jsext->extension; free(jsext->name); free(jsext->source); } @@ -822,15 +824,16 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint zend_hash_copy(jsext->deps_ht, Z_ARRVAL_P(deps_arr), (copy_ctor_func_t) v8js_persistent_zval_ctor, NULL, sizeof(zval *)); } + jsext->extension = new v8::Extension(jsext->name, jsext->source, jsext->deps_count, jsext->deps); + if (zend_hash_add(V8JSG(extensions), name, name_len + 1, jsext, sizeof(v8js_jsext), NULL) == FAILURE) { v8js_jsext_dtor(jsext); free(jsext); return FAILURE; } - v8::Extension *extension = new v8::Extension(jsext->name, jsext->source, jsext->deps_count, jsext->deps); - extension->set_auto_enable(auto_enable ? true : false); - v8::RegisterExtension(extension); + jsext->extension->set_auto_enable(auto_enable ? true : false); + v8::RegisterExtension(jsext->extension); free(jsext); From 981f5023035168ce270ede0fcbeebe314a8b1913 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 19:43:58 +0200 Subject: [PATCH 3/6] ignore run-tests.php valgrind result files --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 4dcc784..4e36813 100644 --- a/.gitignore +++ b/.gitignore @@ -34,3 +34,4 @@ v8js.la /tests/*.out /tests/*.php /tests/*.sh +/tests/*.mem From 1f56c8e43c00ff324836493d40d3565558193b9a Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 21:26:18 +0200 Subject: [PATCH 4/6] Don't only run dtor, really delete the object --- v8js_class.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/v8js_class.cc b/v8js_class.cc index a54c8a7..7ccb033 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -775,7 +775,7 @@ static void v8js_script_free(v8js_script *res, bool dispose_persistent) res->name = NULL; } if (dispose_persistent) { - res->script->~Persistent(); // does Reset() + delete res->script; // does Reset() res->script = NULL; } } From cb7a1b8d6a58c173f73230aef119b12fbace13e5 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 22:36:23 +0200 Subject: [PATCH 5/6] Dispose persisted v8::Script object on resource dtor (memory leak) --- v8js_class.cc | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/v8js_class.cc b/v8js_class.cc index 7ccb033..54a51b8 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -50,7 +50,7 @@ typedef struct _v8js_script { v8::Persistent> *script; } v8js_script; -static void v8js_script_free(v8js_script *res, bool dispose_persistent); +static void v8js_script_free(v8js_script *res); int le_v8js_script; @@ -545,7 +545,7 @@ static PHP_METHOD(V8Js, executeString) RETURN_FALSE; } v8js_execute_script(getThis(), res, flags, time_limit, memory_limit, &return_value TSRMLS_CC); - v8js_script_free(res, true); + v8js_script_free(res); efree(res); } /* }}} */ @@ -611,7 +611,7 @@ static PHP_METHOD(V8Js, checkString) RETURN_FALSE; } - v8js_script_free(res, true); + v8js_script_free(res); efree(res); RETURN_TRUE; } @@ -768,23 +768,17 @@ static void v8js_persistent_zval_dtor(zval **p) /* {{{ */ } /* }}} */ -static void v8js_script_free(v8js_script *res, bool dispose_persistent) +static void v8js_script_free(v8js_script *res) { - if (res->name) { - efree(res->name); - res->name = NULL; - } - if (dispose_persistent) { - delete res->script; // does Reset() - res->script = NULL; - } + efree(res->name); + delete res->script; // does Reset() } static void v8js_script_dtor(zend_rsrc_list_entry *rsrc TSRMLS_DC) /* {{{ */ { v8js_script *res = (v8js_script *)rsrc->ptr; if (res) { - v8js_script_free(res, false); + v8js_script_free(res); efree(res); } } From 0deb7d180298f3cb72898b2ffe5d43b625a2a77a Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 2 Aug 2015 23:35:47 +0200 Subject: [PATCH 6/6] Track list of valid script object in ctx We don't know whether the V8Js object dtor or the script (resource) dtor is called first. Nevertheless they depend on each other as we must not simply Reset the persistent v8::Script if the v8::Isolate was disposed before. Now the V8Js dtor iterates over all script resources and resets the persistent v8::Script instances; leaving an invalidated resource object (which cannot be called anymore). --- v8js_class.cc | 24 +++++++++++++++++++++--- v8js_class.h | 2 ++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/v8js_class.cc b/v8js_class.cc index 54a51b8..d93a56f 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -33,6 +33,7 @@ extern "C" { #include "v8js_timer.h" #include +#include #define PHP_V8JS_SCRIPT_RES_NAME "V8Js script" @@ -46,7 +47,7 @@ static zend_object_handlers v8js_object_handlers; typedef struct _v8js_script { char *name; - v8::Isolate *isolate; + v8js_ctx *ctx; v8::Persistent> *script; } v8js_script; @@ -154,6 +155,13 @@ static void v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ } c->v8js_v8objects.~list(); + for (std::vector::iterator it = c->script_objects.begin(); + it != c->script_objects.end(); it ++) { + (*it)->ctx = NULL; + (*it)->script->Reset(); + } + c->script_objects.~vector(); + /* Clear persistent handles in module cache */ for (std::map::iterator it = c->modules_loaded.begin(); it != c->modules_loaded.end(); ++it) { @@ -210,6 +218,7 @@ static zend_object_value v8js_new(zend_class_entry *ce TSRMLS_DC) /* {{{ */ new(&c->weak_objects) std::map(); new(&c->v8js_v8objects) std::list(); + new(&c->script_objects) std::vector(); retval.handle = zend_objects_store_put(c, NULL, (zend_objects_free_object_storage_t) v8js_free_storage, NULL TSRMLS_CC); retval.handlers = &v8js_object_handlers; @@ -496,7 +505,7 @@ static void v8js_compile_script(zval *this_ptr, const char *str, int str_len, co v8::String::Utf8Value _sname(sname); res->name = estrndup(ToCString(_sname), _sname.length()); - res->isolate = c->isolate; + res->ctx = c; *ret = res; return; } @@ -505,7 +514,7 @@ static void v8js_execute_script(zval *this_ptr, v8js_script *res, long flags, lo { v8js_ctx *c = (v8js_ctx *) zend_object_store_get_object(getThis() TSRMLS_CC); - if (res->isolate != c->isolate) { + if (res->ctx != c) { zend_error(E_WARNING, "Script resource from wrong V8Js object passed"); ZVAL_BOOL(*return_value, 0); return; @@ -566,6 +575,10 @@ static PHP_METHOD(V8Js, compileString) v8js_compile_script(getThis(), str, str_len, identifier, identifier_len, &res TSRMLS_CC); if (res) { ZEND_REGISTER_RESOURCE(return_value, res, le_v8js_script); + + v8js_ctx *ctx; + ctx = (v8js_ctx *) zend_object_store_get_object(this_ptr TSRMLS_CC); + ctx->script_objects.push_back(res); } return; } @@ -778,6 +791,11 @@ static void v8js_script_dtor(zend_rsrc_list_entry *rsrc TSRMLS_DC) /* {{{ */ { v8js_script *res = (v8js_script *)rsrc->ptr; if (res) { + if(res->ctx) { + std::vector::iterator it = std::find(res->ctx->script_objects.begin(), res->ctx->script_objects.end(), res); + res->ctx->script_objects.erase(it); + } + v8js_script_free(res); efree(res); } diff --git a/v8js_class.h b/v8js_class.h index 4500f99..7b36c7e 100644 --- a/v8js_class.h +++ b/v8js_class.h @@ -22,6 +22,7 @@ typedef v8::Persistent > v8 /* Forward declarations */ struct v8js_v8object; struct v8js_accessor_ctx; +struct _v8js_script; /* {{{ Context container */ struct v8js_ctx { @@ -52,6 +53,7 @@ struct v8js_ctx { std::list v8js_v8objects; std::vector accessor_list; + std::vector script_objects; char *tz; #ifdef ZTS void ***zts_ctx;