inspector: avoid more JS calls from V8 interrupts

src/base_object_types.h

@@ -26,7 +26,8 @@ namespace node {
   V(http2_binding_data, http2::BindingData)                                    \
   V(http_parser_binding_data, http_parser::BindingData)                        \
   V(quic_binding_data, quic::BindingData)                                      \
-  V(sqlite_binding_data, sqlite::BindingData)
+  V(sqlite_binding_data, sqlite::BindingData)                                  \
+  V(webstorage_binding_data, webstorage::BindingData)
 
 // List of (non-binding) BaseObjects that are serializable in the snapshot.
 // The first argument should match what the type passes to

src/inspector/dom_storage_agent.cc

@@ -1,7 +1,10 @@
 #include "dom_storage_agent.h"
 #include <optional>
+#include "base_object-inl.h"
 #include "env-inl.h"
 #include "inspector/inspector_object_utils.h"
+#include "node_realm-inl.h"
+#include "path.h"
 #include "util.h"
 #include "v8-exception.h"
 #include "v8-isolate.h"
@@ -100,38 +103,35 @@ protocol::DispatchResponse DOMStorageAgent::getDOMStorageItems(
       is_local_storage ? &local_storage_map_ : &session_storage_map_;
   std::optional<StorageMap> storage_map_fallback;
   if (storage_map->empty()) {
-    auto web_storage_obj = getWebStorage(is_local_storage);
+    bool is_uninitialized = false;
+    auto web_storage_obj = getWebStorage(is_local_storage, &is_uninitialized);
     // Each way of failing below says something different about the store, so
     // each reports a different reason. A frontend that cannot read a store
     // otherwise has no way to tell a missing one from a corrupt one.
-    if (!web_storage_obj) {
+    if (!web_storage_obj && !is_uninitialized) {
       return protocol::DispatchResponse::ServerError(
           "Could not read DOM storage items: storage is unavailable");
     }
-    // A message from a remote frontend is dispatched without a HandleScope
-    // on the stack, and opening the backing file can throw, so give the
-    // exception a scope to be allocated in and somewhere to land.
-    v8::HandleScope handle_scope(env_->isolate());
-    v8::TryCatch try_catch(env_->isolate());
-    storage_map_fallback = web_storage_obj.value()->GetAll();
-    if (try_catch.HasCaught()) {
-      // Pass the reason along; "the file was written by a newer Node.js" and
-      // "the file is locked" are not the same problem to the user. Read it
-      // off the Message, which was built when the exception was thrown.
-      // Converting the exception itself would call a user-patchable
-      // Error.prototype.toString, and there is no JavaScript frame here to
-      // run it from.
-      Local<v8::Message> message = try_catch.Message();
-      if (!message.IsEmpty()) {
-        Utf8Value reason(env_->isolate(), message->Get());
-        return protocol::DispatchResponse::ServerError(
-            std::string("Could not read DOM storage items: ") + reason.out());
+    std::string open_error;
+    if (web_storage_obj) {
+      storage_map_fallback = web_storage_obj.value()->GetAll(&open_error);
+    } else if (is_local_storage) {
+      std::string location = env_->options()->localstorage_file;
+      if (location != node::webstorage::kInMemoryPath) {
+        location = ToNamespacedPath(env_, location);
       }
-      // V8 builds that Message on a best-effort basis, so the throw is all we
-      // can report when it is missing.
+      storage_map_fallback =
+          node::webstorage::Storage::ReadAllFromFile(location, &open_error);
+    } else {
+      storage_map_fallback.emplace();
+    }
+    if (!open_error.empty()) {
+      // Pass the reason along; "the file was written by a newer Node.js" and
+      // "the file is locked" are not the same problem to the user. Prefix with
+      // "Uncaught Error: " to match the v8::Message format without invoking
+      // JavaScript during a V8 interrupt.
       return protocol::DispatchResponse::ServerError(
-          "Could not read DOM storage items: the backing store could not be "
-          "opened");
+          "Could not read DOM storage items: Uncaught Error: " + open_error);
     }
     if (!storage_map_fallback.has_value()) {
       return protocol::DispatchResponse::ServerError(
@@ -318,26 +318,60 @@ void DOMStorageAgent::registerStorage(Local<Context> context,
 }
 
 std::optional<node::webstorage::Storage*> DOMStorageAgent::getWebStorage(
-    bool is_local_storage) {
+    bool is_local_storage, bool* is_uninitialized) {
+  *is_uninitialized = false;
+  if (!env_->options()->webstorage) {
+    return std::nullopt;
+  }
+  if (is_local_storage && env_->options()->localstorage_file.empty()) {
+    return std::nullopt;
+  }
+  if (auto* binding = env_->principal_realm()
+                          ->GetBindingData<node::webstorage::BindingData>()) {
+    const auto& storage_ptr =
+        is_local_storage ? binding->local_storage : binding->session_storage;
+    if (storage_ptr) {
+      return storage_ptr.get();
+    }
+  }
+  if (env_->no_browser_globals()) {
+    return std::nullopt;
+  }
   v8::Isolate* isolate = env_->isolate();
   v8::HandleScope handle_scope(isolate);
-  v8::Local<v8::Object> global = env_->context()->Global();
+  v8::Local<v8::Context> context = env_->context();
+  v8::Local<v8::Object> global = context->Global();
+  v8::Local<v8::String> prop_name =
+      is_local_storage ? FIXED_ONE_BYTE_STRING(isolate, "localStorage")
+                       : FIXED_ONE_BYTE_STRING(isolate, "sessionStorage");
+  // Protocol messages from a remote frontend are dispatched from a V8 API
+  // interrupt, where executing JavaScript is disallowed. Both globals are
+  // defined as lazy JS getters, so check whether the property is still an
+  // accessor before reading it.
+  bool is_accessor = false;
+  if (!global->HasRealNamedCallbackProperty(context, prop_name)
+           .To(&is_accessor)) {
+    return std::nullopt;
+  }
+  if (is_accessor) {
+    if (env_->is_processing_v8_interrupt()) {
+      *is_uninitialized = true;
+      return std::nullopt;
+    }
+    if (!env_->can_call_into_js()) {
+      return std::nullopt;
+    }
+  }
   v8::Local<v8::Value> web_storage_val;
   v8::TryCatch try_catch(isolate);
-  if (!global
-           ->Get(env_->context(),
-                 is_local_storage
-                     ? FIXED_ONE_BYTE_STRING(isolate, "localStorage")
-                     : FIXED_ONE_BYTE_STRING(isolate, "sessionStorage"))
-           .ToLocal(&web_storage_val) ||
+  if (!global->Get(context, prop_name).ToLocal(&web_storage_val) ||
       !web_storage_val->IsObject() || try_catch.HasCaught()) {
     return std::nullopt;
-  } else {
-    node::webstorage::Storage* storage;
-    ASSIGN_OR_RETURN_UNWRAP(
-        &storage, web_storage_val.As<v8::Object>(), std::nullopt);
-    return storage;
   }
+  node::webstorage::Storage* storage;
+  ASSIGN_OR_RETURN_UNWRAP(
+      &storage, web_storage_val.As<v8::Object>(), std::nullopt);
+  return storage;
 }
 
 bool DOMStorageAgent::canEmit(const std::string& domain) {

src/inspector/dom_storage_agent.h

@@ -54,7 +54,7 @@ class DOMStorageAgent : public protocol::DOMStorage::Backend,
  private:
   typedef std::unordered_map<std::u16string, std::u16string> StorageMap;
   std::optional<node::webstorage::Storage*> getWebStorage(
-      bool is_local_storage);
+      bool is_local_storage, bool* is_uninitialized);
   std::unique_ptr<protocol::DOMStorage::Frontend> frontend_;
   StorageMap local_storage_map_ = {};
   StorageMap session_storage_map_ = {};

src/inspector_agent.cc

@@ -80,6 +80,52 @@ static Mutex start_io_thread_agents_mutex;
 static std::vector<Agent*> start_io_thread_agents;
 static bool debug_signal_handler_started = false;
 
+template <typename Defer, typename OnFailure>
+void SyncJavaScriptHookState(Environment* env,
+                             JavaScriptHookState* state,
+                             Local<Function> enable_function,
+                             Local<Function> disable_function,
+                             bool require_bootstrapping,
+                             Defer defer,
+                             OnFailure on_failure) {
+  // The debugger can request an interrupt from within a hook's JS toggle
+  // function. A nested sync only records the new requested state; the
+  // outermost sync sees it when re-checking the loop after each toggle.
+  if (state->syncing) return;
+  state->syncing = true;
+  auto on_exit = OnScopeLeave([state]() { state->syncing = false; });
+
+  Isolate* isolate = env->isolate();
+  while (state->wanted != state->enabled) {
+    // No hook events will be emitted during cleanup, and calling into JS is no
+    // longer possible.
+    // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
+    if (!env->can_call_into_js()) return;
+
+    bool enable = state->wanted;
+    Local<Function> function = enable ? enable_function : disable_function;
+    if (function.IsEmpty()) return;
+
+    if (env->is_processing_v8_interrupt()) {
+      defer();
+      return;
+    }
+
+    if (require_bootstrapping) CHECK(env->has_run_bootstrapping_code());
+    Local<Context> context = env->context();
+    v8::TryCatch try_catch(isolate);
+    USE(function->Call(context, Undefined(isolate), 0, nullptr));
+    if (try_catch.HasCaught()) {
+      // Termination may abort the toggle invocation. Do not record a state that
+      // may not have taken effect, so a later sync can retry it.
+      if (try_catch.HasTerminated()) return;
+      PrintCaughtException(isolate, context, try_catch);
+      on_failure();
+    }
+    state->enabled = enable;
+  }
+}
+
 static void RequestIoThreadStartOnAgents() {
   Mutex::ScopedLock lock(start_io_thread_agents_mutex);
   for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart();
@@ -483,6 +529,16 @@ class SameThreadInspectorSession : public InspectorSession {
 
 void NotifyClusterWorkersDebugEnabled(Environment* env) {
   Isolate* isolate = env->isolate();
+
+  // Starting the inspector from an interrupt is safe, but emitting the cluster
+  // notification calls into JS.
+  if (env->is_processing_v8_interrupt()) {
+    env->SetImmediate(
+        [](Environment* env) { NotifyClusterWorkersDebugEnabled(env); },
+        CallbackFlags::kUnrefed);
+    return;
+  }
+
   HandleScope handle_scope(isolate);
 
   // Send message to enable debug in cluster workers
@@ -1000,54 +1056,40 @@ void Agent::SetupNetworkTracking(Local<Function> enable_function,
                                  Local<Function> disable_function) {
   parent_env_->set_inspector_enable_network_tracking(enable_function);
   parent_env_->set_inspector_disable_network_tracking(disable_function);
-  if (pending_enable_network_tracking) {
-    pending_enable_network_tracking = false;
-    EnableNetworkTracking();
-  } else if (pending_disable_network_tracking) {
-    pending_disable_network_tracking = false;
-    DisableNetworkTracking();
-  }
+  SyncNetworkTrackingState();
 }
 
 void Agent::EnableNetworkTracking() {
-  if (network_tracking_enabled_) {
-    return;
-  }
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> enable = parent_env_->inspector_enable_network_tracking();
-  if (enable.IsEmpty()) {
-    pending_enable_network_tracking = true;
-  } else {
-    ToggleNetworkTracking(parent_env_->isolate(), enable);
-    network_tracking_enabled_ = true;
-  }
+  network_tracking_state_.wanted = true;
+  SyncNetworkTrackingState();
 }
 
 void Agent::DisableNetworkTracking() {
-  if (!network_tracking_enabled_) {
-    return;
-  }
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> disable = parent_env_->inspector_disable_network_tracking();
-  if (disable.IsEmpty()) {
-    pending_disable_network_tracking = true;
-  } else if (!client_->hasConnectedSessions()) {
-    ToggleNetworkTracking(parent_env_->isolate(), disable);
-    network_tracking_enabled_ = false;
-  }
+  if (client_->hasConnectedSessions()) return;
+  network_tracking_state_.wanted = false;
+  SyncNetworkTrackingState();
 }
 
-void Agent::ToggleNetworkTracking(Isolate* isolate, Local<Function> fn) {
-  if (!parent_env_->can_call_into_js()) return;
-  auto context = parent_env_->context();
+void Agent::SyncNetworkTrackingState() {
+  Isolate* isolate = parent_env_->isolate();
   HandleScope scope(isolate);
-  CHECK(!fn.IsEmpty());
-  v8::TryCatch try_catch(isolate);
-  USE(fn->Call(context, Undefined(isolate), 0, nullptr));
-  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
-    PrintCaughtException(isolate, context, try_catch);
-    UNREACHABLE("Cannot toggle network tracking, please report this.");
-  }
+  SyncJavaScriptHookState(
+      parent_env_,
+      &network_tracking_state_,
+      parent_env_->inspector_enable_network_tracking(),
+      parent_env_->inspector_disable_network_tracking(),
+      false,
+      [this]() {
+        parent_env_->SetImmediate(
+            [](Environment* env) {
+              Agent* agent = env->inspector_agent();
+              if (agent != nullptr) agent->SyncNetworkTrackingState();
+            },
+            CallbackFlags::kUnrefed);
+      },
+      []() {
+        UNREACHABLE("Cannot toggle network tracking, please report this.");
+      });
 }
 
 void Agent::WaitForDisconnect() {
@@ -1098,7 +1140,7 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
 }
 
 void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
-  async_hook_wanted_ = enabled;
+  async_hook_state_.wanted = enabled;
   SyncAsyncHookState();
 }
 
@@ -1112,52 +1154,25 @@ void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
 // When it's not safe to call into JS, this is a no-op and we'll try again in
 // RegisterAsyncHook() (for 1) or from a scheduled immediate (for 2).
 void Agent::SyncAsyncHookState() {
-  // The debugger can request an interrupt within the toggle JS function itself,
-  // A nested call only records the new requested state, the outermost call sees
-  // it when re-checking the loop condition after each toggle.
-  if (syncing_async_hook_state_) return;
-  syncing_async_hook_state_ = true;
-  auto on_exit = OnScopeLeave([this]() { syncing_async_hook_state_ = false; });
-
   Isolate* isolate = parent_env_->isolate();
   HandleScope scope(isolate);
-  while (async_hook_wanted_ != async_hook_enabled_) {
-    // Guard against running this during cleanup -- no async events will be
-    // emitted anyway at that point anymore, and calling into JS is not
-    // possible. This should probably not be something we're attempting in the
-    // first place,
-    // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
-    if (!parent_env_->can_call_into_js()) return;
-
-    bool enable = async_hook_wanted_;
-    Local<Function> fn = enable ? parent_env_->inspector_enable_async_hooks()
-                                : parent_env_->inspector_disable_async_hooks();
-    if (fn.IsEmpty()) return;
-
-    if (parent_env_->is_processing_v8_interrupt()) {
-      parent_env_->SetImmediate(
-          [](Environment* env) {
-            Agent* agent = env->inspector_agent();
-            if (agent != nullptr) agent->SyncAsyncHookState();
-          },
-          CallbackFlags::kUnrefed);
-      return;
-    }
-
-    CHECK(parent_env_->has_run_bootstrapping_code());
-    Local<Context> context = parent_env_->context();
-    v8::TryCatch try_catch(isolate);
-    USE(fn->Call(context, Undefined(isolate), 0, nullptr));
-    if (try_catch.HasCaught()) {
-      // Termination may abort the toggle invocation, retrying now would just
-      // be terminated again. Instead of recording the toggle that may not have
-      // taken effect, leave the states as-is so that a later sync retries.
-      if (try_catch.HasTerminated()) return;
-      PrintCaughtException(isolate, context, try_catch);
-      UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
-    }
-    async_hook_enabled_ = enable;
-  }
+  SyncJavaScriptHookState(
+      parent_env_,
+      &async_hook_state_,
+      parent_env_->inspector_enable_async_hooks(),
+      parent_env_->inspector_disable_async_hooks(),
+      true,
+      [this]() {
+        parent_env_->SetImmediate(
+            [](Environment* env) {
+              Agent* agent = env->inspector_agent();
+              if (agent != nullptr) agent->SyncAsyncHookState();
+            },
+            CallbackFlags::kUnrefed);
+      },
+      []() {
+        UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
+      });
 }
 
 void Agent::AsyncTaskScheduled(const StringView& task_name, void* task,