Skip to content

Commit

Permalink
test: workaround for V8 8.1 inspector pause issue
Browse files Browse the repository at this point in the history
`test-inspector-multisession-ws` and `test-inspector-break-when-eval`
will be affected by an upstream bug when we upgrade V8 to 8.1. The bug
is caused when the Inspector sets a pause at the start of a function
compiled with `CompileFunctionInContext`, but that function hasn't been
executed yet.

On both tests, this issue is triggered by pausing while in C++ executing
LookupAndCompile, which is called by requiring internal modules while
running `console.log`. To eliminate this issue in both tests, we add an
extra `console.log` to ensure we only pause we required all internal
modules we need. On `test-inspector-break-when-eval`, we also need to
start the child process with `--inspect-brk` instead of `--inspect` to
ensure the test is predictable (this test would occasianlly fail on
slower machines, when console.log doesn't run fast enough to finish
after emitting `Runtime.consoleAPICalled` and before the parent process
sending `Runtime.evaluate` message.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=10287

PR-URL: nodejs#32234
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=10287
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
  • Loading branch information
mmarchini authored and gengjiawen committed Mar 16, 2020
1 parent a022d38 commit 2e3dc12
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 2 deletions.
4 changes: 4 additions & 0 deletions test/fixtures/inspector-global-function.js
Expand Up @@ -10,4 +10,8 @@ global.sum = function() {
console.log(invocations++, c);
};

// NOTE(mmarchini): Calls console.log two times to ensure we loaded every
// internal module before pausing. See
// https://bugs.chromium.org/p/v8/issues/detail?id=10287.
console.log('Loading');
console.log('Ready!');
5 changes: 5 additions & 0 deletions test/parallel/test-inspector-multisession-ws.js
Expand Up @@ -20,6 +20,7 @@ session.on('Debugger.paused', () => {
session.connect();
session.post('Debugger.enable');
console.log('Ready');
console.log('Ready');
`;

async function setupSession(node) {
Expand All @@ -46,6 +47,10 @@ async function testSuspend(sessionA, sessionB) {
await sessionA.waitForNotification('Debugger.paused', 'Initial pause');
sessionA.send({ 'method': 'Debugger.resume' });

await sessionA.waitForNotification('Runtime.consoleAPICalled',
'Console output');
// NOTE(mmarchini): Remove second console.log when
// https://bugs.chromium.org/p/v8/issues/detail?id=10287 is fixed.
await sessionA.waitForNotification('Runtime.consoleAPICalled',
'Console output');
sessionA.send({ 'method': 'Debugger.pause' });
Expand Down
14 changes: 12 additions & 2 deletions test/sequential/test-inspector-break-when-eval.js
Expand Up @@ -18,7 +18,15 @@ async function setupDebugger(session) {
{ 'method': 'Runtime.runIfWaitingForDebugger' },
];
session.send(commands);
await session.waitForNotification('Runtime.consoleAPICalled');

await session.waitForNotification('Debugger.paused', 'Initial pause');

// NOTE(mmarchini): We wait for the second console.log to ensure we loaded
// every internal module before pausing. See
// https://bugs.chromium.org/p/v8/issues/detail?id=10287.
const waitForReady = session.waitForConsoleOutput('log', 'Ready!');
session.send({ 'method': 'Debugger.resume' });
await waitForReady;
}

async function breakOnLine(session) {
Expand Down Expand Up @@ -56,7 +64,9 @@ async function stepOverConsoleStatement(session) {
}

async function runTests() {
const child = new NodeInstance(['--inspect=0'], undefined, script);
// NOTE(mmarchini): Use --inspect-brk to improve avoid undeterministic
// behavior.
const child = new NodeInstance(['--inspect-brk=0'], undefined, script);
const session = await child.connectInspectorSession();
await setupDebugger(session);
await breakOnLine(session);
Expand Down

0 comments on commit 2e3dc12

Please sign in to comment.