From 18b129b12861fa58050679759280fdbfe8a9d8da Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 26 Oct 2013 17:32:28 +0200 Subject: [PATCH 1/2] Trigger garbage collection within Isolate::Scope Before the idle notifications to V8 were sent without a special Isolate entered, which results in V8 using it's default isolate (which we don't use at all). Hence those were pretty useless anyways (if they were called, which was unlikely). Besides V8 seems to not trigger the weak object callbacks if the isolate is destroyed, hence the PHP objects we add-ref'ed before will leak. Therefore this removes the previous idle notification logic and forces garbage collection from the V8Js object destructor. --- php_v8js_macros.h | 2 -- tests/leak-php-object.phpt | 33 +++++++++++++++++++++ v8js.cc | 59 +++++++------------------------------- 3 files changed, 43 insertions(+), 51 deletions(-) create mode 100644 tests/leak-php-object.phpt diff --git a/php_v8js_macros.h b/php_v8js_macros.h index adfd5b1..59b4f12 100644 --- a/php_v8js_macros.h +++ b/php_v8js_macros.h @@ -196,11 +196,9 @@ struct php_v8js_object { ZEND_BEGIN_MODULE_GLOBALS(v8js) int v8_initialized; HashTable *extensions; - int disposed_contexts; /* Disposed contexts since last time V8 did GC */ /* Ini globals */ char *v8_flags; /* V8 command line flags */ - int max_disposed_contexts; /* Max disposed context allowed before forcing V8 GC */ // Timer thread globals std::stack timer_stack; diff --git a/tests/leak-php-object.phpt b/tests/leak-php-object.phpt new file mode 100644 index 0000000..20e7967 --- /dev/null +++ b/tests/leak-php-object.phpt @@ -0,0 +1,33 @@ +--TEST-- +Test V8::executeString() : Test for leaked PHP object if passed back multiple times +--SKIPIF-- + +--FILE-- +foo = new Foo(); +$v8->executeString($js); +unset($v8); + +?> +===EOF=== +--EXPECT-- +destroyed +===EOF=== diff --git a/v8js.cc b/v8js.cc index 8820304..f4ee898 100644 --- a/v8js.cc +++ b/v8js.cc @@ -39,20 +39,6 @@ ZEND_DECLARE_MODULE_GLOBALS(v8js) /* {{{ INI Settings */ -static ZEND_INI_MH(v8js_OnUpdateMaxDisposedContexts) /* {{{ */ -{ - int max_disposed = zend_atoi(new_value, new_value_length); - - if (max_disposed <= 0) { - return FAILURE; - } - - V8JSG(max_disposed_contexts) = max_disposed; - - return SUCCESS; -} -/* }}} */ - static ZEND_INI_MH(v8js_OnUpdateV8Flags) /* {{{ */ { if (new_value) { @@ -71,7 +57,6 @@ static ZEND_INI_MH(v8js_OnUpdateV8Flags) /* {{{ */ /* }}} */ ZEND_INI_BEGIN() /* {{{ */ - ZEND_INI_ENTRY("v8js.max_disposed_contexts", "25", ZEND_INI_ALL, v8js_OnUpdateMaxDisposedContexts) ZEND_INI_ENTRY("v8js.flags", NULL, ZEND_INI_ALL, v8js_OnUpdateV8Flags) ZEND_INI_END() /* }}} */ @@ -542,14 +527,19 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ /* Clear global object, dispose context */ if (!c->context.IsEmpty()) { c->context.Reset(); - V8JSG(disposed_contexts) = v8::V8::ContextDisposedNotification(); -#if V8JS_DEBUG - fprintf(stderr, "Context dispose notification sent (%d)\n", V8JSG(disposed_contexts)); - fflush(stderr); -#endif } c->context.~Persistent(); + /* Force garbage collection on our isolate, this is needed that V8 triggers + * our MakeWeak callbacks. Without these we won't remove our references + * on the PHP objects leading to memory leaks in PHP context. + */ + { + v8::Locker locker(c->isolate); + v8::Isolate::Scope isolate_scope(c->isolate); + while(!v8::V8::IdleNotification()) {}; + } + c->modules_stack.~vector(); c->modules_base.~vector(); efree(object); @@ -1621,28 +1611,6 @@ static PHP_MINIT_FUNCTION(v8js) } /* }}} */ -static void php_v8js_force_v8_gc(void) /* {{{ */ -{ -#if V8JS_DEBUG - TSRMLS_FETCH(); - fprintf(stderr, "############ Running V8 Idle notification: disposed/max_disposed %d/%d ############\n", V8JSG(disposed_contexts), V8JSG(max_disposed_contexts)); - fflush(stderr); -#endif - - while (!v8::V8::IdleNotification()) { -#if V8JS_DEBUG - fprintf(stderr, "."); - fflush(stderr); -#endif - } - -#if V8JS_DEBUG - fprintf(stderr, "\n"); - fflush(stderr); -#endif -} -/* }}} */ - /* {{{ PHP_MSHUTDOWN_FUNCTION */ static PHP_MSHUTDOWN_FUNCTION(v8js) @@ -1673,11 +1641,6 @@ static PHP_RSHUTDOWN_FUNCTION(v8js) fflush(stderr); #endif - /* Force V8 to do as much GC as possible */ - if (V8JSG(max_disposed_contexts) && (V8JSG(disposed_contexts) > V8JSG(max_disposed_contexts))) { - php_v8js_force_v8_gc(); - } - return SUCCESS; } /* }}} */ @@ -1702,8 +1665,6 @@ static PHP_MINFO_FUNCTION(v8js) static PHP_GINIT_FUNCTION(v8js) { v8js_globals->extensions = NULL; - v8js_globals->disposed_contexts = 0; - v8js_globals->max_disposed_contexts = 0; v8js_globals->v8_initialized = 0; v8js_globals->v8_flags = NULL; v8js_globals->timer_thread = NULL; From 68a579f3aededc12aa78015115ee0e7fc1f39371 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 26 Oct 2013 17:54:12 +0200 Subject: [PATCH 2/2] Delete global PHP object on shutdown Deleting the global object implicitly deletes all the properties, i.e. PHP objects we previously attached to it (and for which we increased the reference counter). Since the following idle notifications trigger the weak callbacks, the references on the PHP objects are finally decreased. --- v8js.cc | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/v8js.cc b/v8js.cc index f4ee898..f1d0621 100644 --- a/v8js.cc +++ b/v8js.cc @@ -511,7 +511,18 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ if (c->pending_exception) { zval_ptr_dtor(&c->pending_exception); } - + + /* Delete PHP global object from JavaScript */ + if (!c->context.IsEmpty()) { + v8::Locker locker(c->isolate); + v8::Isolate::Scope isolate_scope(c->isolate); + v8::HandleScope handle_scope(c->isolate); + v8::Context::Scope context_scope(c->isolate, c->context); + + v8::Local object_name_js = v8::Local::New(c->isolate, c->object_name); + V8JS_GLOBAL->Delete(object_name_js); + } + c->object_name.Reset(); c->object_name.~Persistent(); c->global_template.Reset();