Merge pull request #77 from pinepain/fix-external-exception-lost

Store exception object when external exception given, closes #76

Author: pinepain

src/php_v8_exceptions.h

@@ -53,8 +53,10 @@ extern void php_v8_throw_try_catch_exception(php_v8_context_t *php_v8_context, v
     php_v8_isolate_limits_maybe_stop_timer((php_v8_context)->php_v8_isolate);\
     if ((try_catch).HasCaught()) { \
         php_v8_throw_try_catch_exception((php_v8_context), &(try_catch)); \
+        php_v8_isolate_external_exceptions_maybe_clear((php_v8_context)->php_v8_isolate); \
         return; \
-    }
+    } \
+    php_v8_isolate_external_exceptions_maybe_clear((php_v8_context)->php_v8_isolate); \
 
 
 #define PHP_V8_TRY_CATCH_EXCEPTION_STORE_ISOLATE(to_zval, from_isolate_zv) zend_update_property(php_v8_try_catch_exception_class_entry, (to_zval), ZEND_STRL("isolate"), (from_isolate_zv));

src/php_v8_isolate.cc

@@ -64,6 +64,37 @@ static inline void php_v8_isolate_destroy(php_v8_isolate_t *php_v8_isolate) {
     }
 }
 
+void php_v8_isolate_external_exceptions_maybe_clear(php_v8_isolate_t *php_v8_isolate) {
+    if (!php_v8_isolate->limits.depth) {
+        php_v8_isolate->external_exceptions->clear();
+    }
+}
+
+namespace phpv8 {
+    int ExternalExceptionsStack::getGcCount() {
+        return static_cast<int>(exceptions.size());
+    }
+    void ExternalExceptionsStack::collectGcZvals(zval *& zv) {
+        for (auto const &item : exceptions) {
+            ZVAL_COPY_VALUE(zv++, &item);
+        }
+    }
+    void ExternalExceptionsStack::add(zval zv) {
+        assert(IS_OBJECT == Z_TYPE(zv));
+        Z_ADDREF(zv);
+        exceptions.push_back(zv);
+        assert(exceptions.size() < INT_MAX);
+    }
+    void ExternalExceptionsStack::clear() {
+        for (auto &item : exceptions) {
+            zval_ptr_dtor(&item);
+        }
+        exceptions.clear();
+    }
+    ExternalExceptionsStack::~ExternalExceptionsStack() {
+        clear();
+    }
+}
 
 static HashTable * php_v8_isolate_gc(zval *object, zval **table, int *n) {
     PHP_V8_ISOLATE_FETCH_INTO(object, php_v8_isolate);
@@ -73,6 +104,7 @@ static HashTable * php_v8_isolate_gc(zval *object, zval **table, int *n) {
     size += php_v8_isolate->weak_function_templates->getGcCount();
     size += php_v8_isolate->weak_object_templates->getGcCount();
     size += php_v8_isolate->weak_values->getGcCount();
+    size += php_v8_isolate->external_exceptions->getGcCount();
 
     if (php_v8_isolate->gc_data_count < size) {
         php_v8_isolate->gc_data = (zval *)safe_erealloc(php_v8_isolate->gc_data, size, sizeof(zval), 0);
@@ -85,6 +117,7 @@ static HashTable * php_v8_isolate_gc(zval *object, zval **table, int *n) {
     php_v8_isolate->weak_function_templates->collectGcZvals(gc_data);
     php_v8_isolate->weak_object_templates->collectGcZvals(gc_data);
     php_v8_isolate->weak_values->collectGcZvals(gc_data);
+    php_v8_isolate->external_exceptions->collectGcZvals(gc_data);
 
     *table = php_v8_isolate->gc_data;
     *n     = php_v8_isolate->gc_data_count;
@@ -109,6 +142,10 @@ static void php_v8_isolate_free(zend_object *object) {
         delete php_v8_isolate->weak_values;
     }
 
+    if (php_v8_isolate->external_exceptions) {
+        delete php_v8_isolate->external_exceptions;
+    }
+
     if (php_v8_isolate->gc_data) {
         efree(php_v8_isolate->gc_data);
     }
@@ -159,6 +196,7 @@ static zend_object *php_v8_isolate_ctor(zend_class_entry *ce) {
     php_v8_isolate->weak_function_templates = new phpv8::PersistentCollection<v8::FunctionTemplate>();
     php_v8_isolate->weak_object_templates = new phpv8::PersistentCollection<v8::ObjectTemplate>();
     php_v8_isolate->weak_values = new phpv8::PersistentCollection<v8::Value>();
+    php_v8_isolate->external_exceptions = new phpv8::ExternalExceptionsStack();
     new(&php_v8_isolate->key) v8::Persistent<v8::Private>();
 
     php_v8_isolate->std.handlers = &php_v8_isolate_object_handlers;
@@ -409,6 +447,7 @@ static PHP_METHOD(Isolate, getEnteredContext) {
 }
 
 static PHP_METHOD(Isolate, throwException) {
+    zval tmp;
     zval *php_v8_context_zv;
     zval *php_v8_value_zv;
     zval *exception_zv = NULL;
@@ -444,6 +483,9 @@ static PHP_METHOD(Isolate, throwException) {
         }
 
         ZVAL_COPY(&php_v8_value->exception, exception_zv);
+
+        ZVAL_OBJ(&tmp, &php_v8_value->std);
+        php_v8_isolate->external_exceptions->add(tmp);
     }
 
     isolate->ThrowException(local_value);

src/php_v8_isolate.h

@@ -20,7 +20,7 @@ typedef struct _php_v8_isolate_t php_v8_isolate_t;
 #include "php_v8_exceptions.h"
 #include "php_v8_callbacks.h"
 #include <v8.h>
-#include <map>
+#include <vector>
 
 extern "C" {
 #include "php.h"
@@ -34,6 +34,7 @@ extern zend_class_entry *php_v8_isolate_class_entry;
 
 inline php_v8_isolate_t * php_v8_isolate_fetch_object(zend_object *obj);
 inline v8::Local<v8::Private> php_v8_isolate_get_key_local(php_v8_isolate_t *php_v8_isolate);
+extern void php_v8_isolate_external_exceptions_maybe_clear(php_v8_isolate_t *php_v8_isolate);
 
 // TODO: remove or cleanup to use for debug reasons
 #define SX(x) #x
@@ -130,6 +131,20 @@ inline v8::Local<v8::Private> php_v8_isolate_get_key_local(php_v8_isolate_t *php
     }
 
 
+namespace phpv8 {
+
+    class ExternalExceptionsStack {
+    public:
+        int getGcCount();
+        void collectGcZvals(zval *& zv);
+        void add(zval zv);
+        void clear();
+        ~ExternalExceptionsStack();
+    private:
+        std::vector<zval> exceptions;
+    };
+}
+
 struct _php_v8_isolate_t {
     v8::Isolate *isolate;
     v8::Isolate::CreateParams *create_params;
@@ -138,6 +153,7 @@ struct _php_v8_isolate_t {
     phpv8::PersistentCollection<v8::FunctionTemplate> *weak_function_templates;
     phpv8::PersistentCollection<v8::ObjectTemplate> *weak_object_templates;
     phpv8::PersistentCollection<v8::Value> *weak_values;
+    phpv8::ExternalExceptionsStack *external_exceptions;
 
     v8::Persistent<v8::Private> key;
 

tests/Isolate_throwException_with_external_preserved.phpt

@@ -0,0 +1,72 @@
+--TEST--
+V8\Isolate::throwException() - external exception is not lost when provided with refcount=1
+--SKIPIF--
+<?php if (!extension_loaded("v8")) print "skip"; ?>
+--ENV--
+HOME=/tmp/we-need-home-env-var-set-to-load-valgrindrc
+--FILE--
+<?php
+/** @var \Phpv8Testsuite $helper */
+$helper = require '.testsuite.php';
+
+require '.v8-helpers.php';
+$v8_helper = new PhpV8Helpers($helper);
+
+
+class TestException extends RuntimeException {
+    public function __destruct()
+    {
+        echo __METHOD__, PHP_EOL;
+    }
+}
+
+$isolate = new \V8\Isolate();
+$context = new \V8\Context($isolate);
+$v8_helper->injectConsoleLog($context);
+
+$global = $context->globalObject();
+
+$func_tpl = new \V8\FunctionObject($context, function (\V8\FunctionCallbackInfo $info) {
+    $isolate = $info->getIsolate();
+    $context = $info->getContext();
+    $info->getIsolate()->throwException($info->getContext(), \V8\ExceptionManager::createError($context, new \V8\StringValue($isolate, 'test')), new TestException('test'));
+});
+
+$global->set($context, new \V8\StringValue($isolate, 'e'), $func_tpl);
+
+try {
+    $v8_helper->CompileRun($context, 'e()');
+} catch (\V8\Exceptions\TryCatchException $e) {
+    $helper->exception_export($e);
+    $helper->assert('External exception present', $e->getTryCatch()->getExternalException() instanceof TestException);
+    $helper->exception_export($e->getTryCatch()->getExternalException());
+    $e = null;
+}
+
+$helper->message('done');
+$helper->line();
+
+$helper->header('Run with js try-catch');
+$v8_helper->CompileRun($context, 'try {e()} catch(e) {}');
+
+$helper->message('done');
+
+$func_tpl = null;
+$global = null;
+$context = null;
+$v8_helper = null;
+
+$isolate = null;
+
+?>
+--EXPECT--
+V8\Exceptions\TryCatchException: Error: test
+External exception present: ok
+TestException: test
+TestException::__destruct
+done
+
+Run with js try-catch:
+----------------------
+TestException::__destruct
+done