diff --git a/common.gypi b/common.gypi index 9fc77e9445cb..21281097b731 100644 --- a/common.gypi +++ b/common.gypi @@ -44,7 +44,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.15', + 'v8_embedder_string': '-node.16', ##### V8 defaults for Node.js ##### @@ -56,6 +56,8 @@ # Refs: https://github.com/nodejs/node/issues/23167 # Enable compiler warnings when using V8_DEPRECATED apis from V8 code. 'v8_deprecation_warnings': 0, + # Check that JavaScript execution is disallowed in V8 API interrupts. + 'v8_disallow_js_in_api_interrupts_is_checked': 1, # Enable compiler warnings when using V8_DEPRECATE_SOON apis from V8 code. 'v8_imminent_deprecation_warnings': 0, @@ -557,6 +559,9 @@ ['v8_deprecation_warnings == 1', { 'defines': ['V8_DEPRECATION_WARNINGS',], }], + ['v8_disallow_js_in_api_interrupts_is_checked == 1', { + 'defines': ['V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED',], + }], ['v8_imminent_deprecation_warnings == 1', { 'defines': ['V8_IMMINENT_DEPRECATION_WARNINGS',], }], diff --git a/deps/v8/AUTHORS b/deps/v8/AUTHORS index ee6d6af3a38d..527caa37309f 100644 --- a/deps/v8/AUTHORS +++ b/deps/v8/AUTHORS @@ -102,6 +102,7 @@ Cheng Zhao Chengzhong Wu Choongwoo Han Chris Nardi +Christian Aurich Zanettini Martins Christopher A. Taylor Christopher Nady Colin Ihrig diff --git a/deps/v8/src/inspector/v8-debugger.cc b/deps/v8/src/inspector/v8-debugger.cc index 346c6c94a6dd..ab52a9b915f1 100644 --- a/deps/v8/src/inspector/v8-debugger.cc +++ b/deps/v8/src/inspector/v8-debugger.cc @@ -524,6 +524,11 @@ void V8Debugger::handleProgramBreak( DCHECK(contextGroupId); m_pausedContextGroupId = contextGroupId; + // Breaks scheduled through an API interrupt (Debugger.pause, OOM) arrive + // here with JavaScript execution disallowed. Like the pause message loop, + // the paused and resumed notifications can reach an embedder channel that + // runs JavaScript. + v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate); m_inspector->forEachSession( contextGroupId, [&pausedContext, &exception, &breakpointIds, &exceptionType, &isUncaught, @@ -535,7 +540,6 @@ void V8Debugger::handleProgramBreak( } }); { - v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate); v8::Context::Scope scope(pausedContext); m_inspector->forEachSession( diff --git a/deps/v8/test/unittests/inspector/inspector-unittest.cc b/deps/v8/test/unittests/inspector/inspector-unittest.cc index 7e08be739bf6..227cb5e0d4af 100644 --- a/deps/v8/test/unittests/inspector/inspector-unittest.cc +++ b/deps/v8/test/unittests/inspector/inspector-unittest.cc @@ -5,6 +5,8 @@ #include #include +#include "include/v8-external.h" +#include "include/v8-function.h" #include "include/v8-inspector.h" #include "include/v8-local-handle.h" #include "include/v8-primitive.h" @@ -625,5 +627,94 @@ TEST_F(InspectorTest, WrapObjectWithCustomPreviewNoPreview) { EXPECT_FALSE(remote_object->hasCustomPreview()); EXPECT_EQ(channel.triggerCount(), 0); } + +namespace { + +// Calls into JavaScript for every Debugger.paused and Debugger.resumed +// notification, like an embedder that exposes an in-process session to JS. +class JSCallbackChannel : public V8Inspector::Channel { + public: + JSCallbackChannel(v8::Isolate* isolate, v8::Local context, + v8::Local callback) + : m_isolate(isolate), + m_context(isolate, context), + m_callback(isolate, callback) {} + ~JSCallbackChannel() override = default; + + void sendResponse(int callId, + std::unique_ptr message) override {} + + void sendNotification(std::unique_ptr message) override { + String16 message_str = toString16(message->string()); + if (message_str.find("Debugger.paused") == String16::kNotFound && + message_str.find("Debugger.resumed") == String16::kNotFound) { + return; + } + v8::HandleScope handle_scope(m_isolate); + v8::Local context = m_context.Get(m_isolate); + v8::Context::Scope context_scope(context); + m_callback.Get(m_isolate) + ->Call(context, v8::Undefined(m_isolate), 0, nullptr) + .ToLocalChecked(); + } + + void flushProtocolNotifications() override {} + + private: + v8::Isolate* m_isolate; + v8::Global m_context; + v8::Global m_callback; +}; + +void DispatchPause(const v8::FunctionCallbackInfo& info) { + static_cast(info.Data().As()->Value( + v8::kExternalPointerTypeTagDefault)) + ->dispatchProtocolMessage( + toStringView(R"({"id": 2, "method": "Debugger.pause"})")); +} + +} // namespace + +TEST_F(InspectorTest, ChannelCanRunJSWhenPausedOnInterrupt) { + v8::Isolate* isolate = v8_isolate(); + v8::HandleScope handle_scope(isolate); + + v8_inspector::V8InspectorClient default_client; + std::unique_ptr inspector = + V8Inspector::create(isolate, &default_client); + V8ContextInfo context_info(v8_context(), 1, toStringView("")); + inspector->contextCreated(context_info); + + v8::Local callback = + RunJS("globalThis.notifications = 0; () => notifications++") + .As(); + JSCallbackChannel channel(isolate, v8_context(), callback); + std::unique_ptr session = inspector->connect( + 1, &channel, toStringView("{}"), v8_inspector::V8Inspector::kFullyTrusted, + v8_inspector::V8Inspector::kNotWaitingForDebugger); + session->dispatchProtocolMessage( + toStringView(R"({"id": 1, "method": "Debugger.enable"})")); + + v8::Local pause = + v8::Function::New(v8_context(), &DispatchPause, + v8::External::New(isolate, session.get(), + v8::kExternalPointerTypeTagDefault)) + .ToLocalChecked(); + CHECK(v8_context() + ->Global() + ->Set(v8_context(), NewString("pause"), pause) + .FromJust()); + + // Debugger.pause while JavaScript runs schedules the break through an API + // interrupt, where JavaScript execution is disallowed. The loop spins until + // it takes that interrupt on a back edge. Taken on function entry instead, + // V8 would step in and pause at the next statement, outside the interrupt, + // so the count is read in the same script. + v8::Local result = RunJS( + "pause();" + "for (let i = 0; i < 1e7 && notifications === 0; i++) {}" + "notifications"); + CHECK_EQ(result->Int32Value(v8_context()).FromJust(), 2); +} } // namespace internal } // namespace v8 diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 22506e26b32c..5f1a3090e0af 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -80,6 +80,52 @@ static Mutex start_io_thread_agents_mutex; static std::vector start_io_thread_agents; static bool debug_signal_handler_started = false; +template +void SyncJavaScriptHookState(Environment* env, + JavaScriptHookState* state, + Local enable_function, + Local 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 = 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 = 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 enable_function, Local 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 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 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 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 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 = 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, diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 5a1d1de26547..6b92ef2a08fe 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -29,6 +29,12 @@ class ParentInspectorHandle; class NodeInspectorClient; class WorkerManager; +struct JavaScriptHookState { + bool wanted = false; + bool enabled = false; + bool syncing = false; +}; + class InspectorSession { public: virtual ~InspectorSession() = default; @@ -134,7 +140,7 @@ class Agent { private: void SyncAsyncHookState(); - void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local fn); + void SyncNetworkTrackingState(); node::Environment* parent_env_; // Encapsulates majority of the Inspector functionality @@ -154,9 +160,7 @@ class Agent { // The state of the async hook used for async stack traces that the protocol // last requested, and the state JS currently has. SyncAsyncHookState() // reconciles the two when it is possible and safe to call into JS. - bool async_hook_wanted_ = false; - bool async_hook_enabled_ = false; - bool syncing_async_hook_state_ = false; + JavaScriptHookState async_hook_state_; // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(), // whichever runs first, and freed by its close callback. @@ -164,9 +168,9 @@ class Agent { void StopAcceptingIoThreadStarts(); static void StopAcceptingIoThreadStartsHook(void* agent); - bool network_tracking_enabled_ = false; - bool pending_enable_network_tracking = false; - bool pending_disable_network_tracking = false; + // Network tracking uses JS hooks. Reconcile the protocol requested and + // applied states after leaving a V8 interrupt. + JavaScriptHookState network_tracking_state_; std::shared_ptr network_resource_manager_; }; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 336930f18679..98dc03b2d6ad 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -62,13 +62,31 @@ struct MainThreadConnection { template class JSBindingsConnection : public BaseObject { public: + struct PendingMicrotask { + explicit PendingMicrotask(JSBindingsConnection* connection) + : env(connection->env()), connection(connection) { + env->AddCleanupHook(Delete, this); + } + + static void Delete(void* data) { + delete static_cast(data); + } + + static void Run(void* data) { + std::unique_ptr pending( + static_cast(data)); + pending->env->RemoveCleanupHook(Delete, pending.get()); + pending->connection->FlushPendingMessages(); + } + + Environment* env; + BaseObjectPtr connection; + }; + class JSBindingsSessionDelegate : public InspectorSessionDelegate { public: - JSBindingsSessionDelegate(Environment* env, - JSBindingsConnection* connection) - : env_(env), - connection_(connection) { - } + explicit JSBindingsSessionDelegate(JSBindingsConnection* connection) + : connection_(connection) {} void SendMessageToFrontend(const v8_inspector::StringView& message) override { @@ -76,7 +94,6 @@ class JSBindingsConnection : public BaseObject { } private: - Environment* env_; BaseObjectPtr connection_; }; @@ -86,10 +103,10 @@ class JSBindingsConnection : public BaseObject { : BaseObject(env, wrap), callback_(env->isolate(), callback) { Agent* inspector = env->inspector_agent(); session_ = ConnectionType::Connect( - inspector, std::make_unique(env, this)); - // Inspector responses may be produced from a GC weak callback, where - // invoking the JavaScript session callback is forbidden. Defer delivery - // until the GC has completed. + inspector, std::make_unique(this)); + // Inspector responses may be produced from a GC weak callback or a V8 + // interrupt, where invoking the JavaScript session callback is forbidden. + // Defer delivery until it is safe to call into JavaScript. env->isolate()->AddGCPrologueCallback(GCPrologueCallback, this); env->isolate()->AddGCEpilogueCallback(GCEpilogueCallback, this); } @@ -100,7 +117,8 @@ class JSBindingsConnection : public BaseObject { } void SendMessageToFrontend(const v8_inspector::StringView& message) { - if (in_gc_ || delivering_ || !pending_messages_.empty()) { + if (in_gc_ || env()->is_processing_v8_interrupt() || delivering_ || + !pending_messages_.empty()) { pending_messages_.emplace_back( message.is8Bit() ? std::u16string(message.characters8(), @@ -128,6 +146,12 @@ class JSBindingsConnection : public BaseObject { void ScheduleFlush() { if (flush_scheduled_ || delivering_) return; flush_scheduled_ = true; + if (!in_gc_ && env()->is_processing_v8_interrupt()) { + auto* pending = new PendingMicrotask(this); + env()->context()->GetMicrotaskQueue()->EnqueueMicrotask( + env()->isolate(), PendingMicrotask::Run, pending); + return; + } BaseObjectPtr strong_ref{this}; env()->SetImmediate( [strong_ref](Environment*) { strong_ref->FlushPendingMessages(); }, diff --git a/test/parallel/test-inspector-pause-with-js-session.js b/test/parallel/test-inspector-pause-with-js-session.js new file mode 100644 index 000000000000..b509d30c125f --- /dev/null +++ b/test/parallel/test-inspector-pause-with-js-session.js @@ -0,0 +1,53 @@ +'use strict'; +const common = require('../common'); + +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const { NodeInstance } = require('../common/inspector-helper.js'); + +// An in-process session observes Debugger.paused while the process is paused, +// so its callback runs from the Isolate::RequestInterrupt handler that +// Debugger.pause schedules, where V8 asserts that JS is not executed. +const script = ` +const { Session } = require('inspector'); +const session = new Session(); +let paused = false; +session.on('Debugger.paused', () => { + paused = true; +}); +session.connect(); +session.post('Debugger.enable'); +console.log('Ready'); +// Spin so that the pause is requested while JS is on the stack, which is what +// makes V8 break from the interrupt handler instead of on the next call. +const deadline = Date.now() + ${common.platformTimeout(10000)}; +while (!paused && Date.now() < deadline); +console.log(paused ? 'Notified' : 'Not notified'); +`; + +async function runTest() { + const child = new NodeInstance(undefined, script); + const session = await child.connectInspectorSession(); + await session.send({ method: 'NodeRuntime.enable' }); + await session.waitForNotification('NodeRuntime.waitingForDebugger'); + await session.send([ + { 'method': 'Runtime.enable' }, + { 'method': 'Debugger.enable' }, + ]); + await session.send({ method: 'Runtime.runIfWaitingForDebugger' }); + await session.send({ method: 'NodeRuntime.disable' }); + await session.waitForNotification('Debugger.paused', 'Break on start'); + await session.send({ 'method': 'Debugger.resume' }); + await session.waitForConsoleOutput('log', ['Ready']); + + await session.send({ 'method': 'Debugger.pause' }); + await session.waitForNotification('Debugger.paused', 'Paused'); + await session.send({ 'method': 'Debugger.resume' }); + await session.waitForConsoleOutput('log', ['Notified']); + + await session.waitForDisconnect(); + assert.strictEqual((await child.expectShutdown()).exitCode, 0); +} + +runTest().then(common.mustCall()); diff --git a/tools/v8_gypfiles/features.gypi b/tools/v8_gypfiles/features.gypi index a6e2c9b0d0ef..f9c5494217ec 100644 --- a/tools/v8_gypfiles/features.gypi +++ b/tools/v8_gypfiles/features.gypi @@ -107,6 +107,9 @@ # Enable compiler warnings when using V8_DEPRECATED apis. 'v8_deprecation_warnings%': 0, + # Check that JavaScript execution is disallowed in V8 API interrupts. + 'v8_disallow_js_in_api_interrupts_is_checked%': 0, + # Enable compiler warnings when using V8_DEPRECATE_SOON apis. 'v8_imminent_deprecation_warnings%': 0, @@ -422,6 +425,9 @@ ['v8_deprecation_warnings==1', { 'defines': ['V8_DEPRECATION_WARNINGS',], }], + ['v8_disallow_js_in_api_interrupts_is_checked==1', { + 'defines': ['V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED',], + }], ['v8_imminent_deprecation_warnings==1', { 'defines': ['V8_IMMINENT_DEPRECATION_WARNINGS',], }],