Clear persistent cells with weak references correctly, refs #88

php_v8js_macros.h

@@ -90,6 +90,7 @@ namespace v8 {
 
 /* Abbreviate long type names */
 typedef v8::Persistent<v8::FunctionTemplate, v8::CopyablePersistentTraits<v8::FunctionTemplate> > v8js_tmpl_t;
+typedef v8::Persistent<v8::Object, v8::CopyablePersistentTraits<v8::Object> > v8js_persistent_obj_t;
 
 /* Hidden field name used to link JS wrappers with underlying PHP object */
 #define PHPJS_OBJECT_KEY "phpjs::object"
@@ -187,6 +188,10 @@ struct php_v8js_ctx {
   std::vector<char *> modules_stack;
   std::vector<char *> modules_base;
   std::map<const char *,v8js_tmpl_t> template_cache;
+
+  std::map<zval *, v8js_persistent_obj_t> weak_objects;
+  std::map<v8js_tmpl_t *, v8js_persistent_obj_t> weak_closures;
+
   std::vector<php_v8js_accessor_ctx *> accessor_list;
 #ifdef ZTS
   void ***zts_ctx;

tests/leak-php-object.phpt

@@ -6,7 +6,9 @@ Test V8::executeString() : Test for leaked PHP object if passed back multiple ti
 <?php
 
 $js =<<< EOF
-for(var i = 0; i < 1000; i ++) {
+// Generate a large number of objects so we trigger definitely trigger the
+// garbage collector.  5000 * 1 kB should be enough.
+for(var i = 0; i < 5000; i ++) {
     PHP.foo.getStdClassObject();
 }
 EOF;

v8js.cc

@@ -607,6 +607,26 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 		while(!v8::V8::IdleNotification()) {};
 	}
 
+	/* Dispose yet undisposed weak refs */
+	for (std::map<zval *, v8js_persistent_obj_t>::iterator it = c->weak_objects.begin();
+		 it != c->weak_objects.end(); ++it) {
+		zval *value = it->first;
+		zval_ptr_dtor(&value);
+		c->isolate->AdjustAmountOfExternalAllocatedMemory(-1024);
+		it->second.Reset();
+	}
+	c->weak_objects.~map();
+
+	for (std::map<v8js_tmpl_t *, v8js_persistent_obj_t>::iterator it = c->weak_closures.begin();
+		 it != c->weak_closures.end(); ++it) {
+		v8js_tmpl_t *persist_tpl_ = it->first;
+		persist_tpl_->Reset();
+		delete persist_tpl_;
+		it->second.Reset();
+	}
+	c->weak_closures.~map();
+
+
 	c->modules_stack.~vector();
 	c->modules_base.~vector();
 	efree(object);
@@ -639,6 +659,9 @@ static zend_object_value php_v8js_new(zend_class_entry *ce TSRMLS_DC) /* {{{ */
 	new(&c->template_cache) std::map<const char *,v8js_tmpl_t>();
 	new(&c->accessor_list) std::vector<php_v8js_accessor_ctx *>();
 
+	new(&c->weak_closures) std::map<v8js_tmpl_t *, v8js_persistent_obj_t>();
+	new(&c->weak_objects) std::map<zval *, v8js_persistent_obj_t>();
+
 	retval.handle = zend_objects_store_put(c, NULL, (zend_objects_free_object_storage_t) php_v8js_free_storage, NULL TSRMLS_CC);
 	retval.handlers = &v8js_object_handlers;
 

v8js_convert.cc

@@ -222,11 +222,18 @@ static void php_v8js_construct_callback(const v8::FunctionCallbackInfo<v8::Value
 	newobj->SetAlignedPointerInInternalField(0, ext_tmpl->Value());
 	newobj->SetHiddenValue(V8JS_SYM(PHPJS_OBJECT_KEY), php_object);
 
+#if PHP_V8_API_VERSION <= 3023008
+	/* Until V8 3.23.8 Isolate could only take one external pointer. */
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData();
+#else
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData(0);
+#endif
+
 	// Since we got to decrease the reference count again, in case v8 garbage collector
 	// decides to dispose the JS object, we add a weak persistent handle and register
 	// a callback function that removes the reference.
-	v8::Persistent<v8::Object> persist_newobj(isolate, newobj);
-	persist_newobj.SetWeak(value, php_v8js_weak_object_callback);
+	ctx->weak_objects[value].Reset(isolate, newobj);
+	ctx->weak_objects[value].SetWeak(value, php_v8js_weak_object_callback);
 
 	// Just tell v8 that we're allocating some external memory
 	// (for the moment we just always tell 1k instead of trying to find out actual values)
@@ -257,16 +264,40 @@ static int _php_v8js_is_assoc_array(HashTable *myht TSRMLS_DC) /* {{{ */
 /* }}} */
 
 static void php_v8js_weak_object_callback(const v8::WeakCallbackData<v8::Object, zval> &data) {
+	v8::Isolate *isolate = data.GetIsolate();
+
 	zval *value = data.GetParameter();
 	zval_ptr_dtor(&value);
 
-	data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(-1024);
+#if PHP_V8_API_VERSION <= 3023008
+	/* Until V8 3.23.8 Isolate could only take one external pointer. */
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData();
+#else
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData(0);
+#endif
+
+	ctx->weak_objects.at(value).Reset();
+	ctx->weak_objects.erase(value);
+
+	isolate->AdjustAmountOfExternalAllocatedMemory(-1024);
 }
 
 static void php_v8js_weak_closure_callback(const v8::WeakCallbackData<v8::Object, v8js_tmpl_t> &data) {
+	v8::Isolate *isolate = data.GetIsolate();
+
 	v8js_tmpl_t *persist_tpl_ = data.GetParameter();
 	persist_tpl_->Reset();
 	delete persist_tpl_;
+
+#if PHP_V8_API_VERSION <= 3023008
+	/* Until V8 3.23.8 Isolate could only take one external pointer. */
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData();
+#else
+	php_v8js_ctx *ctx = (php_v8js_ctx *) isolate->GetData(0);
+#endif
+
+	ctx->weak_closures.at(persist_tpl_).Reset();
+	ctx->weak_closures.erase(persist_tpl_);
 };
 
 /* These are not defined by Zend */
@@ -746,8 +777,8 @@ static v8::Handle<v8::Value> php_v8js_hash_to_jsobj(zval *value, v8::Isolate *is
 
 		if (ce == zend_ce_closure) {
 			// free uncached function template when object is freed
-			v8::Persistent<v8::Object> persist_newobj2(isolate, newobj);
-			persist_newobj2.SetWeak(persist_tpl_, php_v8js_weak_closure_callback);
+			ctx->weak_closures[persist_tpl_].Reset(isolate, newobj);
+			ctx->weak_closures[persist_tpl_].SetWeak(persist_tpl_, php_v8js_weak_closure_callback);
 		}
 	} else {
 		// @todo re-use template likewise