From 0c1c33a2a3dec06b1cea4f9cff2f9bd757ba23f3 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 18 Jun 2024 18:02:25 +0200 Subject: [PATCH] inspector: fix disable async hooks on Debugger.setAsyncCallStackDepth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was previously calling the enable function by mistake. As a result, when profiling using Chrome DevTools, the async hooks won't be turned off properly after receiving Debugger.setAsyncCallStackDepth with depth 0. PR-URL: https://github.com/nodejs/node/pull/53473 Reviewed-By: Michaël Zasso Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum --- src/async_wrap.cc | 8 ++ src/env.cc | 12 +++ src/env.h | 4 +- src/inspector_agent.cc | 2 +- .../test-inspector-async-call-stack.js | 86 +++++++++---------- 5 files changed, 65 insertions(+), 47 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 8b784cddf41..4b96331c187 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -193,6 +193,12 @@ static void SetPromiseHooks(const FunctionCallbackInfo& args) { args[3]->IsFunction() ? args[3].As() : Local()); } +static void GetPromiseHooks(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + args.GetReturnValue().Set( + env->async_hooks()->GetPromiseHooks(args.GetIsolate())); +} + class DestroyParam { public: double asyncId; @@ -364,6 +370,7 @@ void AsyncWrap::CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "clearAsyncIdStack", ClearAsyncIdStack); SetMethod(isolate, target, "queueDestroyAsyncId", QueueDestroyAsyncId); SetMethod(isolate, target, "setPromiseHooks", SetPromiseHooks); + SetMethod(isolate, target, "getPromiseHooks", GetPromiseHooks); SetMethod(isolate, target, "registerDestroyHook", RegisterDestroyHook); AsyncWrap::GetConstructorTemplate(isolate_data); } @@ -469,6 +476,7 @@ void AsyncWrap::RegisterExternalReferences( registry->Register(ClearAsyncIdStack); registry->Register(QueueDestroyAsyncId); registry->Register(SetPromiseHooks); + registry->Register(GetPromiseHooks); registry->Register(RegisterDestroyHook); registry->Register(AsyncWrap::GetAsyncId); registry->Register(AsyncWrap::AsyncReset); diff --git a/src/env.cc b/src/env.cc index 5a7c5a4ddfc..7835c562f8e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -87,6 +87,18 @@ void AsyncHooks::ResetPromiseHooks(Local init, js_promise_hooks_[3].Reset(env()->isolate(), resolve); } +Local AsyncHooks::GetPromiseHooks(Isolate* isolate) { + std::vector> values; + for (size_t i = 0; i < js_promise_hooks_.size(); ++i) { + if (js_promise_hooks_[i].IsEmpty()) { + values.push_back(Undefined(isolate)); + } else { + values.push_back(js_promise_hooks_[i].Get(isolate)); + } + } + return Array::New(isolate, values.data(), values.size()); +} + void Environment::ResetPromiseHooks(Local init, Local before, Local after, diff --git a/src/env.h b/src/env.h index 58419bd31f1..a864da46fb2 100644 --- a/src/env.h +++ b/src/env.h @@ -322,7 +322,9 @@ class AsyncHooks : public MemoryRetainer { v8::Local before, v8::Local after, v8::Local resolve); - + // Used for testing since V8 doesn't provide API for retrieving configured + // JS promise hooks. + v8::Local GetPromiseHooks(v8::Isolate* isolate); inline v8::Local provider_string(int idx); inline void no_force_checks(); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 16021a9d19e..f298ab73285 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -923,7 +923,7 @@ void Agent::EnableAsyncHook() { void Agent::DisableAsyncHook() { HandleScope scope(parent_env_->isolate()); - Local disable = parent_env_->inspector_enable_async_hooks(); + Local disable = parent_env_->inspector_disable_async_hooks(); if (!disable.IsEmpty()) { ToggleAsyncHook(parent_env_->isolate(), disable); } else if (pending_enable_async_hook_) { diff --git a/test/parallel/test-inspector-async-call-stack.js b/test/parallel/test-inspector-async-call-stack.js index f050487da7a..d42cf42c7d7 100644 --- a/test/parallel/test-inspector-async-call-stack.js +++ b/test/parallel/test-inspector-async-call-stack.js @@ -5,21 +5,32 @@ common.skipIfInspectorDisabled(); common.skipIf32Bits(); const assert = require('assert'); +const { inspect } = require('util'); const { internalBinding } = require('internal/test/binding'); -const { async_hook_fields, constants } = internalBinding('async_wrap'); +const { async_hook_fields, constants, getPromiseHooks } = internalBinding('async_wrap'); const { kTotals } = constants; -const inspector = require('inspector'); +const inspector = require('inspector/promises'); const setDepth = 'Debugger.setAsyncCallStackDepth'; - +const emptyPromiseHooks = [ undefined, undefined, undefined, undefined ]; function verifyAsyncHookDisabled(message) { assert.strictEqual(async_hook_fields[kTotals], 0, `${async_hook_fields[kTotals]} !== 0: ${message}`); + const promiseHooks = getPromiseHooks(); + assert.deepStrictEqual( + promiseHooks, emptyPromiseHooks, + `${message}: promise hooks ${inspect(promiseHooks)}` + ); } function verifyAsyncHookEnabled(message) { assert.strictEqual(async_hook_fields[kTotals], 4, `${async_hook_fields[kTotals]} !== 4: ${message}`); + const promiseHooks = getPromiseHooks(); + assert.notDeepStrictEqual( + promiseHooks, emptyPromiseHooks, + `${message}: promise hooks ${inspect(promiseHooks)}` + ); } // By default inspector async hooks should not have been installed. @@ -31,53 +42,38 @@ verifyAsyncHookDisabled('creating a session should not enable async hooks'); session.connect(); verifyAsyncHookDisabled('connecting a session should not enable async hooks'); -session.post('Debugger.enable', () => { +(async () => { + await session.post('Debugger.enable'); verifyAsyncHookDisabled('enabling debugger should not enable async hooks'); + await assert.rejects(session.post(setDepth, { invalid: 'message' }), { code: 'ERR_INSPECTOR_COMMAND' }); + verifyAsyncHookDisabled('invalid message should not enable async hooks'); + await assert.rejects(session.post(setDepth, { maxDepth: 'five' }), { code: 'ERR_INSPECTOR_COMMAND' }); + verifyAsyncHookDisabled('invalid maxDepth (string) should not enable ' + + 'async hooks'); + await assert.rejects(session.post(setDepth, { maxDepth: NaN }), { code: 'ERR_INSPECTOR_COMMAND' }); + verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable ' + + 'async hooks'); + await session.post(setDepth, { maxDepth: 10 }); + verifyAsyncHookEnabled('valid message should enable async hooks'); - session.post(setDepth, { invalid: 'message' }, () => { - verifyAsyncHookDisabled('invalid message should not enable async hooks'); + await session.post(setDepth, { maxDepth: 0 }); + verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' + + 'async hooks'); - session.post(setDepth, { maxDepth: 'five' }, () => { - verifyAsyncHookDisabled('invalid maxDepth (string) should not enable ' + - 'async hooks'); + await session.post(setDepth, { maxDepth: 32 }); + verifyAsyncHookEnabled('valid message should enable async hooks'); - session.post(setDepth, { maxDepth: NaN }, () => { - verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable ' + - 'async hooks'); + await session.post('Debugger.disable'); + verifyAsyncHookDisabled('Debugger.disable should disable async hooks'); - session.post(setDepth, { maxDepth: 10 }, () => { - verifyAsyncHookEnabled('valid message should enable async hooks'); + await session.post('Debugger.enable'); + verifyAsyncHookDisabled('Enabling debugger should not enable hooks'); - session.post(setDepth, { maxDepth: 0 }, () => { - verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' + - 'async hooks'); + await session.post(setDepth, { maxDepth: 64 }); + verifyAsyncHookEnabled('valid message should enable async hooks'); - runTestSet2(session); - }); - }); - }); - }); - }); -}); + await session.disconnect(); -function runTestSet2(session) { - session.post(setDepth, { maxDepth: 32 }, () => { - verifyAsyncHookEnabled('valid message should enable async hooks'); - - session.post('Debugger.disable', () => { - verifyAsyncHookDisabled('Debugger.disable should disable async hooks'); - - session.post('Debugger.enable', () => { - verifyAsyncHookDisabled('Enabling debugger should not enable hooks'); - - session.post(setDepth, { maxDepth: 64 }, () => { - verifyAsyncHookEnabled('valid message should enable async hooks'); - - session.disconnect(); - verifyAsyncHookDisabled('Disconnecting session should disable ' + - 'async hooks'); - }); - }); - }); - }); -} + verifyAsyncHookDisabled('Disconnecting session should disable ' + + 'async hooks'); +})().then(common.mustCall());