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.

Sign up for GitHub

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

Jump to bottom

test(core): Test that sync ops return/throw objects in the right realm #14750

Merged
merged 1 commit into from
May 30, 2022
Merged

test(core): Test that sync ops return/throw objects in the right realm #14750

merged 1 commit into from
May 30, 2022

Conversation

andreubotella
Copy link
Contributor

This behavior was introduced in #14019 but wasn't properly tested in that PR.

@andreubotella
test(core): Test that sync ops return/throw objects in the right realm
eb49d32 
This behavior was introduced in denoland#14019 but wasn't properly tested in
that PR.
bartlomieju
bartlomieju approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 01e5bba into denoland:main May 30, 2022
@andreubotella andreubotella deleted the test-realm-ops-objects branch May 31, 2022 11:36
bartlomieju pushed a commit that referenced this pull request Jun 2, 2022
@andreubotella @bartlomieju
test(core): Test that sync ops return/throw objects in the right realm (
6fe724c 
#14750)

This behavior was introduced in #14019 but wasn't properly tested in
that PR.
@andreubotella andreubotella mentioned this pull request Dec 14, 2022
Merged
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 14, 2022
@andreubotella
fix(core): Have custom errors be created in the right realm
8ac2b61 
Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change
made `js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than
in the current one.

This change adds back `ContextState`, adds back the `known_realms`
field of `JsRuntimeState` (needed to be able to drop the callback when
snapshotting), and also relands denoland#14750, which adds the
`js_realm_sync_ops` test removed in denoland#16366.
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 14, 2022
@andreubotella
fix(core): Have custom errors be created in the right realm
fc06281 
Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change
made `js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than
in the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop
the callback when snapshotting), and also relands denoland#14750, which adds
the `js_realm_sync_ops` test for this bug that was removed in denoland#16366.
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 17, 2022
@andreubotella
fix(core): Have custom errors be created in the right realm
7ab9bc3 
Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change
made `js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than
in the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop
the callback when snapshotting), and also relands denoland#14750, which adds
the `js_realm_sync_ops` test for this bug that was removed in denoland#16366.
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 17, 2022
@andreubotella
fix(core): Have custom errors be created in the right realm
fd7928d 
Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change
made `js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than
in the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop
the callback when snapshotting), and also relands denoland#14750, which adds
the `js_realm_sync_ops` test for this bug that was removed in denoland#16366.
bartlomieju pushed a commit that referenced this pull request Dec 20, 2022
@andreubotella
fix(core): Have custom errors be created in the right realm (#17050)
fcca54d 
Although PR #16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in #14734. This change made
`js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than in
the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop the
callback when snapshotting), and also relands #14750, which adds the
`js_realm_sync_ops` test for this bug that was removed in #16366.
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Dec 22, 2022
@andreubotella @bartlomieju
fix(core): Have custom errors be created in the right realm (denoland…
4bb8925 
…#17050)

Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change made
`js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than in
the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop the
callback when snapshotting), and also relands denoland#14750, which adds the
`js_realm_sync_ops` test for this bug that was removed in denoland#16366.
@andreubotella andreubotella mentioned this pull request Dec 28, 2022
Open
13 tasks
bartlomieju pushed a commit that referenced this pull request Jan 5, 2023
@andreubotella @bartlomieju
fix(core): Have custom errors be created in the right realm (#17050)
175f973 
Although PR #16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in #14734. This change made
`js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than in
the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop the
callback when snapshotting), and also relands #14750, which adds the
`js_realm_sync_ops` test for this bug that was removed in #16366.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@bartlomieju bartlomieju bartlomieju approved these changes

@AaronO AaronO Awaiting requested review from AaronO

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andreubotella @cjihrig @bartlomieju