From 0840aab1e0c549755e85aaa187278ddea8a294a9 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 19 Jun 2020 23:19:25 +0200
Subject: [PATCH] src: reduce scope of code cache mutex

As indicated in the added comment, this can lead to a deadlock
otherwise. In the concrete instance in which I encountered this,
the relevant nested call is the one to `require('internal/tty')`
inside of the `afterInspector()` function for uncaught exception
handling.
---
 src/node_native_module.cc | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/node_native_module.cc b/src/node_native_module.cc
index 8791c99a0657fd..17abd057eaa06d 100644
--- a/src/node_native_module.cc
+++ b/src/node_native_module.cc
@@ -263,10 +263,13 @@ MaybeLocal<Function> NativeModuleLoader::LookupAndCompile(
   Local<Integer> column_offset = Integer::New(isolate, 0);
   ScriptOrigin origin(filename, line_offset, column_offset, True(isolate));
 
-  Mutex::ScopedLock lock(code_cache_mutex_);
-
   ScriptCompiler::CachedData* cached_data = nullptr;
   {
+    // Note: The lock here should not extend into the
+    // `CompileFunctionInContext()` call below, because this function may
+    // recurse if there is a syntax error during bootstrap (because the fatal
+    // exception handler is invoked, which may load built-in modules).
+    Mutex::ScopedLock lock(code_cache_mutex_);
     auto cache_it = code_cache_.find(id);
     if (cache_it != code_cache_.end()) {
       // Transfer ownership to ScriptCompiler::Source later.
@@ -313,8 +316,13 @@ MaybeLocal<Function> NativeModuleLoader::LookupAndCompile(
       ScriptCompiler::CreateCodeCacheForFunction(fun));
   CHECK_NOT_NULL(new_cached_data);
 
-  // The old entry should've been erased by now so we can just emplace
-  code_cache_.emplace(id, std::move(new_cached_data));
+  {
+    Mutex::ScopedLock lock(code_cache_mutex_);
+    // The old entry should've been erased by now so we can just emplace.
+    // If another thread did the same thing in the meantime, that should not
+    // be an issue.
+    code_cache_.emplace(id, std::move(new_cached_data));
+  }
 
   return scope.Escape(fun);
 }