From fe85a9403590f65bee749c1fd2116f6270ca45cd Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 14 Sep 2020 11:21:23 -0700 Subject: [PATCH] fix: handle electron script errors better (#25331) (#25453) Co-authored-by: Samuel Attard Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> --- build/webpack/run-compiler.js | 19 +++++++++++------ build/webpack/webpack.config.base.js | 4 +++- .../webpack.config.isolated_renderer.js | 3 ++- build/webpack/webpack.config.renderer.js | 3 ++- .../webpack.config.sandboxed_renderer.js | 1 + build/webpack/webpack.config.worker.js | 3 ++- shell/common/node_bindings.cc | 21 ++++++++++++++++--- shell/common/node_util.cc | 12 +++++++++-- 8 files changed, 51 insertions(+), 15 deletions(-) diff --git a/build/webpack/run-compiler.js b/build/webpack/run-compiler.js index 9aa75e7bf5498..5acd9884770f4 100644 --- a/build/webpack/run-compiler.js +++ b/build/webpack/run-compiler.js @@ -10,10 +10,9 @@ config.output = { filename: path.basename(outPath) } -const { wrapInitWithProfilingTimeout } = config; -delete config.wrapInitWithProfilingTimeout; +const { wrapInitWithProfilingTimeout, wrapInitWithTryCatch, ...webpackConfig } = config; -webpack(config, (err, stats) => { +webpack(webpackConfig, (err, stats) => { if (err) { console.error(err) process.exit(1) @@ -21,9 +20,17 @@ webpack(config, (err, stats) => { console.error(stats.toString('normal')) process.exit(1) } else { + let contents = fs.readFileSync(outPath, 'utf8'); + if (wrapInitWithTryCatch) { + contents = `try { +${contents} +} catch (err) { + console.error('Electron ${webpackConfig.output.filename} script failed to run'); + console.error(err); +}`; + } if (wrapInitWithProfilingTimeout) { - const contents = fs.readFileSync(outPath, 'utf8'); - const newContents = `function ___electron_webpack_init__() { + contents = `function ___electron_webpack_init__() { ${contents} }; if ((globalThis.process || binding.process).argv.includes("--profile-electron-init")) { @@ -31,8 +38,8 @@ if ((globalThis.process || binding.process).argv.includes("--profile-electron-in } else { ___electron_webpack_init__(); }`; - fs.writeFileSync(outPath, newContents); } + fs.writeFileSync(outPath, contents) process.exit(0) } }) diff --git a/build/webpack/webpack.config.base.js b/build/webpack/webpack.config.base.js index 45ee5fdd454f1..464960d1abf0c 100644 --- a/build/webpack/webpack.config.base.js +++ b/build/webpack/webpack.config.base.js @@ -86,7 +86,8 @@ module.exports = ({ loadElectronFromAlternateTarget, targetDeletesNodeGlobals, target, - wrapInitWithProfilingTimeout + wrapInitWithProfilingTimeout, + wrapInitWithTryCatch }) => { let entry = path.resolve(electronRoot, 'lib', target, 'init.ts') if (!fs.existsSync(entry)) { @@ -102,6 +103,7 @@ module.exports = ({ filename: `${target}.bundle.js` }, wrapInitWithProfilingTimeout, + wrapInitWithTryCatch, resolve: { alias: { '@electron/internal': path.resolve(electronRoot, 'lib'), diff --git a/build/webpack/webpack.config.isolated_renderer.js b/build/webpack/webpack.config.isolated_renderer.js index 28b9e940fa971..95a553d899547 100644 --- a/build/webpack/webpack.config.isolated_renderer.js +++ b/build/webpack/webpack.config.isolated_renderer.js @@ -1,4 +1,5 @@ module.exports = require('./webpack.config.base')({ target: 'isolated_renderer', - alwaysHasNode: false + alwaysHasNode: false, + wrapInitWithTryCatch: true }) diff --git a/build/webpack/webpack.config.renderer.js b/build/webpack/webpack.config.renderer.js index 8f890ee6e0d76..594aaa7725d88 100644 --- a/build/webpack/webpack.config.renderer.js +++ b/build/webpack/webpack.config.renderer.js @@ -2,5 +2,6 @@ module.exports = require('./webpack.config.base')({ target: 'renderer', alwaysHasNode: true, targetDeletesNodeGlobals: true, - wrapInitWithProfilingTimeout: true + wrapInitWithProfilingTimeout: true, + wrapInitWithTryCatch: true }) diff --git a/build/webpack/webpack.config.sandboxed_renderer.js b/build/webpack/webpack.config.sandboxed_renderer.js index 9c38d6acb99bf..7a96344978ab3 100644 --- a/build/webpack/webpack.config.sandboxed_renderer.js +++ b/build/webpack/webpack.config.sandboxed_renderer.js @@ -2,4 +2,5 @@ module.exports = require('./webpack.config.base')({ target: 'sandboxed_renderer', alwaysHasNode: false, wrapInitWithProfilingTimeout: true, + wrapInitWithTryCatch: true }) diff --git a/build/webpack/webpack.config.worker.js b/build/webpack/webpack.config.worker.js index 7fc167b54f281..007c82dee27b5 100644 --- a/build/webpack/webpack.config.worker.js +++ b/build/webpack/webpack.config.worker.js @@ -2,5 +2,6 @@ module.exports = require('./webpack.config.base')({ target: 'worker', loadElectronFromAlternateTarget: 'renderer', alwaysHasNode: true, - targetDeletesNodeGlobals: true + targetDeletesNodeGlobals: true, + wrapInitWithTryCatch: true }) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index a5ee59e2666f2..84b4a914632f2 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -342,9 +342,24 @@ node::Environment* NodeBindings::CreateEnvironment( std::unique_ptr c_argv = StringVectorToArgArray(args); isolate_data_ = node::CreateIsolateData(context->GetIsolate(), uv_loop_, platform); - node::Environment* env = node::CreateEnvironment( - isolate_data_, context, args.size(), c_argv.get(), 0, nullptr); - DCHECK(env); + + node::Environment* env; + if (browser_env_ != BrowserEnvironment::BROWSER) { + v8::TryCatch try_catch(context->GetIsolate()); + env = node::CreateEnvironment(isolate_data_, context, args.size(), + c_argv.get(), 0, nullptr); + DCHECK(env); + // This will only be caught when something has gone terrible wrong as all + // electron scripts are wrapped in a try {} catch {} in run-compiler.js + if (try_catch.HasCaught()) { + LOG(ERROR) << "Failed to initialize node environment in process: " + << process_type; + } + } else { + env = node::CreateEnvironment(isolate_data_, context, args.size(), + c_argv.get(), 0, nullptr); + DCHECK(env); + } // Clean up the global _noBrowserGlobals that we unironically injected into // the global scope diff --git a/shell/common/node_util.cc b/shell/common/node_util.cc index 777732b81bac9..ab4207d5909d5 100644 --- a/shell/common/node_util.cc +++ b/shell/common/node_util.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "shell/common/node_util.h" +#include "base/logging.h" #include "shell/common/node_includes.h" #include "third_party/electron_node/src/node_native_module_env.h" @@ -17,6 +18,7 @@ v8::MaybeLocal CompileAndCall( std::vector>* arguments, node::Environment* optional_env) { v8::Isolate* isolate = context->GetIsolate(); + v8::TryCatch try_catch(isolate); v8::MaybeLocal compiled = node::native_module::NativeModuleEnv::LookupAndCompile( context, id, parameters, optional_env); @@ -24,8 +26,14 @@ v8::MaybeLocal CompileAndCall( return v8::MaybeLocal(); } v8::Local fn = compiled.ToLocalChecked().As(); - return fn->Call(context, v8::Null(isolate), arguments->size(), - arguments->data()); + v8::MaybeLocal ret = fn->Call( + context, v8::Null(isolate), arguments->size(), arguments->data()); + // This will only be caught when something has gone terrible wrong as all + // electron scripts are wrapped in a try {} catch {} in run-compiler.js + if (try_catch.HasCaught()) { + LOG(ERROR) << "Failed to CompileAndCall electron script: " << id; + } + return ret; } } // namespace util