Fix module caching, closes #107

Use v8::Persistent handle to keep module instances around.

Objects cannot be shared between isolates anyhow, hence moved
modules_loaded map from global V8JSG structure to php_v8js_ctx.

Besides fixes a use-after-free on normalised_module_id.

Author: stesie

php_v8js_macros.h

@@ -200,6 +200,7 @@ struct php_v8js_ctx {
   zval *module_loader;
   std::vector<char *> modules_stack;
   std::vector<char *> modules_base;
+  std::map<char *, v8js_persistent_obj_t> modules_loaded;
   std::map<const char *,v8js_tmpl_t> template_cache;
 
   std::map<zval *, v8js_persistent_obj_t> weak_objects;
@@ -262,8 +263,6 @@ ZEND_BEGIN_MODULE_GLOBALS(v8js)
   std::mutex timer_mutex;
   bool timer_stop;
 
-  std::map<char *, v8::Handle<v8::Object> > modules_loaded;
-
   // fatal error unwinding
   bool fatal_error_abort;
   int error_num;

tests/commonjs_multiassign.phpt

@@ -0,0 +1,26 @@
+--TEST--
+Test V8Js::setModuleLoader : Assign result multiple times
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("test");
+var bar = require("test");
+var_dump(foo.bar);
+var_dump(bar.bar);
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+  return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+int(23)
+int(23)
+===EOF===

v8js.cc

@@ -718,6 +718,13 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 	}
 	c->php_v8js_objects.~list();
 
+	/* Clear persistent handles in module cache */
+	for (std::map<char *, v8js_persistent_obj_t>::iterator it = c->modules_loaded.begin();
+		 it != c->modules_loaded.end(); ++it) {
+		it->second.Reset();
+	}
+	c->modules_loaded.~map();
+
 	c->isolate->Dispose();
 
 	if(c->tz != NULL) {
@@ -726,6 +733,7 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 
 	c->modules_stack.~vector();
 	c->modules_base.~vector();
+
 	efree(object);
 }
 /* }}} */
@@ -753,6 +761,8 @@ static zend_object_value php_v8js_new(zend_class_entry *ce TSRMLS_DC) /* {{{ */
 
 	new(&c->modules_stack) std::vector<char*>();
 	new(&c->modules_base) std::vector<char*>();
+	new(&c->modules_loaded) std::map<char *, v8js_persistent_obj_t>;
+
 	new(&c->template_cache) std::map<const char *,v8js_tmpl_t>();
 	new(&c->accessor_list) std::vector<php_v8js_accessor_ctx *>();
 
@@ -1957,11 +1967,11 @@ static PHP_GINIT_FUNCTION(v8js)
 	v8js_globals->extensions = NULL;
 	v8js_globals->v8_initialized = 0;
 	v8js_globals->v8_flags = NULL;
+
 	v8js_globals->timer_thread = NULL;
 	v8js_globals->timer_stop = false;
 	new(&v8js_globals->timer_mutex) std::mutex;
 	new(&v8js_globals->timer_stack) std::stack<php_v8js_timer_ctx *>;
-	new(&v8js_globals->modules_loaded) std::map<char *, v8::Handle<v8::Object>>;
 
 	v8js_globals->fatal_error_abort = 0;
 	v8js_globals->error_num = 0;
@@ -1990,7 +2000,6 @@ static PHP_GSHUTDOWN_FUNCTION(v8js)
 #ifdef ZTS
 	v8js_globals->timer_stack.~stack();
 	v8js_globals->timer_mutex.~mutex();
-	v8js_globals->modules_loaded.~map();
 #endif
 }
 /* }}} */

v8js_methods.cc

@@ -245,11 +245,13 @@ V8JS_METHOD(require)
     }
 
     // If we have already loaded and cached this module then use it
-	if (V8JSG(modules_loaded).count(normalised_module_id) > 0) {
+	if (c->modules_loaded.count(normalised_module_id) > 0) {
 		efree(normalised_module_id);
 		efree(normalised_path);
 
-		info.GetReturnValue().Set(V8JSG(modules_loaded)[normalised_module_id]);
+		v8::Persistent<v8::Object> newobj;
+		newobj.Reset(isolate, c->modules_loaded[normalised_module_id]);
+		info.GetReturnValue().Set(newobj);
 		return;
 	}
 
@@ -324,7 +326,7 @@ V8JS_METHOD(require)
 	v8::Locker locker(isolate);
 	v8::Isolate::Scope isolate_scope(isolate);
 
-	v8::EscapableHandleScope handle_scope(isolate);
+	v8::HandleScope handle_scope(isolate);
 
 	// Enter the module context
 	v8::Context::Scope scope(context);
@@ -357,33 +359,38 @@ V8JS_METHOD(require)
 	c->modules_stack.pop_back();
 	c->modules_base.pop_back();
 
-	efree(normalised_module_id);
 	efree(normalised_path);
 
 	// Script possibly terminated, return immediately
 	if (!try_catch.CanContinue()) {
 		info.GetReturnValue().Set(isolate->ThrowException(V8JS_SYM("Module script compile failed")));
+		efree(normalised_module_id);
 		return;
 	}
 
 	// Handle runtime JS exceptions
 	if (try_catch.HasCaught()) {
 		// Rethrow the exception back to JS
 		info.GetReturnValue().Set(try_catch.ReThrow());
+		efree(normalised_module_id);
 		return;
 	}
 
+	v8::Handle<v8::Object> newobj;
+
 	// Cache the module so it doesn't need to be compiled and run again
 	// Ensure compatibility with CommonJS implementations such as NodeJS by playing nicely with module.exports and exports
 	if (module->Has(V8JS_SYM("exports")) && !module->Get(V8JS_SYM("exports"))->IsUndefined()) {
 		// If module.exports has been set then we cache this arbitrary value...
-		V8JSG(modules_loaded)[normalised_module_id] = handle_scope.Escape(module->Get(V8JS_SYM("exports"))->ToObject());
+		newobj = module->Get(V8JS_SYM("exports"))->ToObject();
 	} else {
 		// ...otherwise we cache the exports object itself
-		V8JSG(modules_loaded)[normalised_module_id] = handle_scope.Escape(exports);
+		newobj = exports;
 	}
 
-	info.GetReturnValue().Set(V8JSG(modules_loaded)[normalised_module_id]);
+	c->modules_loaded[normalised_module_id].Reset(isolate, newobj);
+	info.GetReturnValue().Set(newobj);
+	efree(normalised_module_id);
 }
 
 void php_v8js_register_methods(v8::Handle<v8::ObjectTemplate> global, php_v8js_ctx *c) /* {{{ */