Skip to content

Commit fa32f3f

Browse files
committed
deps: cherry-pick 5ba9200 from V8 upstream
Original commit message: 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 <leszeks@chromium.org> Commit-Queue: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/main@{#100029} Refs: v8/v8@5ba9200 Fixes: nodejs#60580 PR-URL: nodejs#60620 Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
1 parent dd9a117 commit fa32f3f

File tree

4 files changed

+79
-10
lines changed

4 files changed

+79
-10
lines changed

deps/v8/src/debug/debug-scopes.cc

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,22 @@ bool ScopeIterator::DeclaresLocals(Mode mode) const {
431431
}
432432

433433
bool ScopeIterator::HasContext() const {
434+
// In rare cases we pause in a scope that doesn't have its context pushed yet.
435+
// E.g. when pausing in for-of loop headers (see https://crbug.com/399002824).
436+
//
437+
// We can detect this by comparing the scope ID of the parsed scope and the
438+
// runtime scope.
439+
// We can skip this check for function scopes, those will have their context
440+
// always pushed. Also, there is an oddity where parsing::ParseFunction
441+
// produces function scopes with (-1, -1) as the start/end position,
442+
// which messes up the unique ID.
443+
if (current_scope_ && !current_scope_->is_function_scope() &&
444+
NeedsContext() &&
445+
current_scope_->UniqueIdInScript() !=
446+
context_->scope_info()->UniqueIdInScript()) {
447+
return false;
448+
}
449+
434450
return !InInnerScope() || NeedsContext();
435451
}
436452

@@ -475,7 +491,7 @@ void ScopeIterator::AdvanceScope() {
475491
DCHECK(InInnerScope());
476492

477493
do {
478-
if (NeedsContext()) {
494+
if (NeedsAndHasContext()) {
479495
// current_scope_ needs a context so moving one scope up requires us to
480496
// also move up one context.
481497
AdvanceOneContext();
@@ -538,6 +554,11 @@ void ScopeIterator::Next() {
538554
MaybeCollectAndStoreLocalBlocklists();
539555
UnwrapEvaluationContext();
540556

557+
DCHECK_IMPLIES(current_scope_ && !current_scope_->is_function_scope() &&
558+
NeedsAndHasContext(),
559+
current_scope_->UniqueIdInScript() ==
560+
context_->scope_info()->UniqueIdInScript());
561+
541562
if (leaving_closure) function_ = Handle<JSFunction>();
542563
}
543564

@@ -547,32 +568,33 @@ ScopeIterator::ScopeType ScopeIterator::Type() const {
547568
if (InInnerScope()) {
548569
switch (current_scope_->scope_type()) {
549570
case FUNCTION_SCOPE:
550-
DCHECK_IMPLIES(NeedsContext(), context_->IsFunctionContext() ||
551-
context_->IsDebugEvaluateContext());
571+
DCHECK_IMPLIES(NeedsAndHasContext(),
572+
context_->IsFunctionContext() ||
573+
context_->IsDebugEvaluateContext());
552574
return ScopeTypeLocal;
553575
case MODULE_SCOPE:
554-
DCHECK_IMPLIES(NeedsContext(), context_->IsModuleContext());
576+
DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsModuleContext());
555577
return ScopeTypeModule;
556578
case SCRIPT_SCOPE:
557579
case REPL_MODE_SCOPE:
558-
DCHECK_IMPLIES(NeedsContext(), context_->IsScriptContext() ||
559-
IsNativeContext(*context_));
580+
DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsScriptContext() ||
581+
IsNativeContext(*context_));
560582
return ScopeTypeScript;
561583
case WITH_SCOPE:
562-
DCHECK_IMPLIES(NeedsContext(), context_->IsWithContext());
584+
DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsWithContext());
563585
return ScopeTypeWith;
564586
case CATCH_SCOPE:
565587
DCHECK(context_->IsCatchContext());
566588
return ScopeTypeCatch;
567589
case BLOCK_SCOPE:
568590
case CLASS_SCOPE:
569-
DCHECK_IMPLIES(NeedsContext(), context_->IsBlockContext());
591+
DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsBlockContext());
570592
return ScopeTypeBlock;
571593
case EVAL_SCOPE:
572-
DCHECK_IMPLIES(NeedsContext(), context_->IsEvalContext());
594+
DCHECK_IMPLIES(NeedsAndHasContext(), context_->IsEvalContext());
573595
return ScopeTypeEval;
574596
case SHADOW_REALM_SCOPE:
575-
DCHECK_IMPLIES(NeedsContext(), IsNativeContext(*context_));
597+
DCHECK_IMPLIES(NeedsAndHasContext(), IsNativeContext(*context_));
576598
// TODO(v8:11989): New ScopeType for ShadowRealms?
577599
return ScopeTypeScript;
578600
}
@@ -962,6 +984,12 @@ bool ScopeIterator::VisitLocals(const Visitor& visitor, Mode mode,
962984

963985
case VariableLocation::CONTEXT:
964986
if (mode == Mode::STACK) continue;
987+
if (!HasContext()) {
988+
// If the context was not yet pushed we report the variable as
989+
// unavailable.
990+
value = isolate_->factory()->the_hole_value();
991+
break;
992+
}
965993
DCHECK(var->IsContextSlot());
966994

967995
DCHECK_EQ(context_->scope_info()->ContextSlotIndex(var->name()), index);

deps/v8/src/debug/debug-scopes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ class V8_EXPORT_PRIVATE ScopeIterator {
106106
bool InInnerScope() const { return !function_.is_null(); }
107107
bool HasContext() const;
108108
bool NeedsContext() const;
109+
bool NeedsAndHasContext() const { return NeedsContext() && HasContext(); }
109110
Handle<Context> CurrentContext() const {
110111
DCHECK(HasContext());
111112
return context_;
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Don't crash when pausing on the iterator ".next" call
2+
function crashMe() {
3+
for (const #e of iter()) {
4+
() => e; // Context allocate e.
5+
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright 2025 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
const { session, Protocol, contextGroup } =
6+
InspectorTest.start('Don\'t crash when pausing on the iterator ".next" call');
7+
8+
session.setupScriptMap();
9+
contextGroup.addScript(`
10+
function *iter() {
11+
yield 1;
12+
debugger;
13+
yield 2;
14+
}
15+
16+
function crashMe() {
17+
for (const e of iter()) {
18+
() => e; // Context allocate e.
19+
}
20+
}
21+
`);
22+
23+
Protocol.Debugger.enable();
24+
25+
(async () => {
26+
let pausedPromise = Protocol.Debugger.oncePaused();
27+
Protocol.Runtime.evaluate({ expression: 'crashMe()' });
28+
29+
let { params: { callFrames } } = await pausedPromise;
30+
await session.logSourceLocation(callFrames[1].location);
31+
32+
Protocol.Debugger.resume();
33+
34+
InspectorTest.completeTest();
35+
})();

0 commit comments

Comments
 (0)