Add some missing error propagation (#179)

* Use MaybeLocal ToLocal instead of IsEmpty and ToLocalChecked (closes #177, merged in):

Using a ToLocal makes it clearer that the error is being handled
which makes uses of ToLocalChecked stand out more as places where
we are missing error handling.

* Add valueResult to get the value and error from C results (closes #178, merged in):
  * Remove dead code path in getError to handle non-errors
  * Rename getError to newJSError and move to errors.go

* Add some missing error propagation:
  * Propogate errors in ObjectTemplate.NewInstance
  * Propogate new v8 string errors in Object.Get
  * Propagate NewPromiseResolver error
  * Propagate promise errors from C to Go
  * Remove unused Exception*Error C functions
  * Propagate v8::FunctionTemplate::GetFunction error from C to Go
  * Propagate new v8 string errors in Context.RunScript
  * Propagate NewValue errors for string and big.Int type values
  * Propagate v8::Value::ToObject error from C to Go
  * Propagate v8::Value::ToDetailString error from C to Go
  * Add a change log entry for adding missing error propagation

Author: dylanahsmith

CHANGELOG.md

@@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Removed error return value from NewObjectTemplate and NewFunctionTemplate. Panic if given a nil argument.
 - Function Call accepts receiver as first argument.
 
+### Fixed
+- Add some missing error propagation
+
 ## [v0.6.0] - 2021-05-11
 
 ### Added

context.go

@@ -94,7 +94,7 @@ func (c *Context) RunScript(source string, origin string) (*Value, error) {
 	rtn := C.RunScript(c.ptr, cSource, cOrigin)
 	c.deregister()
 
-	return getValue(c, rtn), getError(rtn)
+	return valueResult(c, rtn)
 }
 
 // Global returns the global proxy object.
@@ -165,31 +165,16 @@ func goContext(ref int) C.ContextPtr {
 	return ctx.ptr
 }
 
-func getValue(ctx *Context, rtn C.RtnValue) *Value {
+func valueResult(ctx *Context, rtn C.RtnValue) (*Value, error) {
 	if rtn.value == nil {
-		return nil
+		return nil, newJSError(rtn.error)
 	}
-	return &Value{rtn.value, ctx}
+	return &Value{rtn.value, ctx}, nil
 }
 
-func getObject(ctx *Context, rtn C.RtnValue) *Object {
+func objectResult(ctx *Context, rtn C.RtnValue) (*Object, error) {
 	if rtn.value == nil {
-		return nil
-	}
-	return &Object{&Value{rtn.value, ctx}}
-}
-
-func getError(rtn C.RtnValue) error {
-	if rtn.error.msg == nil {
-		return nil
-	}
-	err := &JSError{
-		Message:    C.GoString(rtn.error.msg),
-		Location:   C.GoString(rtn.error.location),
-		StackTrace: C.GoString(rtn.error.stack),
+		return nil, newJSError(rtn.error)
 	}
-	C.free(unsafe.Pointer(rtn.error.msg))
-	C.free(unsafe.Pointer(rtn.error.location))
-	C.free(unsafe.Pointer(rtn.error.stack))
-	return err
+	return &Object{&Value{rtn.value, ctx}}, nil
 }

errors.go

@@ -4,9 +4,13 @@
 
 package v8go
 
+// #include <stdlib.h>
+// #include "v8go.h"
+import "C"
 import (
 	"fmt"
 	"io"
+	"unsafe"
 )
 
 // JSError is an error that is returned if there is are any
@@ -18,6 +22,18 @@ type JSError struct {
 	StackTrace string
 }
 
+func newJSError(rtnErr C.RtnError) error {
+	err := &JSError{
+		Message:    C.GoString(rtnErr.msg),
+		Location:   C.GoString(rtnErr.location),
+		StackTrace: C.GoString(rtnErr.stack),
+	}
+	C.free(unsafe.Pointer(rtnErr.msg))
+	C.free(unsafe.Pointer(rtnErr.location))
+	C.free(unsafe.Pointer(rtnErr.stack))
+	return err
+}
+
 func (e *JSError) Error() string {
 	return e.Message
 }

function.go

@@ -28,7 +28,7 @@ func (fn *Function) Call(recv Valuer, args ...Valuer) (*Value, error) {
 	fn.ctx.register()
 	rtn := C.FunctionCall(fn.ptr, recv.value().ptr, C.int(len(args)), argptr)
 	fn.ctx.deregister()
-	return getValue(fn.ctx, rtn), getError(rtn)
+	return valueResult(fn.ctx, rtn)
 }
 
 // Invoke a constructor function to create an object instance.
@@ -44,7 +44,7 @@ func (fn *Function) NewInstance(args ...Valuer) (*Object, error) {
 	fn.ctx.register()
 	rtn := C.FunctionNewInstance(fn.ptr, C.int(len(args)), argptr)
 	fn.ctx.deregister()
-	return getObject(fn.ctx, rtn), getError(rtn)
+	return objectResult(fn.ctx, rtn)
 }
 
 // Return the source map url for a function.

function_template.go

@@ -65,9 +65,12 @@ func NewFunctionTemplate(iso *Isolate, callback FunctionCallback) *FunctionTempl
 
 // GetFunction returns an instance of this function template bound to the given context.
 func (tmpl *FunctionTemplate) GetFunction(ctx *Context) *Function {
-	// TODO: Consider propagating the v8::FunctionTemplate::GetFunction error
-	val_ptr := C.FunctionTemplateGetFunction(tmpl.ptr, ctx.ptr)
-	return &Function{&Value{val_ptr, ctx}}
+	rtn := C.FunctionTemplateGetFunction(tmpl.ptr, ctx.ptr)
+	val, err := valueResult(ctx, rtn)
+	if err != nil {
+		panic(err) // TODO: Consider returning the error
+	}
+	return &Function{val}
 }
 
 // Note that ideally `thisAndArgs` would be split into two separate arguments, but they were combined

json.go

@@ -22,7 +22,7 @@ func JSONParse(ctx *Context, str string) (*Value, error) {
 	defer C.free(unsafe.Pointer(cstr))
 
 	rtn := C.JSONParse(ctx.ptr, cstr)
-	return getValue(ctx, rtn), getError(rtn)
+	return valueResult(ctx, rtn)
 }
 
 // JSONStringify tries to stringify the JSON-serializable object value and returns it as string.

object.go

@@ -24,11 +24,11 @@ func (o *Object) MethodCall(methodName string, args ...Valuer) (*Value, error) {
 	defer C.free(unsafe.Pointer(ckey))
 
 	getRtn := C.ObjectGet(o.ptr, ckey)
-	err := getError(getRtn)
+	prop, err := valueResult(o.ctx, getRtn)
 	if err != nil {
 		return nil, err
 	}
-	fn, err := getValue(o.ctx, getRtn).AsFunction()
+	fn, err := prop.AsFunction()
 	if err != nil {
 		return nil, err
 	}
@@ -89,13 +89,13 @@ func (o *Object) Get(key string) (*Value, error) {
 	defer C.free(unsafe.Pointer(ckey))
 
 	rtn := C.ObjectGet(o.ptr, ckey)
-	return getValue(o.ctx, rtn), getError(rtn)
+	return valueResult(o.ctx, rtn)
 }
 
 // GetIdx tries to get a Value at a give Object index.
 func (o *Object) GetIdx(idx uint32) (*Value, error) {
 	rtn := C.ObjectGetIdx(o.ptr, C.uint32_t(idx))
-	return getValue(o.ctx, rtn), getError(rtn)
+	return valueResult(o.ctx, rtn)
 }
 
 // Has calls the abstract operation HasProperty(O, P) described in ECMA-262, 7.3.10.

object_template.go

@@ -55,9 +55,8 @@ func (o *ObjectTemplate) NewInstance(ctx *Context) (*Object, error) {
 		return nil, errors.New("v8go: Context cannot be <nil>")
 	}
 
-	// TODO: propagate v8 error
-	valPtr := C.ObjectTemplateNewInstance(o.ptr, ctx.ptr)
-	return &Object{&Value{valPtr, ctx}}, nil
+	rtn := C.ObjectTemplateNewInstance(o.ptr, ctx.ptr)
+	return objectResult(ctx, rtn)
 }
 
 func (o *ObjectTemplate) apply(opts *contextOptions) {

promise.go

@@ -39,10 +39,12 @@ func NewPromiseResolver(ctx *Context) (*PromiseResolver, error) {
 	if ctx == nil {
 		return nil, errors.New("v8go: Context is required")
 	}
-	ptr := C.NewPromiseResolver(ctx.ptr)
-	val := &Value{ptr, ctx}
-	// TODO: Propagate Promise::Resolver::New error
-	return &PromiseResolver{&Object{val}, nil}, nil
+	rtn := C.NewPromiseResolver(ctx.ptr)
+	obj, err := objectResult(ctx, rtn)
+	if err != nil {
+		return nil, err
+	}
+	return &PromiseResolver{obj, nil}, nil
 }
 
 // GetPromise returns the associated Promise object for this resolver.
@@ -99,20 +101,24 @@ func (p *Promise) Then(cbs ...FunctionCallback) *Promise {
 	p.ctx.register()
 	defer p.ctx.deregister()
 
-	var ptr C.ValuePtr
+	var rtn C.RtnValue
 	switch len(cbs) {
 	case 1:
 		cbID := p.ctx.iso.registerCallback(cbs[0])
-		ptr = C.PromiseThen(p.ptr, C.int(cbID))
+		rtn = C.PromiseThen(p.ptr, C.int(cbID))
 	case 2:
 		cbID1 := p.ctx.iso.registerCallback(cbs[0])
 		cbID2 := p.ctx.iso.registerCallback(cbs[1])
-		ptr = C.PromiseThen2(p.ptr, C.int(cbID1), C.int(cbID2))
+		rtn = C.PromiseThen2(p.ptr, C.int(cbID1), C.int(cbID2))
 
 	default:
 		panic("1 or 2 callbacks required")
 	}
-	return &Promise{&Object{&Value{ptr, p.ctx}}}
+	obj, err := objectResult(p.ctx, rtn)
+	if err != nil {
+		panic(err) // TODO: Return error
+	}
+	return &Promise{obj}
 }
 
 // Catch invokes the given function if the promise is rejected.
@@ -121,6 +127,10 @@ func (p *Promise) Catch(cb FunctionCallback) *Promise {
 	p.ctx.register()
 	defer p.ctx.deregister()
 	cbID := p.ctx.iso.registerCallback(cb)
-	ptr := C.PromiseCatch(p.ptr, C.int(cbID))
-	return &Promise{&Object{&Value{ptr, p.ctx}}}
+	rtn := C.PromiseCatch(p.ptr, C.int(cbID))
+	obj, err := objectResult(p.ctx, rtn)
+	if err != nil {
+		panic(err) // TODO: Return error
+	}
+	return &Promise{obj}
 }