Skip to content

deps: V8: cherry-pick beebee4f80ff #37293

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

santigimeno
Copy link
Member

It contains the following commits:

deps: V8: cherry-pick beebee4f80ff

Original commit message:

cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.
    
This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}

Refs: v8/v8@beebee4

test: add cpu-profiler-crash test

This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

@santigimeno santigimeno self-assigned this Feb 9, 2021
@nodejs-github-bot nodejs-github-bot added v14.x v8 engine Issues and PRs related to the V8 dependency. labels Feb 9, 2021
@santigimeno santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from cde372c to af26f41 Compare February 9, 2021 15:12
@richardlau
Copy link
Member

Can you target the staging branch (v14.x-staging)? Does this also apply to 15.x/master?

@santigimeno santigimeno changed the base branch from v14.x to v14.x-staging February 9, 2021 16:26
@santigimeno
Copy link
Member Author

santigimeno commented Feb 9, 2021

Can you target the staging branch (v14.x-staging)?

Done

Does this also apply to 15.x/master?

No, the fix is already there though it would be great if it could be applied to 12.x at some point.

Thanks!

@richardlau

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

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

You forget update common.gypi in first commit.

@santigimeno santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from af26f41 to f582bd8 Compare February 21, 2021 19:57
@santigimeno
Copy link
Member Author

@gengjiawen thanks for pointing that out! Just updated the PR.

@juanarbol
Copy link
Member

I could work on the v12.x backport

psmarshall and others added 2 commits March 1, 2021 10:42
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4
@santigimeno santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from f582bd8 to 7908e99 Compare March 1, 2021 09:42
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2021

@nodejs-github-bot
Copy link
Collaborator

richardlau pushed a commit that referenced this pull request Mar 2, 2021
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 2, 2021
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@richardlau
Copy link
Member

Landed in 292dffe...69057eb.

@richardlau richardlau closed this Mar 2, 2021
@santigimeno santigimeno deleted the santi/backport_v8_10778_bugfix branch March 2, 2021 17:23
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: nodejs#37293
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: nodejs#37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: nodejs#37293
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: nodejs#37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 16, 2021
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Backport-PR-URL: #37578
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 16, 2021
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Backport-PR-URL: #37578
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@richardlau richardlau mentioned this pull request Mar 18, 2021
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants