Merge pull request #150 from stesie/issue-140

Fix module caching & memory leaks

tests/commonjs_caching_001.phpt

@@ -0,0 +1,26 @@
+--TEST--
+Test V8Js::setModuleLoader : Returned modules are cached
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("test");
+var bar = require("test2");
+var baz = require("test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for test
+setModuleLoader called for test2
+===EOF===

tests/commonjs_caching_002.phpt

@@ -0,0 +1,34 @@
+--TEST--
+Test V8Js::setModuleLoader : module cache seperated per isolate
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("test");
+var baz = require("test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8two = new V8Js();
+$v8two->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+echo "--- v8two ---\n";
+$v8two->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for test
+--- v8two ---
+setModuleLoader called for test
+===EOF===

tests/commonjs_normalise_001.phpt

@@ -0,0 +1,23 @@
+--TEST--
+Test V8Js::setModuleLoader : Path normalisation #001
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("./test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for test
+===EOF===

tests/commonjs_normalise_002.phpt

@@ -0,0 +1,23 @@
+--TEST--
+Test V8Js::setModuleLoader : Path normalisation #002
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("../../../test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for test
+===EOF===

tests/commonjs_normalise_003.phpt

@@ -0,0 +1,27 @@
+--TEST--
+Test V8Js::setModuleLoader : Path normalisation #003
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("foo/test");
+var foo = require("foo/bar/baz/test");
+var foo = require("foo//bar//baz//blub");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+    return 'exports.bar = 23;';
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for foo/test
+setModuleLoader called for foo/bar/baz/test
+setModuleLoader called for foo/bar/baz/blub
+===EOF===

tests/commonjs_normalise_004.phpt

@@ -0,0 +1,30 @@
+--TEST--
+Test V8Js::setModuleLoader : Path normalisation #004
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("foo/test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+
+    switch($module) {
+    case 'foo/test':
+        return 'require("./blar");';
+    case 'foo/blar':
+        return 'exports.bar = 23;';
+    }
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for foo/test
+setModuleLoader called for foo/blar
+===EOF===

tests/commonjs_normalise_005.phpt

@@ -0,0 +1,30 @@
+--TEST--
+Test V8Js::setModuleLoader : Path normalisation #005
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--FILE--
+<?php
+
+$JS = <<< EOT
+var foo = require("foo/test");
+EOT;
+
+$v8 = new V8Js();
+$v8->setModuleLoader(function($module) {
+    print("setModuleLoader called for ".$module."\n");
+
+    switch($module) {
+    case 'foo/test':
+        return 'require("../blar");';
+    case 'blar':
+        return 'exports.bar = 23;';
+    }
+});
+
+$v8->executeString($JS, 'module.js');
+?>
+===EOF===
+--EXPECT--
+setModuleLoader called for foo/test
+setModuleLoader called for blar
+===EOF===

v8js_class.cc

@@ -165,6 +165,7 @@ static void v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 	/* 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) {
+		efree(it->first);
 		it->second.Reset();
 	}
 	c->modules_loaded.~map();
@@ -209,7 +210,7 @@ static zend_object_value 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->modules_loaded) std::map<char *, v8js_persistent_obj_t, cmp_str>;
 
 	new(&c->template_cache) std::map<const char *,v8js_tmpl_t>();
 	new(&c->accessor_list) std::vector<v8js_accessor_ctx *>();

v8js_class.h

@@ -24,6 +24,12 @@ struct v8js_v8object;
 struct v8js_accessor_ctx;
 struct _v8js_script;
 
+struct cmp_str {
+    bool operator()(char const *a, char const *b) const {
+        return strcmp(a, b) < 0;
+    }
+};
+
 /* {{{ Context container */
 struct v8js_ctx {
   zend_object std;
@@ -44,7 +50,7 @@ struct 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<char *, v8js_persistent_obj_t, cmp_str> modules_loaded;
   std::map<const char *,v8js_tmpl_t> template_cache;
 
   std::map<zval *, v8js_persistent_obj_t> weak_objects;

v8js_commonjs.cc

@@ -22,22 +22,18 @@ extern "C" {
 
 #include "php_v8js_macros.h"
 
-void v8js_commonjs_split_terms(char *identifier, std::vector<char *> &terms)
+static void v8js_commonjs_split_terms(const char *identifier, std::vector<char *> &terms)
 {
-    char *term = (char *)malloc(PATH_MAX), *ptr = term;
+    char *term = (char *) emalloc(PATH_MAX), *ptr = term;
-
-    // Initialise the term string
-    *term = 0;
 
     while (*identifier > 0) {
         if (*identifier == '/') {
-            if (strlen(term) > 0) {
+            if (ptr > term) {
                 // Terminate term string and add to terms vector
                 *ptr++ = 0;
-                terms.push_back(strdup(term));
+                terms.push_back(estrdup(term));
 
                 // Reset term string
-                memset(term, 0, strlen(term));
                 ptr = term;
             }
         } else {
@@ -47,18 +43,16 @@ void v8js_commonjs_split_terms(char *identifier, std::vector<char *> &terms)
         identifier++;
     }
 
-    if (strlen(term) > 0) {
+    if (ptr > term) {
         // Terminate term string and add to terms vector
         *ptr++ = 0;
-        terms.push_back(strdup(term));
+        terms.push_back(estrdup(term));
     }
 
-    if (term > 0) {
+    efree(term);
-        free(term);
-    }
 }
 
-void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name)
+void v8js_commonjs_normalise_identifier(const char *base, const char *identifier, char *normalised_path, char *module_name)
 {
     std::vector<char *> id_terms, terms;
     v8js_commonjs_split_terms(identifier, id_terms);
@@ -78,12 +72,19 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm
         if (!strcmp(term, "..")) {
             // Ignore parent term (..) if it's the first normalised term
             if (normalised_terms.size() > 0) {
-                // Remove the parent normalized term
+                // Remove the parent normalized term (and free it)
+                efree(normalised_terms.back());
                 normalised_terms.pop_back();
             }
+
+            // free the ".." term
+            efree(term);
         } else if (strcmp(term, ".")) {
             // Add the term if it's not the current term (.)
             normalised_terms.push_back(term);
+        } else {
+            // Discard "." term
+            efree(term);
         }
     }
 
@@ -92,6 +93,8 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm
     *module_name = 0;
 
     strcat(module_name, normalised_terms.back());
+
+    efree(normalised_terms.back());
     normalised_terms.pop_back();
 
     for (std::vector<char *>::iterator it = normalised_terms.begin(); it != normalised_terms.end(); it++) {
@@ -102,5 +105,6 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm
         }
 
         strcat(normalised_path, term);
+        efree(term);
     }
 }

v8js_commonjs.h

@@ -0,0 +1,18 @@
+/*
+  +----------------------------------------------------------------------+
+  | PHP Version 5                                                        |
+  +----------------------------------------------------------------------+
+  | Copyright (c) 1997-2015 The PHP Group                                |
+  +----------------------------------------------------------------------+
+  | http://www.opensource.org/licenses/mit-license.php  MIT License      |
+  +----------------------------------------------------------------------+
+  | Author: Stefan Siegl <stesie@brokenpipe.de>                          |
+  +----------------------------------------------------------------------+
+*/
+
+#ifndef V8JS_COMMONJS_H
+#define V8JS_COMMONJS_H
+
+void v8js_commonjs_normalise_identifier(const char *base, const char *identifier, char *normalised_path, char *module_name);
+
+#endif /* V8JS_COMMONJS_H */

v8js_methods.cc

@@ -16,14 +16,12 @@
 #endif
 
 #include "php_v8js_macros.h"
+#include "v8js_commonjs.h"
 
 extern "C" {
 #include "zend_exceptions.h"
 }
 
-/* Forward declarations */
-void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name);
-
 /* global.exit - terminate execution */
 V8JS_METHOD(exit) /* {{{ */
 {
@@ -223,13 +221,11 @@ V8JS_METHOD(require)
 
 	v8::String::Utf8Value module_id_v8(info[0]);
 
-	// Make sure to duplicate the module name string so it doesn't get freed by the V8 garbage collector
+	const char *module_id = ToCString(module_id_v8);
-	char *module_id = estrdup(ToCString(module_id_v8));
 	char *normalised_path = (char *)emalloc(PATH_MAX);
 	char *module_name = (char *)emalloc(PATH_MAX);
 
 	v8js_commonjs_normalise_identifier(c->modules_base.back(), module_id, normalised_path, module_name);
-	efree(module_id);
 
 	char *normalised_module_id = (char *)emalloc(strlen(normalised_path)+1+strlen(module_name)+1);
 	*normalised_module_id = 0;
@@ -256,12 +252,13 @@ V8JS_METHOD(require)
 
     // If we have already loaded and cached this module then use it
 	if (c->modules_loaded.count(normalised_module_id) > 0) {
-		efree(normalised_module_id);
-		efree(normalised_path);
-
 		v8::Persistent<v8::Object> newobj;
 		newobj.Reset(isolate, c->modules_loaded[normalised_module_id]);
 		info.GetReturnValue().Set(newobj);
+
+		efree(normalised_module_id);
+		efree(normalised_path);
+
 		return;
 	}
 
@@ -400,7 +397,6 @@ V8JS_METHOD(require)
 
 	c->modules_loaded[normalised_module_id].Reset(isolate, newobj);
 	info.GetReturnValue().Set(newobj);
-	efree(normalised_module_id);
 }
 
 void v8js_register_methods(v8::Handle<v8::ObjectTemplate> global, v8js_ctx *c) /* {{{ */