Merge pull request #192 from stesie/fix-v8-relock

Fix v8::Unlocker behaviour and related memory leaks

v8js_class.cc

@@ -554,8 +554,17 @@ static PHP_METHOD(V8Js, executeString)
 	if (!res) {
 		RETURN_FALSE;
 	}
-	v8js_execute_script(getThis(), res, flags, time_limit, memory_limit, &return_value TSRMLS_CC);
-	v8js_script_free(res);
+
+	zend_try {
+		v8js_execute_script(getThis(), res, flags, time_limit, memory_limit, &return_value TSRMLS_CC);
+		v8js_script_free(res);
+	}
+	zend_catch {
+		v8js_script_free(res);
+		zend_bailout();
+	}
+	zend_end_try()
+
 	efree(res);
 }
 /* }}} */

v8js_methods.cc

@@ -354,27 +354,29 @@ V8JS_METHOD(require)
 	int call_result;
 	zval params[1];
 
-	zend_try {
-		{
-			isolate->Exit();
-			v8::Unlocker unlocker(isolate);
+	{
+		isolate->Exit();
+		v8::Unlocker unlocker(isolate);
 
+		zend_try {
 			ZVAL_STRING(&params[0], normalised_module_id);
 			call_result = call_user_function_ex(EG(function_table), NULL, &c->module_loader, &module_code, 1, params, 0, NULL TSRMLS_CC);
 		}
-
-		isolate->Enter();
-
-		if (call_result == FAILURE) {
-			info.GetReturnValue().Set(isolate->ThrowException(V8JS_SYM("Module loader callback failed")));
+		zend_catch {
+			v8js_terminate_execution(isolate);
+			V8JSG(fatal_error_abort) = 1;
 		}
+		zend_end_try();
 	}
-	zend_catch {
-		v8js_terminate_execution(isolate);
-		V8JSG(fatal_error_abort) = 1;
+
+	isolate->Enter();
+
+	if (V8JSG(fatal_error_abort)) {
 		call_result = FAILURE;
 	}
-	zend_end_try();
+	else if (call_result == FAILURE) {
+		info.GetReturnValue().Set(isolate->ThrowException(V8JS_SYM("Module loader callback failed")));
+	}
 
 	zval_dtor(&params[0]);
 

v8js_object_export.cc

@@ -114,11 +114,11 @@ static void v8js_call_php_func(zend_object *object, zend_function *method_ptr, v
 	fci.no_separation = 1;
 	info.GetReturnValue().Set(V8JS_NULL);
 
-	zend_try {
-		{
-			isolate->Exit();
-			v8::Unlocker unlocker(isolate);
+	{
+		isolate->Exit();
+		v8::Unlocker unlocker(isolate);
 
+		zend_try {
 			/* zend_fcall_info_cache */
 			fcc.initialized = 1;
 			fcc.function_handler = method_ptr;
@@ -128,14 +128,13 @@ static void v8js_call_php_func(zend_object *object, zend_function *method_ptr, v
 
 			zend_call_function(&fci, &fcc TSRMLS_CC);
 		}
-
-		isolate->Enter();
-	}
-	zend_catch {
-		v8js_terminate_execution(isolate);
-		V8JSG(fatal_error_abort) = 1;
+		zend_catch {
+			v8js_terminate_execution(isolate);
+			V8JSG(fatal_error_abort) = 1;
+		}
+		zend_end_try();
 	}
-	zend_end_try();
+	isolate->Enter();
 
 failure:
 	/* Cleanup */

v8js_v8.cc

@@ -82,123 +82,127 @@ void v8js_v8_call(v8js_ctx *c, zval **return_value,
 {
 	char *tz = NULL;
 
-	V8JS_CTX_PROLOGUE(c);
+	{
+		V8JS_CTX_PROLOGUE(c);
 
-	V8JSG(timer_mutex).lock();
-	c->time_limit_hit = false;
-	c->memory_limit_hit = false;
-	V8JSG(timer_mutex).unlock();
+		V8JSG(timer_mutex).lock();
+		c->time_limit_hit = false;
+		c->memory_limit_hit = false;
+		V8JSG(timer_mutex).unlock();
 
-	/* Catch JS exceptions */
-	v8::TryCatch try_catch;
+		/* Catch JS exceptions */
+		v8::TryCatch try_catch;
 
-	/* Set flags for runtime use */
-	c->flags = flags;
+		/* Set flags for runtime use */
+		c->flags = flags;
 
-	/* Check if timezone has been changed and notify V8 */
-	tz = getenv("TZ");
+		/* Check if timezone has been changed and notify V8 */
+		tz = getenv("TZ");
 
-	if (tz != NULL) {
-		if (c->tz == NULL) {
-			c->tz = strdup(tz);
-		}
-		else if (strcmp(c->tz, tz) != 0) {
-			v8::Date::DateTimeConfigurationChangeNotification(c->isolate);
+		if (tz != NULL) {
+			if (c->tz == NULL) {
+				c->tz = strdup(tz);
+			}
+			else if (strcmp(c->tz, tz) != 0) {
+				v8::Date::DateTimeConfigurationChangeNotification(c->isolate);
 
-			free(c->tz);
-			c->tz = strdup(tz);
+				free(c->tz);
+				c->tz = strdup(tz);
+			}
 		}
-	}
 
-	if (time_limit > 0 || memory_limit > 0) {
-		// If timer thread is not running then start it
-		if (!V8JSG(timer_thread)) {
-			// If not, start timer thread
-			V8JSG(timer_thread) = new std::thread(v8js_timer_thread, ZEND_MODULE_GLOBALS_BULK(v8js));
+		if (time_limit > 0 || memory_limit > 0) {
+			// If timer thread is not running then start it
+			if (!V8JSG(timer_thread)) {
+				// If not, start timer thread
+				V8JSG(timer_thread) = new std::thread(v8js_timer_thread, ZEND_MODULE_GLOBALS_BULK(v8js));
+			}
 		}
-	}
 
-	/* Always pass the timer to the stack so there can be follow-up changes to
-	 * the time & memory limit. */
-	v8js_timer_push(time_limit, memory_limit, c TSRMLS_CC);
+		/* Always pass the timer to the stack so there can be follow-up changes to
+		 * the time & memory limit. */
+		v8js_timer_push(time_limit, memory_limit, c TSRMLS_CC);
 
-	/* Execute script */
-	c->in_execution++;
-	v8::Local<v8::Value> result = v8_call(c->isolate);
-	c->in_execution--;
+		/* Execute script */
+		c->in_execution++;
+		v8::Local<v8::Value> result = v8_call(c->isolate);
+		c->in_execution--;
 
-	/* Pop our context from the stack and read (possibly updated) limits
-	 * into local variables. */
-	V8JSG(timer_mutex).lock();
-	v8js_timer_ctx *timer_ctx = V8JSG(timer_stack).front();
-	V8JSG(timer_stack).pop_front();
-	V8JSG(timer_mutex).unlock();
+		/* Pop our context from the stack and read (possibly updated) limits
+		 * into local variables. */
+		V8JSG(timer_mutex).lock();
+		v8js_timer_ctx *timer_ctx = V8JSG(timer_stack).front();
+		V8JSG(timer_stack).pop_front();
+		V8JSG(timer_mutex).unlock();
 
-	time_limit = timer_ctx->time_limit;
-	memory_limit = timer_ctx->memory_limit;
+		time_limit = timer_ctx->time_limit;
+		memory_limit = timer_ctx->memory_limit;
 
-	efree(timer_ctx);
+		efree(timer_ctx);
 
-	/* Check for fatal error marker possibly set by v8js_error_handler; just
-	 * rethrow the error since we're now out of V8. */
-	if(V8JSG(fatal_error_abort)) {
-		zend_bailout();
-	}
+		if(!V8JSG(fatal_error_abort)) {
+			char exception_string[64];
 
-	char exception_string[64];
+			if (c->time_limit_hit) {
+				// Execution has been terminated due to time limit
+				sprintf(exception_string, "Script time limit of %lu milliseconds exceeded", time_limit);
+				zend_throw_exception(php_ce_v8js_time_limit_exception, exception_string, 0 TSRMLS_CC);
+				return;
+			}
 
-	if (c->time_limit_hit) {
-		// Execution has been terminated due to time limit
-		sprintf(exception_string, "Script time limit of %lu milliseconds exceeded", time_limit);
-		zend_throw_exception(php_ce_v8js_time_limit_exception, exception_string, 0 TSRMLS_CC);
-		return;
-	}
+			if (c->memory_limit_hit) {
+				// Execution has been terminated due to memory limit
+				sprintf(exception_string, "Script memory limit of %lu bytes exceeded", memory_limit);
+				zend_throw_exception(php_ce_v8js_memory_limit_exception, exception_string, 0 TSRMLS_CC);
+				return;
+			}
 
-	if (c->memory_limit_hit) {
-		// Execution has been terminated due to memory limit
-		sprintf(exception_string, "Script memory limit of %lu bytes exceeded", memory_limit);
-		zend_throw_exception(php_ce_v8js_memory_limit_exception, exception_string, 0 TSRMLS_CC);
-		return;
-	}
+			if (!try_catch.CanContinue()) {
+				// At this point we can't re-throw the exception
+				return;
+			}
 
-	if (!try_catch.CanContinue()) {
-		// At this point we can't re-throw the exception
-		return;
-	}
+			/* There was pending exception left from earlier executions -> throw to PHP */
+			if (Z_TYPE(c->pending_exception) == IS_OBJECT) {
+				zend_throw_exception_object(&c->pending_exception TSRMLS_CC);
+				ZVAL_NULL(&c->pending_exception);
+			}
 
-	/* There was pending exception left from earlier executions -> throw to PHP */
-	if (Z_TYPE(c->pending_exception) == IS_OBJECT) {
-		zend_throw_exception_object(&c->pending_exception TSRMLS_CC);
-		ZVAL_NULL(&c->pending_exception);
-	}
+			/* Handle runtime JS exceptions */
+			if (try_catch.HasCaught()) {
 
-	/* Handle runtime JS exceptions */
-	if (try_catch.HasCaught()) {
+				/* Pending exceptions are set only in outer caller, inner caller exceptions are always rethrown */
+				if (c->in_execution < 1) {
 
-		/* Pending exceptions are set only in outer caller, inner caller exceptions are always rethrown */
-		if (c->in_execution < 1) {
+					/* Report immediately if report_uncaught is true */
+					if (c->report_uncaught) {
+						v8js_throw_script_exception(c->isolate, &try_catch TSRMLS_CC);
+						return;
+					}
 
-			/* Report immediately if report_uncaught is true */
-			if (c->report_uncaught) {
-				v8js_throw_script_exception(c->isolate, &try_catch TSRMLS_CC);
+					/* Exception thrown from JS, preserve it for future execution */
+					if (result.IsEmpty()) {
+						v8js_create_script_exception(&c->pending_exception, c->isolate, &try_catch TSRMLS_CC);
+						return;
+					}
+				}
+
+				/* Rethrow back to JS */
+				try_catch.ReThrow();
 				return;
 			}
 
-			/* Exception thrown from JS, preserve it for future execution */
-			if (result.IsEmpty()) {
-				v8js_create_script_exception(&c->pending_exception, c->isolate, &try_catch TSRMLS_CC);
-				return;
+			/* Convert V8 value to PHP value */
+			if (!result.IsEmpty()) {
+				v8js_to_zval(result, *return_value, flags, c->isolate TSRMLS_CC);
 			}
 		}
+	} /* /V8JS_CTX_PROLOGUE */
 
-		/* Rethrow back to JS */
-		try_catch.ReThrow();
-		return;
-	}
-
-	/* Convert V8 value to PHP value */
-	if (!result.IsEmpty()) {
-		v8js_to_zval(result, *return_value, flags, c->isolate TSRMLS_CC);
+	if(V8JSG(fatal_error_abort)) {
+		/* Check for fatal error marker possibly set by v8js_error_handler; just
+		 * rethrow the error since we're now out of V8. */
+		zend_bailout();
 	}
 }
 /* }}} */