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

refactor(core): Move optional callbacks from JsRuntimeState to ContextState #17422

Conversation

andreubotella
Copy link
Contributor

The JsRuntimeState struct stores a number of JS callbacks that are used either in the event loop or when interacting with V8. Some of these callback fields are vectors of callbacks, and therefore could plausibly store at least one callback per realm. However, some of those fields are Option<v8::Global<v8::Function>>, which would make the callbacks set by a realm override the one that might have been set by a different realm.

As it turns out, all of the current such optional callbacks (js_promise_reject_cb, js_format_exception_cb and js_wasm_streaming_cb) are only used from inside a realm, and therefore this change makes it so such callbacks can only be set from inside a realm, and will only affect that realm.

This is a reland of #15599.

Towards #13239.

…textState`

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of
these callback fields are vectors of callbacks, and therefore could
plausibly store at least one callback per realm. However, some of
those fields are `Option<v8::Global<v8::Function>>`, which would make
the callbacks set by a realm override the one that might have been set
by a different realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and
therefore this change makes it so such callbacks can only be set from
inside a realm, and will only affect that realm.

This is a reland of denoland#15599.

Towards denoland#13239.
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@littledivy littledivy merged commit 05ef925 into denoland:main Jan 15, 2023
@andreubotella andreubotella deleted the realms-reland-move-callbacks-to-context-state branch January 15, 2023 03:39
bartlomieju pushed a commit that referenced this pull request Jan 16, 2023
…textState` (#17422)

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of these
callback fields are vectors of callbacks, and therefore could plausibly
store at least one callback per realm. However, some of those fields are
`Option<v8::Global<v8::Function>>`, which would make the callbacks set
by a realm override the one that might have been set by a different
realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and therefore
this change makes it so such callbacks can only be set from inside a
realm, and will only affect that realm.

This is a reland of #15599.

Towards #13239.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants