From a12b52bf1b0ffc751a7bd16746cd6f812c8fc416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Jos=C3=A9=20Arboleda?= Date: Fri, 7 Nov 2025 14:26:58 -0500 Subject: [PATCH] deps: V8: cherry-pick 5ba9200cd046 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [debug] Fix crash when pausing in for-of loop header Given a for-of loop: for (const each of subject) { The bytecode generator emits the iterator.next call + done check + assigning to `each` all into the source position of `const each`. The pseudo-desugared code looks something like: var tmp; loop { var result = iterator.next(); if (result.done) break; tmp = result.value; PushBlockContext; const each = tmp; // rest of the loop. } This is a problem, as the parser starts the block scope already on the `const each`. If the scope requires a context we can pause on bytecode that has or has not pushed the block context yet, while the source position looks the same. The recent addition of per-script unique scope IDs lets us fix this problem in the debugger: We can check if the scope ID of the runtime scope matches the parser scope. If not, the context was not pushed yet. The debugger already has a `HasContext` helper. We extend it to also check for matching scope IDs and then use `HasContext` where we would read variable values off the context. If the context was not pushed yet, we report them as 'unavailable'. R=leszeks@chromium.org Fixed: 384413079,399002824 Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402 Reviewed-by: Leszek Swirski Commit-Queue: Simon Zünd Cr-Commit-Position: refs/heads/main@{#100029} Refs: https://github.com/v8/v8/commit/5ba9200cd04602c486ff5a5fc4f9e0d4ab19606b Fixes: https://github.com/nodejs/node/issues/60580 PR-URL: https://github.com/nodejs/node/pull/60620 Signed-off-by: Juan José Arboleda --- common.gypi | 2 +- deps/v8/src/debug/debug-scopes.cc | 48 +++++++++++++++---- deps/v8/src/debug/debug-scopes.h | 1 + .../regress-crbug-399002824-expected.txt | 5 ++ .../regress/regress-crbug-399002824.js | 35 ++++++++++++++ 5 files changed, 80 insertions(+), 11 deletions(-) create mode 100644 deps/v8/test/inspector/regress/regress-crbug-399002824-expected.txt create mode 100644 deps/v8/test/inspector/regress/regress-crbug-399002824.js diff --git a/common.gypi b/common.gypi index a6896876d45e4c..479fd0e6d7d60f 100644 --- a/common.gypi +++ b/common.gypi @@ -38,7 +38,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.31', + 'v8_embedder_string': '-node.32', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/debug/debug-scopes.cc b/deps/v8/src/debug/debug-scopes.cc index 1d6ceb72a0a108..d9595c2372b7e0 100644 --- a/deps/v8/src/debug/debug-scopes.cc +++ b/deps/v8/src/debug/debug-scopes.cc @@ -431,6 +431,22 @@ bool ScopeIterator::DeclaresLocals(Mode mode) const { } bool ScopeIterator::HasContext() const { + // In rare cases we pause in a scope that doesn't have its context pushed yet. + // E.g. when pausing in for-of loop headers (see https://crbug.com/399002824). + // + // We can detect this by comparing the scope ID of the parsed scope and the + // runtime scope. + // We can skip this check for function scopes, those will have their context + // always pushed. Also, there is an oddity where parsing::ParseFunction + // produces function scopes with (-1, -1) as the start/end position, + // which messes up the unique ID. + if (current_scope_ && !current_scope_->is_function_scope() && + NeedsContext() && + current_scope_->UniqueIdInScript() != + context_->scope_info()->UniqueIdInScript()) { + return false; + } + return !InInnerScope() || NeedsContext(); } @@ -475,7 +491,7 @@ void ScopeIterator::AdvanceScope() { DCHECK(InInnerScope()); do { - if (NeedsContext()) { + if (NeedsAndHasContext()) { // current_scope_ needs a context so moving one scope up requires us to // also move up one context. AdvanceOneContext(); @@ -538,6 +554,11 @@ void ScopeIterator::Next() { MaybeCollectAndStoreLocalBlocklists(); UnwrapEvaluationContext(); + DCHECK_IMPLIES(current_scope_ && !current_scope_->is_function_scope() && + NeedsAndHasContext(), + current_scope_->UniqueIdInScript() == + context_->scope_info()->UniqueIdInScript()); + if (leaving_closure) function_ = Handle(); } @@ -547,32 +568,33 @@ ScopeIterator::ScopeType ScopeIterator::Type() const { if (InInnerScope()) { switch (current_scope_->scope_type()) { case FUNCTION_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsFunctionContext() || - context_->IsDebugEvaluateContext()); + DCHECK_IMPLIES(NeedsAndHasContext(), + context_->IsFunctionContext() || + context_->IsDebugEvaluateContext()); return ScopeTypeLocal; case MODULE_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsModuleContext()); + DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsModuleContext()); return ScopeTypeModule; case SCRIPT_SCOPE: case REPL_MODE_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsScriptContext() || - IsNativeContext(*context_)); + DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsScriptContext() || + IsNativeContext(*context_)); return ScopeTypeScript; case WITH_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsWithContext()); + DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsWithContext()); return ScopeTypeWith; case CATCH_SCOPE: DCHECK(context_->IsCatchContext()); return ScopeTypeCatch; case BLOCK_SCOPE: case CLASS_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsBlockContext()); + DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsBlockContext()); return ScopeTypeBlock; case EVAL_SCOPE: - DCHECK_IMPLIES(NeedsContext(), context_->IsEvalContext()); + DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsEvalContext()); return ScopeTypeEval; case SHADOW_REALM_SCOPE: - DCHECK_IMPLIES(NeedsContext(), IsNativeContext(*context_)); + DCHECK_IMPLIES(NeedsAndHasContext(), IsNativeContext(*context_)); // TODO(v8:11989): New ScopeType for ShadowRealms? return ScopeTypeScript; } @@ -962,6 +984,12 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode, case VariableLocation::CONTEXT: if (mode == Mode::STACK) continue; + if (!HasContext()) { + // If the context was not yet pushed we report the variable as + // unavailable. + value = isolate_->factory()->the_hole_value(); + break; + } DCHECK(var->IsContextSlot()); DCHECK_EQ(context_->scope_info()->ContextSlotIndex(var->name()), index); diff --git a/deps/v8/src/debug/debug-scopes.h b/deps/v8/src/debug/debug-scopes.h index e02f1b73702e2b..f5736220f6bc0e 100644 --- a/deps/v8/src/debug/debug-scopes.h +++ b/deps/v8/src/debug/debug-scopes.h @@ -106,6 +106,7 @@ class V8_EXPORT_PRIVATE ScopeIterator { bool InInnerScope() const { return !function_.is_null(); } bool HasContext() const; bool NeedsContext() const; + bool NeedsAndHasContext() const { return NeedsContext() && HasContext(); } Handle CurrentContext() const { DCHECK(HasContext()); return context_; diff --git a/deps/v8/test/inspector/regress/regress-crbug-399002824-expected.txt b/deps/v8/test/inspector/regress/regress-crbug-399002824-expected.txt new file mode 100644 index 00000000000000..f94e3c7abebffc --- /dev/null +++ b/deps/v8/test/inspector/regress/regress-crbug-399002824-expected.txt @@ -0,0 +1,5 @@ +Don't crash when pausing on the iterator ".next" call + function crashMe() { + for (const #e of iter()) { + () => e; // Context allocate e. + diff --git a/deps/v8/test/inspector/regress/regress-crbug-399002824.js b/deps/v8/test/inspector/regress/regress-crbug-399002824.js new file mode 100644 index 00000000000000..45c06e7d5d8174 --- /dev/null +++ b/deps/v8/test/inspector/regress/regress-crbug-399002824.js @@ -0,0 +1,35 @@ +// Copyright 2025 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +const { session, Protocol, contextGroup } = + InspectorTest.start('Don\'t crash when pausing on the iterator ".next" call'); + +session.setupScriptMap(); +contextGroup.addScript(` + function *iter() { + yield 1; + debugger; + yield 2; + } + + function crashMe() { + for (const e of iter()) { + () => e; // Context allocate e. + } + } +`); + +Protocol.Debugger.enable(); + +(async () => { + let pausedPromise = Protocol.Debugger.oncePaused(); + Protocol.Runtime.evaluate({ expression: 'crashMe()' }); + + let { params: { callFrames } } = await pausedPromise; + await session.logSourceLocation(callFrames[1].location); + + Protocol.Debugger.resume(); + + InspectorTest.completeTest(); +})();