Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

JsRuntime::create_realm should be a static method #15293

Closed
andreubotella opened this issue Jul 24, 2022 · 5 comments · May be fixed by #16211
Closed

JsRuntime::create_realm should be a static method #15293

andreubotella opened this issue Jul 24, 2022 · 5 comments · May be fixed by #16211
Labels
deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)

Comments

@andreubotella
Copy link
Contributor

The JsRuntime::create_realm method, introduced, in #14019, made it possible to create V8 contexts that had the same set of extensions and ops loaded, and it has since been used to write tests for deno_core's realm support. However, this method takes a &mut self, so it can only be called from code embedding the Deno runtime, as opposed to from the runtime itself.

Given that the whole intention of having deno_core support realms was to make it possible to implement the upcoming ShadowRealm TC39 proposal (see #13239), it must be possible to create such a realm from a callback passed to V8.

Making this method static would require making JsRuntime::init_extension_js and JsRuntime::init_cbs static as well, plus probably moving the extension field from JsRuntime to JsRuntimeState.

@andreubotella
Copy link
Contributor Author

As I was trying this out, I realized that since JsRuntime::handle_scope and JsRealm::handle_scope create a brand new scope rather than adding to the existing scope stack, JsRealm::execute_script can't be used in a V8 callback since there's an existing scope stack. This means that you can't really run initialization code inside the context from the ShadowRealm constructor – or at least not without building on the existing scopes.

@kitsonk kitsonk added deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted) labels Aug 1, 2022
andreubotella added a commit to andreubotella/deno that referenced this issue Oct 8, 2022
@andreubotella
Copy link
Contributor Author

andreubotella commented Oct 8, 2022

I either made such methods static, or moved them to JsRealm, in #16211. But this seems not to run into the issue I mentioned in the second comment, even though I'm pretty sure it used to when I tried months ago.

Edit: Never mind, it does run into that issue, but JsRealm::execute_script only runs on realm creation when the isolate hasn't been built from a snapshot, so you don't end up seeing this in Deno CLI builds (which was what I was first using for testing), but you do see it in deno_core's unit tests.

andreubotella added a commit to andreubotella/deno that referenced this issue Oct 20, 2022
@bartlomieju
Copy link
Member

@andreubotella is this something you still want to pursue?

@andreubotella
Copy link
Contributor Author

@andreubotella is this something you still want to pursue?

I'm not sure at the moment. I'll try rebasing #16211 to see if it can be done without this.

@andreubotella
Copy link
Contributor Author

Closing in favour of denoland/deno_core#101.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants