Skip to content

Conversation

@juanarbol
Copy link
Member

@juanarbol juanarbol commented Nov 7, 2025

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=[email protected]

Fixed: 384413079,399002824
Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
Reviewed-by: Leszek Swirski [email protected]
Commit-Queue: Simon Zünd [email protected]
Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: #60580
PR-URL: #60620

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2025

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Nov 7, 2025
juanarbol added a commit to juanarbol/node that referenced this pull request Nov 7, 2025
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'.

  [email protected]

  Fixed: 384413079,399002824
  Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
  Reviewed-by: Leszek Swirski <[email protected]>
  Commit-Queue: Simon Zünd <[email protected]>
  Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: nodejs#60580
PR-URL: nodejs#60620
Signed-off-by: Juan José Arboleda <[email protected]>
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like a cherry-pick of v8/v8@5ba9200, which should already be on main anyway since it landed in V8 13.8

@juanarbol
Copy link
Member Author

This doesn't look like a cherry-pick of v8/v8@5ba9200, which should already be on main anyway since it landed in V8 13.8

Whoops, that's right, wrong base branch.

@juanarbol juanarbol requested a review from a team as a code owner November 7, 2025 16:52
@juanarbol juanarbol changed the base branch from main to v24.x-staging November 7, 2025 16:53
juanarbol added a commit to juanarbol/node that referenced this pull request Nov 7, 2025
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'.

  [email protected]

  Fixed: 384413079,399002824
  Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
  Reviewed-by: Leszek Swirski <[email protected]>
  Commit-Queue: Simon Zünd <[email protected]>
  Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: nodejs#60580
PR-URL: nodejs#60620
Signed-off-by: Juan José Arboleda <[email protected]>
@targos
Copy link
Member

targos commented Nov 7, 2025

Still not a cherry-pick. I highly recommend you use @node-core/utils to prepare it: https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-v8-backport-sha

juanarbol added a commit to juanarbol/node that referenced this pull request Nov 7, 2025
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'.

  [email protected]

  Fixed: 384413079,399002824
  Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
  Reviewed-by: Leszek Swirski <[email protected]>
  Commit-Queue: Simon Zünd <[email protected]>
  Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: nodejs#60580
PR-URL: nodejs#60620
Signed-off-by: Juan José Arboleda <[email protected]>
juanarbol added a commit to juanarbol/node that referenced this pull request Nov 7, 2025
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'.

  [email protected]

  Fixed: 384413079,399002824
  Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
  Reviewed-by: Leszek Swirski <[email protected]>
  Commit-Queue: Simon Zünd <[email protected]>
  Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: nodejs#60580
PR-URL: nodejs#60620
Signed-off-by: Juan José Arboleda <[email protected]>
@juanarbol
Copy link
Member Author

Still not a cherry-pick. I highly recommend you use @node-core/utils to prepare it: https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-v8-backport-sha

yes yes, I'm just a mess with git! Thanks!

Also, is it possible to send my "fork" of v8 to node core utils? I don't want to fetch that again, I've my own tree.

Seems ready now, sorry for the noise.

@juanarbol juanarbol added the v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch. label Nov 7, 2025
@targos
Copy link
Member

targos commented Nov 7, 2025

Also, is it possible to send my "fork" of v8 to node core utils?

Yes, with --v8-dir=path/to/your/fork

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'.

    [email protected]

    Fixed: 384413079,399002824
    Change-Id: Ia2d0008d574e7eaf6c06b640053df696014d37f8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6507402
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Simon Zünd <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#100029}

Refs: v8/v8@5ba9200
Fixes: nodejs#60580
PR-URL: nodejs#60620
Signed-off-by: Juan José Arboleda <[email protected]>
@juanarbol
Copy link
Member Author

Yes, with --v8-dir=path/to/your/fork

Thanks! PTAL :)

@aduh95
Copy link
Contributor

aduh95 commented Nov 7, 2025

$ curl -L https://github.com/v8/v8/commit/5ba9200cd04602c486ff5a5fc4f9e0d4ab19606b.patch | head -3
From 5ba9200cd04602c486ff5a5fc4f9e0d4ab19606b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simon=20Z=C3=BCnd?= <[email protected]>
Date: Fri, 2 May 2025 10:44:42 +0000

$ curl -L https://github.com/nodejs/node/pull/60620/commits/d8e7ab5d77e85ff0257314acfd43546ae1a4ff2f.patch | head -3
From d8e7ab5d77e85ff0257314acfd43546ae1a4ff2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Juan=20Jos=C3=A9=20Arboleda?= <[email protected]>
Date: Fri, 7 Nov 2025 14:26:58 -0500

Is it expected that the date and author do not match?

@juanarbol
Copy link
Member Author

Is it expected that the date and author do not match?

Yes, it is expected

replace yourself as the author. `git commit --amend --reset-author`. You may

@richardlau richardlau changed the title deps: cherry-pick 5ba9200 from V8 upstream [v24.x] deps: cherry-pick 5ba9200 from V8 upstream Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NodeJS segfaults when trying to debug this code

4 participants