Skip to content

Commit

Permalink
src: fix fatal errors when a current isolate not exist
Browse files Browse the repository at this point in the history
napi_fatal_error and node watchdog trigger fatal error but rather
running on a thread that hold no current isolate.

PR-URL: #38624
Reviewed-By: Michael Dawson <[email protected]>
  • Loading branch information
legendecas authored and targos committed May 18, 2021
1 parent a0dc194 commit 718ad10
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,12 @@ void OnFatalError(const char* location, const char* message) {
}

Isolate* isolate = Isolate::GetCurrent();
// TODO(legendecas): investigate failures on triggering node-report with
// nullptr isolates.
if (isolate == nullptr) {
fflush(stderr);
ABORT();
}
Environment* env = Environment::GetCurrent(isolate);
bool report_on_fatalerror;
{
Expand Down
1 change: 1 addition & 0 deletions test/node-api/test_fatal/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ const p = child_process.spawnSync(
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: test_fatal::Test fatal message'));
assert.ok(p.status === 134 || p.signal === 'SIGABRT');
1 change: 1 addition & 0 deletions test/node-api/test_fatal/test2.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ const p = child_process.spawnSync(
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: test_fatal::Test fatal message'));
assert.ok(p.status === 134 || p.signal === 'SIGABRT');
21 changes: 21 additions & 0 deletions test/node-api/test_fatal/test_fatal.c
Original file line number Diff line number Diff line change
@@ -1,12 +1,32 @@
// For the purpose of this test we use libuv's threading library. When deciding
// on a threading library for a new project it bears remembering that in the
// future libuv may introduce API changes which may render it non-ABI-stable,
// which, in turn, may affect the ABI stability of the project despite its use
// of N-API.
#include <uv.h>
#include <node_api.h>
#include "../../js-native-api/common.h"

static uv_thread_t uv_thread;

static void work_thread(void* data) {
napi_fatal_error("work_thread", NAPI_AUTO_LENGTH,
"foobar", NAPI_AUTO_LENGTH);
}

static napi_value Test(napi_env env, napi_callback_info info) {
napi_fatal_error("test_fatal::Test", NAPI_AUTO_LENGTH,
"fatal message", NAPI_AUTO_LENGTH);
return NULL;
}

static napi_value TestThread(napi_env env, napi_callback_info info) {
NODE_API_ASSERT(env,
(uv_thread_create(&uv_thread, work_thread, NULL) == 0),
"Thread creation");
return NULL;
}

static napi_value TestStringLength(napi_env env, napi_callback_info info) {
napi_fatal_error("test_fatal::TestStringLength", 16, "fatal message", 13);
return NULL;
Expand All @@ -16,6 +36,7 @@ static napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor properties[] = {
DECLARE_NODE_API_PROPERTY("Test", Test),
DECLARE_NODE_API_PROPERTY("TestStringLength", TestStringLength),
DECLARE_NODE_API_PROPERTY("TestThread", TestThread),
};

NODE_API_CALL(env, napi_define_properties(
Expand Down
20 changes: 20 additions & 0 deletions test/node-api/test_fatal/test_threads.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';
const common = require('../../common');
const assert = require('assert');
const child_process = require('child_process');
const test_fatal = require(`./build/${common.buildType}/test_fatal`);

// Test in a child process because the test code will trigger a fatal error
// that crashes the process.
if (process.argv[2] === 'child') {
test_fatal.TestThread();
// Busy loop to allow the work thread to abort.
while (true) {}
}

const p = child_process.spawnSync(
process.execPath, [ __filename, 'child' ]);
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: work_thread foobar'));
assert.ok(p.status === 134 || p.signal === 'SIGABRT');

0 comments on commit 718ad10

Please sign in to comment.