GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: migrate to script context based host defined options by legendecas · Pull Request #44923 · nodejs/node

Conversation

@legendecas

@legendecas legendecas commented Oct 8, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This is not ready to be reviewed properly. Just a preview of the solution.

deps: v8: [runtime] Store host-defined options in the context

CR-URL: https://chromium-review.googlesource.com/c/v8/v8/+/3172764

module: migrate to script context based host defined options

This allows the host-defined options to be an arbitrary JavaScript value
so that the module/script wrapper object can be used as the host-defined
options instead. In this way, we can move away from the id-based module
tables and fixes the problem that the scripts can out lives the module
wrapper objects.

Fixes: #43681
Fixes: #43205
Fixes: #44438

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
viceice, SimenB, mopatches, chentsulin, anajavi, styfle, SimonSiefke, dead, andriyor, and rezonant reacted with heart emoji bricss, chentsulin, ywave620, janecakemaster, dead, hildjj, and andriyor reacted with eyes emoji
All reactions
  • ❤️ 10 reactions
  • 👀 7 reactions
@nodejs-github-bot

nodejs-github-bot commented Oct 8, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/v8-update
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Oct 8, 2022
@legendecas legendecas mentioned this pull request Oct 8, 2022
vm: memory leak on vm.compileFunction with importModuleDynamically #44211
Closed
@SimenB

SimenB commented Oct 11, 2022

Copy link
Copy Markdown
Member

I'm beyond happy to see movement on this issue more than a year and a half after it was pointed to as a blocker of stabilization of vm ESM 🎉
chentsulin, mcrawshaw, robhogan, sutt0n, and andriyor reacted with thumbs up emoji
All reactions
  • 👍 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ywave620

ywave620 commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

Can this PR get landed before the v8 team decides to adopt your change? @legendecas
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

legendecas commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

@ywave620 that's not in accord with the working process. I don't think we should land this before the v8 CL gets landed first.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Nov 8, 2022

Copy link
Copy Markdown
Member

@legendecas What's the status on the V8 side?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

legendecas commented Nov 9, 2022

Copy link
Copy Markdown
Member Author

@targos thanks for the ping. I'll reach out to Camillo to see the next steps.
isker, targos, mcrawshaw, sutt0n, hildjj, andriyor, and rezonant reacted with heart emoji
All reactions
  • ❤️ 7 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@dnalborczyk dnalborczyk mentioned this pull request Nov 14, 2022
Import of data url leads to crash #45449
Closed
legendecas added 3 commits November 24, 2022 01:21
@legendecas
deps: v8: [runtime] Store host-defined options in the context
6582d98 
CR-URL: https://chromium-review.googlesource.com/c/v8/v8/+/3172764
@legendecas
deps: v8: record dynamic import call without import assertsions
d731a0e
@legendecas
module: migrate to script context based host defined options
438df3b 
This allows the host-defined options to be an arbitrary JavaScript value
so that the module/script wrapper object can be used as the host-defined
options instead. In this way, we can move away from the id-based module
tables and fixes the problem that the scripts can out lives the module
wrapper objects.
@legendecas
legendecas force-pushed the vm/host-defined-options branch from 54f0d90 to 438df3b Compare November 23, 2022 17:29
@legendecas legendecas closed this Oct 23, 2023
@legendecas

legendecas commented Oct 23, 2023

Copy link
Copy Markdown
Member Author

#48510 fixed the crashes with an alternative approach.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SIGSEGV Segmentation fault when loading data URIs using import await Promise.resolve("import('./mod.mjs')").then(eval) rejects/segfaults in REPL Segmentation fault when executing worker with eval: true containing dynamic import after await

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@legendecas @nodejs-github-bot @SimenB @ywave620 @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.