From 53364fce23f144a6b8aa3752c9d0526b46c592e4 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 16 Aug 2022 00:19:14 +0800 Subject: [PATCH] report: print javascript stack on fatal error Try to print JavaScript stack on fatal error. OOMError needs to be distinguished from fatal error since no new handle can be created at that time. PR-URL: https://github.com/nodejs/node/pull/44242 Reviewed-By: Joyee Cheung Reviewed-By: Michael Dawson Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 1 + src/node_errors.cc | 30 +++++++ src/node_errors.h | 1 + src/node_report.cc | 84 +++++++++++++++++-- test/addons/report-fatalerror/binding.cc | 23 +++++ test/addons/report-fatalerror/binding.gyp | 9 ++ test/addons/report-fatalerror/test.js | 52 ++++++++++++ ....js => test-report-fatalerror-oomerror.js} | 4 +- 8 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 test/addons/report-fatalerror/binding.cc create mode 100644 test/addons/report-fatalerror/binding.gyp create mode 100644 test/addons/report-fatalerror/test.js rename test/report/{test-report-fatal-error.js => test-report-fatalerror-oomerror.js} (96%) diff --git a/src/api/environment.cc b/src/api/environment.cc index 5df88a9dbabf78..e772f172e5ebdc 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -237,6 +237,7 @@ void SetIsolateErrorHandlers(v8::Isolate* isolate, const IsolateSettings& s) { auto* fatal_error_cb = s.fatal_error_callback ? s.fatal_error_callback : OnFatalError; isolate->SetFatalErrorHandler(fatal_error_cb); + isolate->SetOOMErrorHandler(OOMErrorHandler); if ((s.flags & SHOULD_NOT_SET_PREPARE_STACK_TRACE_CALLBACK) == 0) { auto* prepare_stack_trace_cb = s.prepare_stack_trace_callback ? diff --git a/src/node_errors.cc b/src/node_errors.cc index 7c017264a88fd1..b464a37e378f11 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -459,6 +459,36 @@ void OnFatalError(const char* location, const char* message) { ABORT(); } +void OOMErrorHandler(const char* location, bool is_heap_oom) { + const char* message = + is_heap_oom ? "Allocation failed - JavaScript heap out of memory" + : "Allocation failed - process out of memory"; + if (location) { + FPrintF(stderr, "FATAL ERROR: %s %s\n", location, message); + } else { + FPrintF(stderr, "FATAL ERROR: %s\n", message); + } + + Isolate* isolate = Isolate::TryGetCurrent(); + Environment* env = nullptr; + if (isolate != nullptr) { + env = Environment::GetCurrent(isolate); + } + bool report_on_fatalerror; + { + Mutex::ScopedLock lock(node::per_process::cli_options_mutex); + report_on_fatalerror = per_process::cli_options->report_on_fatalerror; + } + + if (report_on_fatalerror) { + report::TriggerNodeReport( + isolate, env, message, "OOMError", "", Local()); + } + + fflush(stderr); + ABORT(); +} + v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings( v8::Local context, v8::Local source, diff --git a/src/node_errors.h b/src/node_errors.h index f6a60a1daa4040..7e0d3a3b26ca32 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -21,6 +21,7 @@ void AppendExceptionLine(Environment* env, [[noreturn]] void FatalError(const char* location, const char* message); void OnFatalError(const char* location, const char* message); +void OOMErrorHandler(const char* location, bool is_heap_oom); // Helpers to construct errors similar to the ones provided by // lib/internal/errors.js. diff --git a/src/node_report.cc b/src/node_report.cc index 455d5c4e875060..446b88303d82a9 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -26,6 +26,7 @@ constexpr int NODE_REPORT_VERSION = 2; constexpr int NANOS_PER_SEC = 1000 * 1000 * 1000; constexpr double SEC_PER_MICROS = 1e-6; +constexpr int MAX_FRAME_COUNT = 10; namespace node { namespace report { @@ -43,6 +44,10 @@ using v8::Maybe; using v8::MaybeLocal; using v8::Nothing; using v8::Object; +using v8::RegisterState; +using v8::SampleInfo; +using v8::StackFrame; +using v8::StackTrace; using v8::String; using v8::TryCatch; using v8::V8; @@ -62,6 +67,9 @@ static void PrintJavaScriptErrorStack(JSONWriter* writer, Isolate* isolate, Local error, const char* trigger); +static void PrintJavaScriptStack(JSONWriter* writer, + Isolate* isolate, + const char* trigger); static void PrintJavaScriptErrorProperties(JSONWriter* writer, Isolate* isolate, Local error); @@ -269,8 +277,6 @@ static void WriteNodeReport(Isolate* isolate, // Report summary JavaScript error stack backtrace PrintJavaScriptErrorStack(&writer, isolate, error, trigger); - // Report summary JavaScript error properties backtrace - PrintJavaScriptErrorProperties(&writer, isolate, error); writer.json_objectend(); // the end of 'javascriptStack' // Report V8 Heap and Garbage Collector information @@ -317,7 +323,7 @@ static void WriteNodeReport(Isolate* isolate, env, "Worker thread subreport", trigger, - Local(), + Local(), os); Mutex::ScopedLock lock(workers_mutex); @@ -534,19 +540,80 @@ static Maybe ErrorToString(Isolate* isolate, return Just<>(std::string(*sv, sv.length())); } +static void PrintEmptyJavaScriptStack(JSONWriter* writer) { + writer->json_keyvalue("message", "No stack."); + writer->json_arraystart("stack"); + writer->json_element("Unavailable."); + writer->json_arrayend(); + + writer->json_objectstart("errorProperties"); + writer->json_objectend(); +} + +// Do our best to report the JavaScript stack without calling into JavaScript. +static void PrintJavaScriptStack(JSONWriter* writer, + Isolate* isolate, + const char* trigger) { + // Can not capture the stacktrace when the isolate is in a OOM state. + if (!strcmp(trigger, "OOMError")) { + PrintEmptyJavaScriptStack(writer); + return; + } + + HandleScope scope(isolate); + RegisterState state; + state.pc = nullptr; + state.fp = &state; + state.sp = &state; + + // in-out params + SampleInfo info; + void* samples[MAX_FRAME_COUNT]; + isolate->GetStackSample(state, samples, MAX_FRAME_COUNT, &info); + + Local stack = StackTrace::CurrentStackTrace( + isolate, MAX_FRAME_COUNT, StackTrace::kDetailed); + + if (stack->GetFrameCount() == 0) { + PrintEmptyJavaScriptStack(writer); + return; + } + + writer->json_keyvalue("message", trigger); + writer->json_arraystart("stack"); + for (int i = 0; i < stack->GetFrameCount(); i++) { + Local frame = stack->GetFrame(isolate, i); + + Utf8Value function_name(isolate, frame->GetFunctionName()); + Utf8Value script_name(isolate, frame->GetScriptName()); + const int line_number = frame->GetLineNumber(); + const int column = frame->GetColumn(); + + std::string stack_line = SPrintF( + "at %s (%s:%d:%d)", *function_name, *script_name, line_number, column); + writer->json_element(stack_line); + } + writer->json_arrayend(); + writer->json_objectstart("errorProperties"); + writer->json_objectend(); +} + // Report the JavaScript stack. static void PrintJavaScriptErrorStack(JSONWriter* writer, Isolate* isolate, Local error, const char* trigger) { + if (error.IsEmpty()) { + return PrintJavaScriptStack(writer, isolate, trigger); + } + TryCatch try_catch(isolate); HandleScope scope(isolate); Local context = isolate->GetCurrentContext(); std::string ss = ""; - if ((!strcmp(trigger, "FatalError")) || - (!strcmp(trigger, "Signal")) || - (!ErrorToString(isolate, context, error).To(&ss))) { - ss = "No stack.\nUnavailable.\n"; + if (!ErrorToString(isolate, context, error).To(&ss)) { + PrintEmptyJavaScriptStack(writer); + return; } int line = ss.find('\n'); @@ -569,6 +636,9 @@ static void PrintJavaScriptErrorStack(JSONWriter* writer, } writer->json_arrayend(); } + + // Report summary JavaScript error properties backtrace + PrintJavaScriptErrorProperties(writer, isolate, error); } // Report a native stack backtrace diff --git a/test/addons/report-fatalerror/binding.cc b/test/addons/report-fatalerror/binding.cc new file mode 100644 index 00000000000000..9f01230bb43a54 --- /dev/null +++ b/test/addons/report-fatalerror/binding.cc @@ -0,0 +1,23 @@ +#include +#include + +using v8::FunctionCallbackInfo; +using v8::Isolate; +using v8::Local; +using v8::MaybeLocal; +using v8::Object; +using v8::Value; + +void TriggerFatalError(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + + // Trigger a v8 ApiCheck failure. + MaybeLocal value; + value.ToLocalChecked(); +} + +void init(Local exports) { + NODE_SET_METHOD(exports, "triggerFatalError", TriggerFatalError); +} + +NODE_MODULE(NODE_GYP_MODULE_NAME, init) diff --git a/test/addons/report-fatalerror/binding.gyp b/test/addons/report-fatalerror/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/addons/report-fatalerror/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/addons/report-fatalerror/test.js b/test/addons/report-fatalerror/test.js new file mode 100644 index 00000000000000..96ee7223c0f9ff --- /dev/null +++ b/test/addons/report-fatalerror/test.js @@ -0,0 +1,52 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const path = require('path'); +const spawnSync = require('child_process').spawnSync; +const helper = require('../../common/report.js'); +const tmpdir = require('../../common/tmpdir'); + +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); + +if (process.argv[2] === 'child') { + (function childMain() { + const addon = require(binding); + addon.triggerFatalError(); + })(); + return; +} + +const ARGS = [ + __filename, + 'child', +]; + +{ + // Verify that --report-on-fatalerror is respected when set. + tmpdir.refresh(); + const args = ['--report-on-fatalerror', ...ARGS]; + const child = spawnSync(process.execPath, args, { cwd: tmpdir.path }); + assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly'); + + const reports = helper.findReports(child.pid, tmpdir.path); + assert.strictEqual(reports.length, 1); + + const report = reports[0]; + helper.validate(report); + + const content = require(report); + assert.strictEqual(content.header.trigger, 'FatalError'); + + // Check that the javascript stack is present. + assert.strictEqual(content.javascriptStack.stack.findIndex((frame) => frame.match('childMain')), 0); +} + +{ + // Verify that --report-on-fatalerror is respected when not set. + const args = ARGS; + const child = spawnSync(process.execPath, args, { cwd: tmpdir.path }); + assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly'); + const reports = helper.findReports(child.pid, tmpdir.path); + assert.strictEqual(reports.length, 0); +} diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatalerror-oomerror.js similarity index 96% rename from test/report/test-report-fatal-error.js rename to test/report/test-report-fatalerror-oomerror.js index c913240c4bc7ee..5b918d65e62e54 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatalerror-oomerror.js @@ -44,10 +44,12 @@ const ARGS = [ const report = reports[0]; helper.validate(report); + const content = require(report); // Errors occur in a context where env is not available, so thread ID is // unknown. Assert this, to verify that the underlying env-less situation is // actually reached. - assert.strictEqual(require(report).header.threadId, null); + assert.strictEqual(content.header.threadId, null); + assert.strictEqual(content.header.trigger, 'OOMError'); } {