track created values and free on isolate dispose (#86)

* track created values and free on isolate dispose * add simple test for Isolate disposal * extra test for new value base cases * track all new values and C fmt
rogchap · Mar 10, 2021 · 387e66e · 387e66e
1 parent 794781e
commit 387e66e
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 70 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 - Promise resolver and promise result
 
+### Fixed
+- Go GC attempting to free C memory (via finalizer) of values after an Isolate is disposed causes a panic
+
 ## [v0.5.1] - 2021-02-19
 
 ### Fixed

diff --git a/context.go b/context.go
@@ -9,7 +9,6 @@ package v8go
 import "C"
 import (
 	"fmt"
-	"runtime"
 	"sync"
 	"unsafe"
 )
@@ -78,7 +77,6 @@ func NewContext(opt ...ContextOption) (*Context, error) {
 		ptr: C.NewContext(opts.iso.ptr, opts.gTmpl.ptr, C.int(ref)),
 		iso: opts.iso,
 	}
-	runtime.SetFinalizer(ctx, (*Context).finalizer)
 	// TODO: [RC] catch any C++ exceptions and return as error
 	return ctx, nil
 }
@@ -122,13 +120,8 @@ func (c *Context) Global() *Object {
 // Close will dispose the context and free the memory.
 // Access to any values assosiated with the context after calling Close may panic.
 func (c *Context) Close() {
-	c.finalizer()
-}
-
-func (c *Context) finalizer() {
 	C.ContextFree(c.ptr)
 	c.ptr = nil
-	runtime.SetFinalizer(c, nil)
 }
 
 func (c *Context) register() {

diff --git a/isolate.go b/isolate.go
@@ -8,7 +8,6 @@ package v8go
 import "C"
 
 import (
-	"runtime"
 	"sync"
 )
 
@@ -55,7 +54,6 @@ func NewIsolate() (*Isolate, error) {
 		ptr: C.NewIsolate(),
 		cbs: make(map[int]FunctionCallback),
 	}
-	runtime.SetFinalizer(iso, (*Isolate).finalizer)
 	// TODO: [RC] catch any C++ exceptions and return as error
 	return iso, nil
 }
@@ -87,23 +85,18 @@ func (i *Isolate) GetHeapStatistics() HeapStatistics {
 
 // Dispose will dispose the Isolate VM; subsequent calls will panic.
 func (i *Isolate) Dispose() {
-	i.finalizer()
-}
-
-// Deprecated: use `iso.Dispose()`.
-func (i *Isolate) Close() {
-	i.Dispose()
-}
-
-func (i *Isolate) finalizer() {
-	defer runtime.SetFinalizer(i, nil)
 	if i.ptr == nil {
 		return
 	}
 	C.IsolateDispose(i.ptr)
 	i.ptr = nil
 }
 
+// Deprecated: use `iso.Dispose()`.
+func (i *Isolate) Close() {
+	i.Dispose()
+}
+
 func (i *Isolate) apply(opts *contextOptions) {
 	opts.iso = i
 }

diff --git a/isolate_test.go b/isolate_test.go
@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"math/rand"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -76,6 +77,43 @@ func TestCallbackRegistry(t *testing.T) {
 	}
 }
 
+func TestIsolateDispose(t *testing.T) {
+	t.Parallel()
+
+	iso, _ := v8go.NewIsolate()
+	if iso.GetHeapStatistics().TotalHeapSize == 0 {
+		t.Error("Isolate incorrectly allocated")
+	}
+
+	iso.Dispose()
+	// noop when called multiple times
+	iso.Dispose()
+	// deprecated
+	iso.Close()
+
+	if iso.GetHeapStatistics().TotalHeapSize != 0 {
+		t.Error("Isolate not disposed correctly")
+	}
+}
+
+func TestIsolateGarbageCollection(t *testing.T) {
+	t.Parallel()
+
+	iso, _ := v8go.NewIsolate()
+	val, _ := v8go.NewValue(iso, "some string")
+	fmt.Println(val.String())
+
+	tmpl, _ := v8go.NewObjectTemplate(iso)
+	tmpl.Set("foo", "bar")
+	v8go.NewContext(iso, tmpl)
+
+	iso.Dispose()
+
+	runtime.GC()
+
+	time.Sleep(time.Second)
+}
+
 func BenchmarkIsolateInitialization(b *testing.B) {
 	b.ReportAllocs()
 	for n := 0; n < b.N; n++ {

diff --git a/v8go.cc b/v8go.cc
@@ -160,9 +160,7 @@ void IsolateDispose(IsolatePtr ptr) {
     return;
   }
   Isolate* iso = static_cast<Isolate*>(ptr);
-  m_ctx* ctx = static_cast<m_ctx*>(iso->GetData(0));
-  ctx->ptr.Reset();
-  delete ctx;
+  ContextFree(iso->GetData(0));
 
   iso->Dispose();
 }
@@ -502,91 +500,94 @@ ValuePtr ContextGlobal(ContextPtr ctx_ptr) {
   Context::Scope context_scope(local_ctx);      \
   Local<Value> value = val->ptr.Get(iso);
 
+#define ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr) \
+  ISOLATE_SCOPE(iso_ptr);                       \
+  m_ctx* ctx = static_cast<m_ctx*>(iso->GetData(0));
+
 ValuePtr NewValueInteger(IsolatePtr iso_ptr, int32_t v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, Integer::New(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso_ptr, uint32_t v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, Integer::NewFromUnsigned(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueString(IsolatePtr iso_ptr, const char* v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, String::NewFromUtf8(iso, v).ToLocalChecked());
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueBoolean(IsolatePtr iso_ptr, int v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, Boolean::New(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueNumber(IsolatePtr iso_ptr, double v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, Number::New(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueBigInt(IsolatePtr iso_ptr, int64_t v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, BigInt::New(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueBigIntFromUnsigned(IsolatePtr iso_ptr, uint64_t v) {
-  ISOLATE_SCOPE(iso_ptr);
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, BigInt::NewFromUnsigned(iso, v));
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 ValuePtr NewValueBigIntFromWords(IsolatePtr iso_ptr,
                                  int sign_bit,
                                  int word_count,
                                  const uint64_t* words) {
-  ISOLATE_SCOPE(iso_ptr);
-  m_ctx* ctx = static_cast<m_ctx*>(iso->GetData(0));
+  ISOLATE_SCOPE_INTERNAL_CONTEXT(iso_ptr);
 
   m_value* val = new m_value;
   val->iso = iso;
-  val->ctx = nullptr;
+  val->ctx = ctx;
   MaybeLocal<BigInt> bigint =
       BigInt::NewFromWords(ctx->ptr.Get(iso), sign_bit, word_count, words);
   val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
       iso, bigint.ToLocalChecked());
-  return static_cast<ValuePtr>(val);
+  return tracked_value(ctx, val);
 }
 
 void ValueFree(ValuePtr ptr) {
@@ -976,7 +977,8 @@ RtnValue ObjectGet(ValuePtr ptr, const char* key) {
   m_value* new_val = new m_value;
   new_val->iso = iso;
   new_val->ctx = ctx;
-  new_val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, result.ToLocalChecked());
+  new_val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, result.ToLocalChecked());
 
   rtn.value = tracked_value(ctx, new_val);
   return rtn;
@@ -1033,7 +1035,8 @@ ValuePtr NewPromiseResolver(ContextPtr ctx_ptr) {
   m_value* val = new m_value;
   val->iso = iso;
   val->ctx = ctx;
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, resolver.ToLocalChecked());
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, resolver.ToLocalChecked());
   return tracked_value(ctx, val);
 }
 
@@ -1044,7 +1047,8 @@ ValuePtr PromiseResolverGetPromise(ValuePtr ptr) {
   m_value* promise_val = new m_value;
   promise_val->iso = iso;
   promise_val->ctx = ctx;
-  promise_val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, promise);
+  promise_val->ptr =
+      Persistent<Value, CopyablePersistentTraits<Value>>(iso, promise);
   return tracked_value(ctx, promise_val);
 }
 
@@ -1075,7 +1079,8 @@ ValuePtr PromiseResult(ValuePtr ptr) {
   m_value* result_val = new m_value;
   result_val->iso = iso;
   result_val->ctx = ctx;
-  result_val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, result);
+  result_val->ptr =
+      Persistent<Value, CopyablePersistentTraits<Value>>(iso, result);
   return tracked_value(ctx, result_val);
 }
 
@@ -1088,48 +1093,58 @@ ValuePtr ExceptionError(IsolatePtr iso_ptr, const char* message) {
   val->iso = iso;
   val->ctx = nullptr;
   // TODO(rogchap): This currently causes a segfault, and I'm not sure why!
-  // Even a simple error with an empty string causes the error: Exception::Error(String::Empty(iso))
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, Exception::Error(msg));
+  // Even a simple error with an empty string causes the error:
+  // Exception::Error(String::Empty(iso))
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, Exception::Error(msg));
   return static_cast<ValuePtr>(val);
 }
 
 ValuePtr ExceptionRangeError(IsolatePtr iso_ptr, const char* message) {
   ISOLATE_SCOPE(iso_ptr);
-  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal).ToLocalChecked();
+  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal)
+                          .ToLocalChecked();
   m_value* val = new m_value;
   val->iso = iso;
   val->ctx = nullptr;
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, Exception::RangeError(msg));
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, Exception::RangeError(msg));
   return static_cast<ValuePtr>(val);
 }
 
 ValuePtr ExceptionReferenceError(IsolatePtr iso_ptr, const char* message) {
   ISOLATE_SCOPE(iso_ptr);
-  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal).ToLocalChecked();
+  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal)
+                          .ToLocalChecked();
   m_value* val = new m_value;
   val->iso = iso;
   val->ctx = nullptr;
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, Exception::ReferenceError(msg));
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, Exception::ReferenceError(msg));
   return static_cast<ValuePtr>(val);
 }
 
 ValuePtr ExceptionSyntaxError(IsolatePtr iso_ptr, const char* message) {
   ISOLATE_SCOPE(iso_ptr);
-  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal).ToLocalChecked();
+  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal)
+                          .ToLocalChecked();
   m_value* val = new m_value;
   val->iso = iso;
   val->ctx = nullptr;
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, Exception::SyntaxError(msg));
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, Exception::SyntaxError(msg));
   return static_cast<ValuePtr>(val);
 }
 
 ValuePtr ExceptionTypeError(IsolatePtr iso_ptr, const char* message) {
   ISOLATE_SCOPE(iso_ptr);
-  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal).ToLocalChecked();
+  Local<String> msg = String::NewFromUtf8(iso, message, NewStringType::kNormal)
+                          .ToLocalChecked();
   m_value* val = new m_value;
   val->iso = iso;
   val->ctx = nullptr;
-  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, Exception::TypeError(msg));
+  val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(
+      iso, Exception::TypeError(msg));
   return static_cast<ValuePtr>(val);
 }
 

diff --git a/v8go.h b/v8go.h
@@ -175,7 +175,8 @@ extern ValuePtr PromiseResult(ValuePtr ptr);
 
 extern ValuePtr ExceptionError(IsolatePtr iso_ptr, const char* message);
 extern ValuePtr ExceptionRangeError(IsolatePtr iso_ptr, const char* message);
-extern ValuePtr ExceptionReferenceError(IsolatePtr iso_ptr, const char* message);
+extern ValuePtr ExceptionReferenceError(IsolatePtr iso_ptr,
+                                        const char* message);
 extern ValuePtr ExceptionSyntaxError(IsolatePtr iso_ptr, const char* message);
 extern ValuePtr ExceptionTypeError(IsolatePtr iso_ptr, const char* message);