From f02538cd41a964fcd3e14e356d5bf1993ee5f61f Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 6 Nov 2020 00:27:36 -0800 Subject: [PATCH 1/4] fix: renderer crash on setImmediate --- shell/common/node_bindings.cc | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index aaceb584da88c..15154967591c7 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -170,6 +170,20 @@ bool AllowWasmCodeGenerationCallback(v8::Local context, v8::String::Empty(isolate)); } +void ErrorMessageListener(v8::Local message, + v8::Local data) { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + node::Environment* env = node::Environment::GetCurrent(isolate); + + // TODO(codebytere): properly emit the after() hooks now + // that the exception has been handled. + // See node/lib/internal/process/execution.js#L176-L180 + + // Ensure that the async id stack is properly cleared so the async + // hook stack does not becomes corrupted. + env->async_hooks()->clear_async_id_stack(); +} + // Initialize Node.js cli options to pass to Node.js // See https://nodejs.org/api/cli.html#cli_options void SetNodeCliFlags() { @@ -470,6 +484,12 @@ node::Environment* NodeBindings::CreateEnvironment( // Blink's. is.flags &= ~node::IsolateSettingsFlags::MESSAGE_LISTENER_WITH_ERROR_LEVEL; + // Isolate message listeners are additive (you can add multiple), so instead + // we add an extra one here to ensure that the async hook stack is properly + // cleared when errors are thrown. + context->GetIsolate()->AddMessageListenerWithErrorLevel( + ErrorMessageListener, v8::Isolate::kMessageError); + // We do not want to use the promise rejection callback that Node.js uses, // because it does not send PromiseRejectionEvents to the global script // context. We need to use the one Blink already provides. From f5e275cb6b64b0669611552a0b8db24c6949a9b2 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sun, 8 Nov 2020 19:42:09 -0800 Subject: [PATCH 2/4] spec: add a crash case --- .../setimmediate-renderer-crash/index.js | 22 +++++++++++++++++++ .../setimmediate-renderer-crash/preload.js | 3 +++ 2 files changed, 25 insertions(+) create mode 100644 spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js create mode 100644 spec-main/fixtures/crash-cases/setimmediate-renderer-crash/preload.js diff --git a/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js new file mode 100644 index 0000000000000..c9f41e71ceb90 --- /dev/null +++ b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js @@ -0,0 +1,22 @@ +const { app, BrowserWindow } = require('electron'); +const path = require('path'); + +app.whenReady().then(() => { + const win = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: true, + preload: path.resolve(__dirname, 'preload.js') + } + }); + + win.loadURL('data:text/html,'); + + win.webContents.on('render-process-gone', () => { + process.exit(1); + }); + + win.webContents.on('did-finish-load', () => { + setTimeout(() => app.quit()); + }); +}); diff --git a/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/preload.js b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/preload.js new file mode 100644 index 0000000000000..8e8afb06af723 --- /dev/null +++ b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/preload.js @@ -0,0 +1,3 @@ +setImmediate(() => { + throw new Error('oh no'); +}); From 99fd265642e372e438db30fb6770237f53dcafa3 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 9 Nov 2020 09:48:42 -0800 Subject: [PATCH 3/4] fix: ensure env exists --- shell/common/node_bindings.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 15154967591c7..6c21378a8aeb8 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -180,8 +180,11 @@ void ErrorMessageListener(v8::Local message, // See node/lib/internal/process/execution.js#L176-L180 // Ensure that the async id stack is properly cleared so the async - // hook stack does not becomes corrupted. - env->async_hooks()->clear_async_id_stack(); + // hook stack does not become corrupted. + + if (env) { + env->async_hooks()->clear_async_id_stack(); + } } // Initialize Node.js cli options to pass to Node.js From 735ab9eccfbba528c4eaa6f6a0173f631176ad79 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 9 Nov 2020 13:25:02 -0800 Subject: [PATCH 4/4] Update spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js Co-authored-by: Jeremy Rose --- .../fixtures/crash-cases/setimmediate-renderer-crash/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js index c9f41e71ceb90..7960616214020 100644 --- a/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js +++ b/spec-main/fixtures/crash-cases/setimmediate-renderer-crash/index.js @@ -10,7 +10,7 @@ app.whenReady().then(() => { } }); - win.loadURL('data:text/html,'); + win.loadURL('about:blank'); win.webContents.on('render-process-gone', () => { process.exit(1);