Fix function callback usage outside of RunScript (rogchap#187)

Author: colinking

context.go

@@ -16,8 +16,6 @@ import (
 // Due to the limitations of passing pointers to C from Go we need to create
 // a registry so that we can lookup the Context from any given callback from V8.
 // This is similar to what is described here: https://github.com/golang/go/wiki/cgo#function-variables
-// To make sure we can still GC *Context we register the context only when we are
-// running a script inside the context and then deregister.
 type ctxRef struct {
 	ctx      *Context
 	refCount int
@@ -73,6 +71,7 @@ func NewContext(opt ...ContextOption) *Context {
 		ptr: C.NewContext(opts.iso.ptr, opts.gTmpl.ptr, C.int(ref)),
 		iso: opts.iso,
 	}
+	ctx.register()
 	runtime.KeepAlive(opts.gTmpl)
 	return ctx
 }
@@ -92,9 +91,7 @@ func (c *Context) RunScript(source string, origin string) (*Value, error) {
 	defer C.free(unsafe.Pointer(cSource))
 	defer C.free(unsafe.Pointer(cOrigin))
 
-	c.register()
 	rtn := C.RunScript(c.ptr, cSource, cOrigin)
-	c.deregister()
 
 	return valueResult(c, rtn)
 }
@@ -115,14 +112,13 @@ func (c *Context) Global() *Object {
 // PerformMicrotaskCheckpoint runs the default MicrotaskQueue until empty.
 // This is used to make progress on Promises.
 func (c *Context) PerformMicrotaskCheckpoint() {
-	c.register()
-	defer c.deregister()
 	C.IsolatePerformMicrotaskCheckpoint(c.iso.ptr)
 }
 
 // Close will dispose the context and free the memory.
-// Access to any values assosiated with the context after calling Close may panic.
+// Access to any values associated with the context after calling Close may panic.
 func (c *Context) Close() {
+	c.deregister()
 	C.ContextFree(c.ptr)
 	c.ptr = nil
 }

context_test.go

@@ -80,23 +80,18 @@ func TestContextRegistry(t *testing.T) {
 	ctxref := ctx.Ref()
 
 	c1 := v8.GetContext(ctxref)
-	if c1 != nil {
-		t.Error("expected context to be <nil>")
-	}
-
-	ctx.Register()
-	c2 := v8.GetContext(ctxref)
-	if c2 == nil {
+	if c1 == nil {
 		t.Error("expected context, but got <nil>")
 	}
-	if c2 != ctx {
-		t.Errorf("contexts should match %p != %p", c2, ctx)
+	if c1 != ctx {
+		t.Errorf("contexts should match %p != %p", c1, ctx)
 	}
-	ctx.Deregister()
 
-	c3 := v8.GetContext(ctxref)
-	if c3 != nil {
-		t.Error("expected context to be <nil>")
+	ctx.Close()
+
+	c2 := v8.GetContext(ctxref)
+	if c2 != nil {
+		t.Error("expected context to be <nil> after close")
 	}
 }
 
@@ -118,6 +113,44 @@ func TestMemoryLeak(t *testing.T) {
 	}
 }
 
+// https://github.com/rogchap/v8go/issues/186
+func TestRegistryFromJSON(t *testing.T) {
+	t.Parallel()
+
+	iso := v8.NewIsolate()
+	defer iso.Dispose()
+
+	global := v8.NewObjectTemplate(iso)
+	err := global.Set("location", v8.NewFunctionTemplate(iso, func(info *v8.FunctionCallbackInfo) *v8.Value {
+		v, err := v8.NewValue(iso, "world")
+		failIf(t, err)
+		return v
+	}))
+	failIf(t, err)
+
+	ctx := v8.NewContext(iso, global)
+	defer ctx.Close()
+
+	v, err := ctx.RunScript(`
+		new Proxy({
+			"hello": "unknown"
+		}, {
+			get: function () {
+				return location()
+			},
+		})
+	`, "main.js")
+	failIf(t, err)
+
+	s, err := v8.JSONStringify(ctx, v)
+	failIf(t, err)
+
+	expected := `{"hello":"world"}`
+	if s != expected {
+		t.Fatalf("expected %q, got %q", expected, s)
+	}
+}
+
 func BenchmarkContext(b *testing.B) {
 	b.ReportAllocs()
 	iso := v8.NewIsolate()

export_test.go

@@ -14,16 +14,6 @@ func (i *Isolate) GetCallback(ref int) FunctionCallback {
 	return i.getCallback(ref)
 }
 
-// Register is exported for testing only.
-func (c *Context) Register() {
-	c.register()
-}
-
-// Deregister is exported for testing only.
-func (c *Context) Deregister() {
-	c.deregister()
-}
-
 // GetContext is exported for testing only.
 var GetContext = getContext
 

function.go

@@ -25,9 +25,7 @@ func (fn *Function) Call(recv Valuer, args ...Valuer) (*Value, error) {
 		}
 		argptr = (*C.ValuePtr)(unsafe.Pointer(&cArgs[0]))
 	}
-	fn.ctx.register()
 	rtn := C.FunctionCall(fn.ptr, recv.value().ptr, C.int(len(args)), argptr)
-	fn.ctx.deregister()
 	return valueResult(fn.ctx, rtn)
 }
 
@@ -41,9 +39,7 @@ func (fn *Function) NewInstance(args ...Valuer) (*Object, error) {
 		}
 		argptr = (*C.ValuePtr)(unsafe.Pointer(&cArgs[0]))
 	}
-	fn.ctx.register()
 	rtn := C.FunctionNewInstance(fn.ptr, C.int(len(args)), argptr)
-	fn.ctx.deregister()
 	return objectResult(fn.ctx, rtn)
 }
 

promise.go

@@ -62,16 +62,12 @@ func (r *PromiseResolver) GetPromise() *Promise {
 // Resolve invokes the Promise resolve state with the given value.
 // The Promise state will transition from Pending to Fulfilled.
 func (r *PromiseResolver) Resolve(val Valuer) bool {
-	r.ctx.register()
-	defer r.ctx.deregister()
 	return C.PromiseResolverResolve(r.ptr, val.value().ptr) != 0
 }
 
 // Reject invokes the Promise reject state with the given value.
 // The Promise state will transition from Pending to Rejected.
 func (r *PromiseResolver) Reject(err *Value) bool {
-	r.ctx.register()
-	defer r.ctx.deregister()
 	return C.PromiseResolverReject(r.ptr, err.ptr) != 0
 }
 
@@ -98,9 +94,6 @@ func (p *Promise) Result() *Value {
 // The default MicrotaskPolicy processes them when the call depth decreases to 0.
 // Call (*Context).PerformMicrotaskCheckpoint to trigger it manually.
 func (p *Promise) Then(cbs ...FunctionCallback) *Promise {
-	p.ctx.register()
-	defer p.ctx.deregister()
-
 	var rtn C.RtnValue
 	switch len(cbs) {
 	case 1:
@@ -124,8 +117,6 @@ func (p *Promise) Then(cbs ...FunctionCallback) *Promise {
 // Catch invokes the given function if the promise is rejected.
 // See Then for other details.
 func (p *Promise) Catch(cb FunctionCallback) *Promise {
-	p.ctx.register()
-	defer p.ctx.deregister()
 	cbID := p.ctx.iso.registerCallback(cb)
 	rtn := C.PromiseCatch(p.ptr, C.int(cbID))
 	obj, err := objectResult(p.ctx, rtn)