Skip to content

Commit

Permalink
inspector: fix disable async hooks on Debugger.setAsyncCallStackDepth
Browse files Browse the repository at this point in the history 
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: #53473
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
@joyeecheung @marco-ippolito
joyeecheung authored and marco-ippolito committed Jul 19, 2024
1 parent 01b61d6 commit eb0b68b
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 54 deletions.
8 changes: 8 additions & 0 deletions src/async_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ static void SetPromiseHooks(const FunctionCallbackInfo<Value>& args) {
args[3]->IsFunction() ? args[3].As<Function>() : Local<Function>());
}

static void GetPromiseHooks(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
args.GetReturnValue().Set(
env->async_hooks()->GetPromiseHooks(args.GetIsolate()));
}

class DestroyParam {
public:
double asyncId;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 12 additions & 0 deletions src/env.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,18 @@ void AsyncHooks::ResetPromiseHooks(Local<Function> init,
js_promise_hooks_[3].Reset(env()->isolate(), resolve);
}

Local<Array> AsyncHooks::GetPromiseHooks(Isolate* isolate) {
std::vector<Local<Value>> 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<Function> init,
Local<Function> before,
Local<Function> after,
Expand Down
4 changes: 3 additions & 1 deletion src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,9 @@ class AsyncHooks : public MemoryRetainer {
v8::Local<v8::Function> before,
v8::Local<v8::Function> after,
v8::Local<v8::Function> resolve);

// Used for testing since V8 doesn't provide API for retrieving configured
// JS promise hooks.
v8::Local<v8::Array> GetPromiseHooks(v8::Isolate* isolate);
inline v8::Local<v8::String> provider_string(int idx);

inline void no_force_checks();
Expand Down
2 changes: 1 addition & 1 deletion src/inspector_agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -923,7 +923,7 @@ void Agent::EnableAsyncHook() {

void Agent::DisableAsyncHook() {
HandleScope scope(parent_env_->isolate());
Local<Function> disable = parent_env_->inspector_enable_async_hooks();
Local<Function> disable = parent_env_->inspector_disable_async_hooks();
if (!disable.IsEmpty()) {
ToggleAsyncHook(parent_env_->isolate(), disable);
} else if (pending_enable_async_hook_) {
Expand Down
100 changes: 48 additions & 52 deletions test/parallel/test-inspector-async-call-stack.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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');

session.post(setDepth, { invalid: 'message' }, () => {
verifyAsyncHookDisabled('invalid message should not enable async hooks');

session.post(setDepth, { maxDepth: 'five' }, () => {
verifyAsyncHookDisabled('invalid maxDepth (string) should not enable ' +
'async hooks');

session.post(setDepth, { maxDepth: NaN }, () => {
verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable ' +
'async hooks');

session.post(setDepth, { maxDepth: 10 }, () => {
verifyAsyncHookEnabled('valid message should enable async hooks');

session.post(setDepth, { maxDepth: 0 }, () => {
verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' +
'async hooks');

runTestSet2(session);
});
});
});
});
});
});

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');
});
});
});
});
}
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');

await session.post(setDepth, { maxDepth: 0 });
verifyAsyncHookDisabled('Setting maxDepth to 0 should disable ' +
'async hooks');

await session.post(setDepth, { maxDepth: 32 });
verifyAsyncHookEnabled('valid message should enable async hooks');

await session.post('Debugger.disable');
verifyAsyncHookDisabled('Debugger.disable should disable async hooks');

await session.post('Debugger.enable');
verifyAsyncHookDisabled('Enabling debugger should not enable hooks');

await session.post(setDepth, { maxDepth: 64 });
verifyAsyncHookEnabled('valid message should enable async hooks');

await session.disconnect();

verifyAsyncHookDisabled('Disconnecting session should disable ' +
'async hooks');
})().then(common.mustCall());

0 comments on commit eb0b68b

Please sign in to comment.