diff --git a/errors/error.go b/errors/error.go index be7b7c89..2ada8c4a 100644 --- a/errors/error.go +++ b/errors/error.go @@ -105,13 +105,8 @@ func (err *Error) Stack() []byte { // stack. func (err *Error) StackFrames() []StackFrame { if err.frames == nil { - err.frames = make([]StackFrame, len(err.stack)) - - for i, pc := range err.stack { - err.frames[i] = NewStackFrame(pc) - } + err.frames = runtimeToErrorFrames(pcsToFrames(err.stack)) } - return err.frames } diff --git a/errors/error_test.go b/errors/error_test.go index 55cd0653..d530ffb0 100644 --- a/errors/error_test.go +++ b/errors/error_test.go @@ -4,28 +4,39 @@ import ( "bytes" "fmt" "io" - "runtime/debug" + "reflect" + "runtime" "testing" ) -func TestStackFormatMatches(t *testing.T) { +type prettyStack []StackFrame +func (s prettyStack) String() string { + buf := &bytes.Buffer{} + for _, f := range s { + _, _ = fmt.Fprintf(buf, "%#v\n", f) + } + return buf.String() +} + +func callerFrames(skip int) []runtime.Frame { + callers := make([]uintptr, MaxStackDepth) + n := runtime.Callers(2+skip, callers) + return pcsToFrames(callers[:n]) +} + +func TestStackFrameMatches(t *testing.T) { defer func() { err := recover() if err != 'a' { t.Fatal(err) } - bs := [][]byte{Errorf("hi").Stack(), debug.Stack()} + expected := runtimeToErrorFrames(callerFrames(0))[1:] + got := Errorf("hi").StackFrames()[1:] - // Ignore the first line (as it contains the PC of the .Stack() call) - bs[0] = bytes.SplitN(bs[0], []byte("\n"), 2)[1] - bs[1] = bytes.SplitN(bs[1], []byte("\n"), 2)[1] - - if bytes.Compare(bs[0], bs[1]) != 0 { - t.Errorf("Stack didn't match") - t.Errorf("%s", bs[0]) - t.Errorf("%s", bs[1]) + if !reflect.DeepEqual(expected, got) { + t.Errorf("Stacks didn't match\nGot:\n%v\nExpected:\n%v", prettyStack(got), prettyStack(expected)) } }() @@ -33,19 +44,17 @@ func TestStackFormatMatches(t *testing.T) { } func TestSkipWorks(t *testing.T) { - defer func() { err := recover() if err != 'a' { t.Fatal(err) } - bs := [][]byte{New("hi", 2).Stack(), debug.Stack()} + expected := runtimeToErrorFrames(callerFrames(2)) + got := New("hi", 2).StackFrames() - if !bytes.HasSuffix(bs[1], bs[0]) { - t.Errorf("Stack didn't match") - t.Errorf("%s", bs[0]) - t.Errorf("%s", bs[1]) + if !reflect.DeepEqual(expected, got) { + t.Errorf("Stacks didn't match\nGot:\n%v\nExpected:\n%v", prettyStack(got), prettyStack(expected)) } }() @@ -124,15 +133,6 @@ func ExampleNew_skip() { }() } -func ExampleError_Stack() { - e := New("Oh noes!", 1) - fmt.Printf("Error: %s\n", e.Error()) - fmt.Printf("Stack is %d bytes", len(e.Stack())) - // Output: - // Error: Oh noes! - // Stack is 589 bytes -} - func a() error { b(5) return nil diff --git a/errors/stackframe.go b/errors/stackframe.go index 4edadbc5..0c8303da 100644 --- a/errors/stackframe.go +++ b/errors/stackframe.go @@ -20,18 +20,35 @@ type StackFrame struct { // NewStackFrame popoulates a stack frame object from the program counter. func NewStackFrame(pc uintptr) (frame StackFrame) { - frame = StackFrame{ProgramCounter: pc} if frame.Func() == nil { return } - frame.Package, frame.Name = packageAndName(frame.Func()) + frame.Package, frame.Name = packageAndName(frame.Func().Name()) // pc -1 because the program counters we use are usually return addresses, // and we want to show the line that corresponds to the function call frame.File, frame.LineNumber = frame.Func().FileLine(pc - 1) return +} +// NewStackFrameFromRuntime populates a stack frame object from a runtime.Frame object. +func NewStackFrameFromRuntime(frame runtime.Frame) StackFrame { + var pkg, name string + if frame.Func != nil { + pkg, name = packageAndName(frame.Func.Name()) + } else if frame.Function != "" { + pkg, name = packageAndName(frame.Function) + } else { + return StackFrame{} + } + return StackFrame{ + File: frame.File, + LineNumber: frame.Line, + Name: name, + Package: pkg, + ProgramCounter: frame.PC, + } } // Func returns the function that this stackframe corresponds to @@ -71,10 +88,9 @@ func (frame *StackFrame) SourceLine() (string, error) { return string(bytes.Trim(lines[frame.LineNumber-1], " \t")), nil } -func packageAndName(fn *runtime.Func) (string, string) { - name := fn.Name() - pkg := "" - +// packageAndName splits a package path-qualified function name into the package path and function name. +func packageAndName(qualifiedName string) (pkg string, name string) { + name = qualifiedName // The name includes the path name to the package, which is unnecessary // since the file name is already included. Plus, it has center dots. // That is, we see @@ -93,5 +109,26 @@ func packageAndName(fn *runtime.Func) (string, string) { } name = strings.Replace(name, "ยท", ".", -1) - return pkg, name + return +} + +func pcsToFrames(pcs []uintptr) []runtime.Frame { + frames := runtime.CallersFrames(pcs) + s := make([]runtime.Frame, 0, len(pcs)) + for { + frame, more := frames.Next() + s = append(s, frame) + if !more { + break + } + } + return s +} + +func runtimeToErrorFrames(rtFrames []runtime.Frame) []StackFrame { + frames := make([]StackFrame, len(rtFrames)) + for i, f := range rtFrames { + frames[i] = NewStackFrameFromRuntime(f) + } + return frames } diff --git a/notifier_test.go b/notifier_test.go index 419e7541..010b9304 100644 --- a/notifier_test.go +++ b/notifier_test.go @@ -2,9 +2,11 @@ package bugsnag_test import ( "fmt" + "runtime" "testing" - simplejson "github.com/bitly/go-simplejson" + "github.com/bitly/go-simplejson" + "github.com/bugsnag/bugsnag-go" . "github.com/bugsnag/bugsnag-go/testutil" ) @@ -22,6 +24,26 @@ func crash(s interface{}) int { return s.(int) } +// numCrashFrames returns the number of frames to expect from calling crash(...) +func numCrashFrames(s interface{}) (n int) { + defer func() { + _ = recover() + pcs := make([]uintptr, 50) + // exclude Callers, deferred anon function, & numCrashFrames itself + m := runtime.Callers(3, pcs) + frames := runtime.CallersFrames(pcs[:m]) + for { + n++ + _, more := frames.Next() + if !more { + break + } + } + }() + crash(s) + return +} + func TestStackframesAreSkippedCorrectly(t *testing.T) { ts, reports := Setup() bugsnaggedReports = reports @@ -61,22 +83,13 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { assertStackframeCount(st, 3) }) - // Expect the following frames to be present for *.AutoNotify - /* - { "file": "runtime/panic.go", "method": "gopanic" }, - { "file": "runtime/iface.go", "method": "panicdottypeE" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func2.1" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func3" }, - { "file": "testing/testing.go", "method": "tRunner" }, - { "file": "runtime/asm_amd64.s", "method": "goexit" } - */ t.Run("notifier.AutoNotify", func(st *testing.T) { func() { defer func() { recover() }() defer notifier.AutoNotify() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) t.Run("bugsnag.AutoNotify", func(st *testing.T) { func() { @@ -84,31 +97,22 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { defer bugsnag.AutoNotify() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) - // Expect the following frames to be present for *.Recover - /* - { "file": "runtime/panic.go", "method": "gopanic" }, - { "file": "runtime/iface.go", "method": "panicdottypeE" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func4.1" }, - { "file": "$GOPATH/src/github.com/bugsnag/bugsnag-go/notifier_test.go", "method": "TestStackframesAreSkippedCorrectly.func4" }, - { "file": "testing/testing.go", "method": "tRunner" }, - { "file": "runtime/asm_amd64.s", "method": "goexit" } - */ t.Run("notifier.Recover", func(st *testing.T) { func() { defer notifier.Recover() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) t.Run("bugsnag.Recover", func(st *testing.T) { func() { defer bugsnag.Recover() crash("NaN") }() - assertStackframeCount(st, 6) + assertStackframeCount(st, numCrashFrames("NaN")) }) } diff --git a/panicwrap_test.go b/panicwrap_test.go index c156d05f..bb8a8539 100644 --- a/panicwrap_test.go +++ b/panicwrap_test.go @@ -20,6 +20,7 @@ func TestPanicHandlerHandledPanic(t *testing.T) { startPanickingProcess(t, "handled", ts.URL) + // Yeah, we just caught a panic from the init() function below and sent it to the server running above (mindblown) json, err := simplejson.NewJson(<-reports) if err != nil { t.Fatal(err) @@ -42,12 +43,24 @@ func TestPanicHandlerHandledPanic(t *testing.T) { event := getIndex(json, "events", 0) assertValidSession(t, event, true) - // Yeah, we just caught a panic from the init() function below and sent it to the server running above (mindblown) - frame := getIndex(getIndex(event, "exceptions", 0), "stacktrace", 1) - if getBool(frame, "inProject") != true || - getString(frame, "file") != "panicwrap_test.go" || - getInt(frame, "lineNumber") == 0 { - t.Errorf("stack frame seems wrong at index 1: %v", frame) + frames := event.Get("exceptions"). + GetIndex(0). + Get("stacktrace") + + found := false + for i := range frames.MustArray() { + frame := frames.GetIndex(i) + if getString(frame, "file") == "panicwrap_test.go" && + getBool(frame, "inProject") && + getInt(frame, "lineNumber") != 0 { + found = true + break + } + } + if !found { + t.Errorf("stack frames seem wrong: can't find panicwrap_test.go frame") + s, _ := frames.EncodePretty() + t.Log(string(s)) } }