Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes from template finalizer #182

Merged
merged 2 commits into from
Sep 27, 2021
Merged

Fix crashes from template finalizer #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b72aa91
Fix crash from template finalizer releasing V8 data, just free the wr…
dylanahsmith Sep 27, 2021
c7c6d8f
Keep alive the template while its C++ pointer is still being used
dylanahsmith Sep 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed
- Add some missing error propagation
- Fix crash from template finalizer releasing V8 data, let it be disposed with the isolate
- Fix crash by keeping alive the template while its C++ pointer is still being used

## [v0.6.0] - 2021-05-11

Expand Down
2 changes: 2 additions & 0 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package v8go
// #include "v8go.h"
import "C"
import (
"runtime"
"sync"
"unsafe"
)
Expand Down Expand Up @@ -72,6 +73,7 @@ func NewContext(opt ...ContextOption) *Context {
ptr: C.NewContext(opts.iso.ptr, opts.gTmpl.ptr, C.int(ref)),
iso: opts.iso,
}
runtime.KeepAlive(opts.gTmpl)
return ctx
}

Expand Down
1 change: 1 addition & 0 deletions function_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ 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 {
rtn := C.FunctionTemplateGetFunction(tmpl.ptr, ctx.ptr)
runtime.KeepAlive(tmpl)
val, err := valueResult(ctx, rtn)
if err != nil {
panic(err) // TODO: Consider returning the error
Expand Down
1 change: 1 addition & 0 deletions object_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func (o *ObjectTemplate) NewInstance(ctx *Context) (*Object, error) {
}

rtn := C.ObjectTemplateNewInstance(o.ptr, ctx.ptr)
runtime.KeepAlive(o)
return objectResult(ctx, rtn)
}

Expand Down
9 changes: 7 additions & 2 deletions template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ func (t *template) Set(name string, val interface{}, attributes ...PropertyAttri
C.TemplateSetValue(t.ptr, cname, newVal.ptr, C.int(attrs))
case *ObjectTemplate:
C.TemplateSetTemplate(t.ptr, cname, v.ptr, C.int(attrs))
runtime.KeepAlive(v)
case *FunctionTemplate:
C.TemplateSetTemplate(t.ptr, cname, v.ptr, C.int(attrs))
runtime.KeepAlive(v)
case *Value:
if v.IsObject() || v.IsExternal() {
return errors.New("v8go: unsupported property: value type must be a primitive or use a template")
Expand All @@ -52,12 +54,15 @@ func (t *template) Set(name string, val interface{}, attributes ...PropertyAttri
default:
return fmt.Errorf("v8go: unsupported property type `%T`, must be one of string, int32, uint32, int64, uint64, float64, *big.Int, *v8go.Value, *v8go.ObjectTemplate or *v8go.FunctionTemplate", v)
}
runtime.KeepAlive(t)

return nil
}

func (t *template) finalizer() {
C.TemplateFree(t.ptr)
// Using v8::PersistentBase::Reset() wouldn't be thread-safe to do from
// this finalizer goroutine so just free the wrapper and let the template
// itself get cleaned up when the isolate is disposed.
C.TemplateFreeWrapper(t.ptr)
t.ptr = nil
runtime.SetFinalizer(t, nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://pkg.go.dev/runtime#SetFinalizer

When the garbage collector finds an unreachable block with an associated finalizer, it clears the association and runs finalizer(obj) in a separate goroutine. This makes obj reachable again, but now without an associated finalizer.

so Go should will clear the finalizer for us, thus we don't need to do that here.

}
5 changes: 3 additions & 2 deletions v8go.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,9 @@ IsolateHStatistics IsolationGetHeapStatistics(IsolatePtr iso) {
HandleScope handle_scope(iso); \
Local<Template> tmpl = tmpl_ptr->ptr.Get(iso);

void TemplateFree(TemplatePtr ptr) {
delete ptr;
void TemplateFreeWrapper(TemplatePtr tmpl) {
tmpl->ptr.Empty(); // Just does `val_ = 0;` without calling V8::DisposeGlobal
delete tmpl;
}

void TemplateSetValue(TemplatePtr ptr,
Expand Down
2 changes: 1 addition & 1 deletion v8go.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ extern RtnValue JSONParse(ContextPtr ctx_ptr, const char* str);
const char* JSONStringify(ContextPtr ctx_ptr, ValuePtr val_ptr);
extern ValuePtr ContextGlobal(ContextPtr ctx_ptr);

extern void TemplateFree(TemplatePtr ptr);
extern void TemplateFreeWrapper(TemplatePtr ptr);
extern void TemplateSetValue(TemplatePtr ptr,
const char* name,
ValuePtr val_ptr,
Expand Down