fix: crash in utilityProcess when generating code from strings (#38156)

fix: crash in utilityProcess when generating code from strings (#38040)

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>

shell/app/electron_main_delegate.cc

@@ -40,6 +40,7 @@
 #include "shell/common/logging.h"
 #include "shell/common/options_switches.h"
 #include "shell/common/platform_util.h"
+#include "shell/common/process_util.h"
 #include "shell/renderer/electron_renderer_client.h"
 #include "shell/renderer/electron_sandboxed_renderer_client.h"
 #include "shell/utility/electron_content_utility_client.h"
@@ -82,11 +83,6 @@ constexpr base::StringPiece kElectronDisableSandbox("ELECTRON_DISABLE_SANDBOX");
 constexpr base::StringPiece kElectronEnableStackDumping(
     "ELECTRON_ENABLE_STACK_DUMPING");
 
-bool IsBrowserProcess(base::CommandLine* cmd) {
-  std::string process_type = cmd->GetSwitchValueASCII(::switches::kProcessType);
-  return process_type.empty();
-}
-
 // Returns true if this subprocess type needs the ResourceBundle initialized
 // and resources loaded.
 bool SubprocessNeedsResourceBundle(const std::string& process_type) {
@@ -249,14 +245,12 @@ absl::optional<int> ElectronMainDelegate::BasicStartupComplete() {
 
   // On Windows the terminal returns immediately, so we add a new line to
   // prevent output in the same line as the prompt.
-  if (IsBrowserProcess(command_line))
+  if (IsBrowserProcess())
     std::wcout << std::endl;
 #endif  // !BUILDFLAG(IS_WIN)
 
   auto env = base::Environment::Create();
 
-  gin_helper::Locker::SetIsBrowserProcess(IsBrowserProcess(command_line));
-
   // Enable convenient stack printing. This is enabled by default in
   // non-official builds.
   if (env->HasVar(kElectronEnableStackDumping))
@@ -289,7 +283,7 @@ absl::optional<int> ElectronMainDelegate::BasicStartupComplete() {
   // bugs, but no use in Electron.
   base::win::DisableHandleVerifier();
 
-  if (IsBrowserProcess(command_line))
+  if (IsBrowserProcess())
     base::win::PinUser32();
 #endif
 
@@ -385,7 +379,7 @@ void ElectronMainDelegate::PreSandboxStartup() {
   crash_keys::SetPlatformCrashKey();
 #endif
 
-  if (IsBrowserProcess(command_line)) {
+  if (IsBrowserProcess()) {
     // Only append arguments for browser process.
 
     // Allow file:// URIs to read other file:// URIs by default.

shell/common/api/electron_bindings.cc

@@ -23,11 +23,11 @@
 #include "shell/common/application_info.h"
 #include "shell/common/gin_converters/file_path_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/heap_snapshot.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/process_util.h"
 #include "third_party/blink/renderer/platform/heap/process_heap.h"  // nogncheck
 
 namespace electron {
@@ -50,7 +50,7 @@ void ElectronBindings::BindProcess(v8::Isolate* isolate,
   process->SetMethod("getCreationTime", &GetCreationTime);
   process->SetMethod("getHeapStatistics", &GetHeapStatistics);
   process->SetMethod("getBlinkMemoryInfo", &GetBlinkMemoryInfo);
-  if (gin_helper::Locker::IsBrowserProcess()) {
+  if (electron::IsBrowserProcess()) {
     process->SetMethod("getProcessMemoryInfo", &GetProcessMemoryInfo);
   }
   process->SetMethod("getSystemMemoryInfo", &GetSystemMemoryInfo);
@@ -209,7 +209,7 @@ v8::Local<v8::Value> ElectronBindings::GetSystemMemoryInfo(
 // static
 v8::Local<v8::Promise> ElectronBindings::GetProcessMemoryInfo(
     v8::Isolate* isolate) {
-  CHECK(gin_helper::Locker::IsBrowserProcess());
+  CHECK(electron::IsBrowserProcess());
   gin_helper::Promise<gin_helper::Dictionary> promise(isolate);
   v8::Local<v8::Promise> handle = promise.GetHandle();
 

shell/common/gin_helper/callback.cc

@@ -7,6 +7,7 @@
 #include "base/cxx17_backports.h"
 #include "content/public/browser/browser_thread.h"
 #include "gin/dictionary.h"
+#include "shell/common/process_util.h"
 
 namespace gin_helper {
 
@@ -70,7 +71,7 @@ void CallTranslater(v8::Local<v8::External> external,
 struct DeleteOnUIThread {
   template <typename T>
   static void Destruct(const T* x) {
-    if (gin_helper::Locker::IsBrowserProcess() &&
+    if (electron::IsBrowserProcess() &&
         !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
       content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE,
                                          x);

shell/common/gin_helper/locker.cc

@@ -4,19 +4,15 @@
 
 #include "shell/common/gin_helper/locker.h"
 
+#include "shell/common/process_util.h"
+
 namespace gin_helper {
 
 Locker::Locker(v8::Isolate* isolate) {
-  if (IsBrowserProcess())
+  if (electron::IsBrowserProcess())
     locker_ = std::make_unique<v8::Locker>(isolate);
 }
 
 Locker::~Locker() = default;
 
-void Locker::SetIsBrowserProcess(bool is_browser_process) {
-  g_is_browser_process = is_browser_process;
-}
-
-bool Locker::g_is_browser_process = false;
-
 }  // namespace gin_helper

shell/common/gin_helper/locker.h

@@ -21,12 +21,6 @@ class Locker {
   Locker(const Locker&) = delete;
   Locker& operator=(const Locker&) = delete;
 
-  // Returns whether current process is browser process, currently we detect it
-  // by checking whether current has used V8 Lock, but it might be a bad idea.
-  static inline bool IsBrowserProcess() { return g_is_browser_process; }
-
-  static void SetIsBrowserProcess(bool is_browser_process);
-
  private:
   void* operator new(size_t size);
   void operator delete(void*, size_t);

shell/common/gin_helper/microtasks_scope.cc

@@ -4,14 +4,14 @@
 
 #include "shell/common/gin_helper/microtasks_scope.h"
 
-#include "shell/common/gin_helper/locker.h"
+#include "shell/common/process_util.h"
 
 namespace gin_helper {
 
 MicrotasksScope::MicrotasksScope(v8::Isolate* isolate,
                                  bool ignore_browser_checkpoint,
                                  v8::MicrotasksScope::Type scope_type) {
-  if (Locker::IsBrowserProcess()) {
+  if (electron::IsBrowserProcess()) {
     if (!ignore_browser_checkpoint)
       v8::MicrotasksScope::PerformCheckpoint(isolate);
   } else {

shell/common/gin_helper/promise.cc

@@ -63,7 +63,7 @@ v8::Local<v8::Promise::Resolver> PromiseBase::GetInner() const {
 
 // static
 void Promise<void>::ResolvePromise(Promise<void> promise) {
-  if (gin_helper::Locker::IsBrowserProcess() &&
+  if (electron::IsBrowserProcess() &&
       !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
     content::GetUIThreadTaskRunner({})->PostTask(
         FROM_HERE,

shell/common/gin_helper/promise.h

@@ -16,6 +16,7 @@
 #include "shell/common/gin_converters/std_converter.h"
 #include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/microtasks_scope.h"
+#include "shell/common/process_util.h"
 
 namespace gin_helper {
 
@@ -45,7 +46,7 @@ class PromiseBase {
   // Note: The parameter type is PromiseBase&& so it can take the instances of
   // Promise<T> type.
   static void RejectPromise(PromiseBase&& promise, base::StringPiece errmsg) {
-    if (gin_helper::Locker::IsBrowserProcess() &&
+    if (electron::IsBrowserProcess() &&
         !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
       content::GetUIThreadTaskRunner({})->PostTask(
           FROM_HERE,
@@ -88,7 +89,7 @@ class Promise : public PromiseBase {
 
   // Helper for resolving the promise with |result|.
   static void ResolvePromise(Promise<RT> promise, RT result) {
-    if (gin_helper::Locker::IsBrowserProcess() &&
+    if (electron::IsBrowserProcess() &&
         !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
       content::GetUIThreadTaskRunner({})->PostTask(
           FROM_HERE, base::BindOnce([](Promise<RT> promise,

shell/common/gin_helper/trackable_object.cc

@@ -9,7 +9,7 @@
 #include "base/bind.h"
 #include "base/supports_user_data.h"
 #include "shell/browser/electron_browser_main_parts.h"
-#include "shell/common/gin_helper/locker.h"
+#include "shell/common/process_util.h"
 
 namespace gin_helper {
 
@@ -31,7 +31,7 @@ class IDUserData : public base::SupportsUserData::Data {
 
 TrackableObjectBase::TrackableObjectBase() {
   // TODO(zcbenz): Make TrackedObject work in renderer process.
-  DCHECK(gin_helper::Locker::IsBrowserProcess())
+  DCHECK(electron::IsBrowserProcess())
       << "This class only works for browser process";
 }
 

shell/common/node_bindings.cc

@@ -171,7 +171,7 @@ bool AllowWasmCodeGenerationCallback(v8::Local<v8::Context> context,
   // If we're running with contextIsolation enabled in the renderer process,
   // fall back to Blink's logic.
   if (node::Environment::GetCurrent(context) == nullptr) {
-    if (gin_helper::Locker::IsBrowserProcess())
+    if (!electron::IsRendererProcess())
       return false;
     return blink::V8Initializer::WasmCodeGenerationCheckCallbackInMainThread(
         context, source);
@@ -188,7 +188,7 @@ v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
     // No node environment means we're in the renderer process, either in a
     // sandboxed renderer or in an unsandboxed renderer with context isolation
     // enabled.
-    if (gin_helper::Locker::IsBrowserProcess()) {
+    if (!electron::IsRendererProcess()) {
       NOTREACHED();
       return {false, {}};
     }
@@ -197,21 +197,20 @@ v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
   }
 
   // If we get here then we have a node environment, so either a) we're in the
-  // main process, or b) we're in the renderer process in a context that has
-  // both node and blink, i.e. contextIsolation disabled.
-
-  // If we're in the main process, delegate to node.
-  if (gin_helper::Locker::IsBrowserProcess()) {
-    return node::ModifyCodeGenerationFromStrings(context, source, is_code_like);
-  }
+  // non-rendrer process, or b) we're in the renderer process in a context that
+  // has both node and blink, i.e. contextIsolation disabled.
 
   // If we're in the renderer with contextIsolation disabled, ask blink first
   // (for CSP), and iff that allows codegen, delegate to node.
-  v8::ModifyCodeGenerationFromStringsResult result =
-      blink::V8Initializer::CodeGenerationCheckCallbackInMainThread(
-          context, source, is_code_like);
-  if (!result.codegen_allowed)
-    return result;
+  if (electron::IsRendererProcess()) {
+    v8::ModifyCodeGenerationFromStringsResult result =
+        blink::V8Initializer::CodeGenerationCheckCallbackInMainThread(
+            context, source, is_code_like);
+    if (!result.codegen_allowed)
+      return result;
+  }
+
+  // If we're in the main process or utility process, delegate to node.
   return node::ModifyCodeGenerationFromStrings(context, source, is_code_like);
 }
 

shell/common/process_util.cc

@@ -4,6 +4,8 @@
 
 #include "shell/common/process_util.h"
 
+#include "base/command_line.h"
+#include "content/public/common/content_switches.h"
 #include "gin/dictionary.h"
 #include "shell/common/gin_converters/callback_converter.h"
 #include "shell/common/node_includes.h"
@@ -24,4 +26,18 @@ void EmitWarning(node::Environment* env,
   emit_warning.Run(warning_msg, warning_type, "");
 }
 
+bool IsBrowserProcess() {
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+  std::string process_type =
+      command_line->GetSwitchValueASCII(switches::kProcessType);
+  return process_type.empty();
+}
+
+bool IsRendererProcess() {
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+  std::string process_type =
+      command_line->GetSwitchValueASCII(switches::kProcessType);
+  return process_type == switches::kRendererProcess;
+}
+
 }  // namespace electron

shell/common/process_util.h

@@ -17,6 +17,9 @@ void EmitWarning(node::Environment* env,
                  const std::string& warning_msg,
                  const std::string& warning_type);
 
+bool IsBrowserProcess();
+bool IsRendererProcess();
+
 }  // namespace electron
 
 #endif  // ELECTRON_SHELL_COMMON_PROCESS_UTIL_H_

spec/api-utility-process-spec.ts

@@ -360,5 +360,19 @@ describe('utilityProcess module', () => {
       await emittedOnce(child, 'exit');
       expect(log).to.equal('hello\n');
     });
+
+    it('does not crash when running eval', async () => {
+      const child = utilityProcess.fork('./eval.js', [], {
+        cwd: fixturesPath,
+        stdio: 'ignore'
+      });
+      await emittedOnce(child, 'spawn');
+      const [data] = await emittedOnce(child, 'message');
+      expect(data).to.equal(42);
+      // Cleanup.
+      const exit = emittedOnce(child, 'exit');
+      expect(child.kill()).to.be.true();
+      await exit;
+    });
   });
 });

spec/fixtures/api/utility-process/eval.js

@@ -0,0 +1,6 @@
+const vm = require('node:vm');
+
+const contextObject = { result: 0 };
+vm.createContext(contextObject);
+vm.runInContext('eval(\'result = 42\')', contextObject);
+process.parentPort.postMessage(contextObject.result);