Properly free isolates on unclean shutdown

This fixes segfaults when isolate entered multiple time or when multiple isolates entered and unclean shutdown performed (when die/exit called or uncaught exception occurs).

Author: pinepain

src/php_v8_isolate.cc

@@ -44,20 +44,21 @@ static void php_v8_maybe_terminate_execution(php_v8_isolate_t *php_v8_isolate) {
 }
 
 static inline void php_v8_isolate_destroy(php_v8_isolate_t *php_v8_isolate) {
+    v8::Isolate *isolate = nullptr;
+
     if (php_v8_isolate->isolate) {
 
         php_v8_maybe_terminate_execution(php_v8_isolate);
 
-        while (php_v8_isolate->isolate->InContext()) {
-            v8::Local<v8::Context> context = php_v8_isolate->isolate->GetEnteredContext();
-            context->Exit();
-        }
-
-        if (php_v8_isolate->isolate == v8::Isolate::GetCurrent()) {
-            php_v8_isolate->isolate->Exit();
+        if (CG(unclean_shutdown)) {
+            // freeing order is not guaranteed upon unclean shutdown, so we explicitly exit all entered isolates,
+            // to ensure that current one won't remain entered so that we'll properly dispose it below
+            while ( (isolate = v8::Isolate::GetCurrent())) {
+                isolate->Exit();
+            }
         }
 
-        php_v8_isolate->isolate->Dispose(); // this cause error when we try to call on already entered context
+        php_v8_isolate->isolate->Dispose(); // this cause error when we try to call on already entered isolate
     }
 }
 

tests/.tracking_dtors.php

@@ -14,44 +14,50 @@
 
 namespace v8Tests\TrackingDtors;
 
-class Isolate extends \V8\Isolate {
-    public function __destruct() {
-        echo 'Isolate dies now!', PHP_EOL;
+
+trait DestructMessageAwareTrait {
+    //private $message = '';
+
+    //public function setOnDestructMessage($message) {
+    //    $this->message = $message;
+    //}
+
+    public function __destruct()
+    {
+        if (isset($this->destructor_test_message)) {
+            $message = $this->destructor_test_message;
+        }else {
+            $message = (new \ReflectionClass($this))->getShortName() . ' dies now!';
+        }
+
+        echo $message, PHP_EOL;
     }
 }
 
+class Isolate extends \V8\Isolate {
+    use DestructMessageAwareTrait;
+}
+
 class Context extends \V8\Context {
-    public function __destruct() {
-        echo 'Context dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }
 
 class Script extends \V8\Script {
-    public function __destruct() {
-        echo 'Script dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }
 
 class FunctionTemplate extends \V8\FunctionTemplate {
-    public function __destruct() {
-        echo 'FunctionTemplate dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }
 
 class ObjectTemplate extends \V8\ObjectTemplate {
-    public function __destruct() {
-        echo 'ObjectTemplate dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }
 
 class FunctionObject extends \V8\FunctionObject {
-    public function __destruct() {
-        echo 'FunctionObject dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }
 
 class Value extends \V8\Value {
-    public function __destruct() {
-        echo 'Value dies now!', PHP_EOL;
-    }
+    use DestructMessageAwareTrait;
 }

tests/V8FunctionObject_die_different_isolates.phpt

@@ -0,0 +1,64 @@
+--TEST--
+V8\FunctionObject - test die() called from different Isolate
+--SKIPIF--
+<?php if (!extension_loaded("v8")) {
+    print "skip";
+} ?>
+--FILE--
+<?php
+
+/** @var \Phpv8Testsuite $helper */
+$helper = require '.testsuite.php';
+
+require '.v8-helpers.php';
+$v8_helper = new PhpV8Helpers($helper);
+
+require '.tracking_dtors.php';
+
+$isolate_inner = new v8Tests\TrackingDtors\Isolate();
+$context_inner = new V8\Context($isolate_inner);
+
+$die_func = new v8Tests\TrackingDtors\FunctionObject($context_inner, function (\V8\FunctionCallbackInfo $info) {
+    echo 'going to die...', PHP_EOL;
+    die();
+});
+
+$context_inner->GlobalObject()->Set($context_inner, new \V8\StringValue($isolate_inner, 'die'), $die_func);
+
+$die_func->destructor_test_message      = 'die() function from inner isolate dtored';
+$isolate_inner->destructor_test_message = 'inner isolate dtored';
+
+
+$isolate_outer = new v8Tests\TrackingDtors\Isolate();
+$context_outer = new V8\Context($isolate_outer);
+
+
+$test_other_func = new v8Tests\TrackingDtors\FunctionObject($context_outer, function (\V8\FunctionCallbackInfo $info) use ($context_inner) {
+    echo 'calling inner...', PHP_EOL;
+    $isolate_inner = $context_inner->GetIsolate();
+    $global_inner = $context_inner->GlobalObject();
+
+    $global_inner->Get($context_inner, new \V8\StringValue($isolate_inner, 'die'))->Call($context_inner, $global_inner);
+});
+
+$test_other_func->destructor_test_message = 'test_other() function from outer isolate dtored';
+$isolate_outer->destructor_test_message = 'outer isolate dtored';
+
+
+$context_outer->GlobalObject()->Set($context_outer, new \V8\StringValue($isolate_outer, 'test_other'), $test_other_func);
+
+
+$res = $v8_helper->CompileRun($context_outer, 'test_other(); "Script done"');
+
+$helper->pretty_dump('Script result', $res->ToString($context_outer)->Value());
+
+echo 'We are done for now', PHP_EOL;
+
+?>
+--EXPECT--
+calling inner...
+going to die...
+test_other() function from outer isolate dtored
+inner isolate dtored
+die() function from inner isolate dtored
+outer isolate dtored

tests/V8FunctionObject_die_nested.phpt

@@ -0,0 +1,54 @@
+--TEST--
+V8\FunctionObject - test die() during nested calling
+--SKIPIF--
+<?php if (!extension_loaded("v8")) print "skip"; ?>
+--FILE--
+<?php
+
+/** @var \Phpv8Testsuite $helper */
+$helper = require '.testsuite.php';
+
+require '.v8-helpers.php';
+$v8_helper = new PhpV8Helpers($helper);
+
+require '.tracking_dtors.php';
+
+$isolate1         = new v8Tests\TrackingDtors\Isolate();
+$global_template1 = new V8\ObjectTemplate($isolate1);
+$context1         = new V8\Context($isolate1, $global_template1);
+
+
+$die_func = new v8Tests\TrackingDtors\FunctionObject($context1, function (\V8\FunctionCallbackInfo $info) {
+    echo 'going to die...', PHP_EOL;
+    die();
+});
+
+$die_func->destructor_test_message = 'die() function dtored';
+
+$teste_nested_func = new v8Tests\TrackingDtors\FunctionObject($context1, function (\V8\FunctionCallbackInfo $info) {
+    echo 'calling nested...', PHP_EOL;
+    $context = $info->GetContext();
+
+    $context->GlobalObject()->Get($context, new \V8\StringValue($context->GetIsolate(), 'die'))->Call($context, $context->GlobalObject());
+});
+
+$teste_nested_func->destructor_test_message = 'test_nested() function dtored';
+
+
+$context1->GlobalObject()->Set($context1, new \V8\StringValue($isolate1, 'die'), $die_func);
+$context1->GlobalObject()->Set($context1, new \V8\StringValue($isolate1, 'test_nested'), $teste_nested_func);
+
+
+$res = $v8_helper->CompileRun($context1, 'test_nested(); "Script done"');
+
+$helper->pretty_dump('Script result', $res->ToString($context1)->Value());
+
+echo 'We are done for now', PHP_EOL;
+
+?>
+--EXPECT--
+calling nested...
+going to die...
+test_nested() function dtored
+Isolate dies now!
+die() function dtored