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

src: bootstrap Web [Exposed=*] APIs in the shadow realm #46809

Closed
wants to merge 3 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Feb 24, 2023

This is the initial work to bootstrap Web interfaces that are defined
with extended attributes [Exposed=*].

The ShadowRealm instances are garbage-collected once it is
unreachable. However, V8 can not infer the reference cycles between
the per-realm strong persistent function handles and the realm's
context handle. To allow the context to be gc-ed once it is not
reachable, the per-realm persistent handles are attached to the
context's global object and the persistent handles are set as weak.

Refs: #42528

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

Review requested:

  • @nodejs/startup
  • @nodejs/realm

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 24, 2023
@legendecas legendecas added the realm Issues and PRs related to the ShadowRealm API and node::Realm label Feb 24, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

// TODO(legendecas): Per-realm prepareStackTrace callback.
// If we are in a Realm that is not the principal Realm (e.g. ShadowRealm),
// skip the prepareStackTrace callback as the context's security token is
// likely to be different.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, why do we need different security tokens? Can we just set the non-principal realm's security token to be the same as the one of the principal realm, like what we do for vm contexts? IIUC security tokens in browsers are generally intended for cross-origin global proxies, which isn't really a thing for Node.js. For shadow realms, the cross-realm access is guarded with wrapped functions and callable boundaries. For Node.js realms, do we care about cross-realm object access?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is true that object exchange across the ShadowRealm boundary is limited with wrapped functions. However, it is still possible for host hooks like the prepareStackTrace callback here to leak objects to the principal realm's userland Error.prepareStackTrace override.

It doesn't hurt to set the security token to be the principal realm's. But I don't see the reason to allow the access either.

I've updated the comment to point out that this branch is intended to avoid calling the principal realm's Error.prepareStackTrace override instead of the security token mismatches.

test/parallel/test-shadow-realm-globals.js Outdated Show resolved Hide resolved
test/common/globals.js Outdated Show resolved Hide resolved
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

src/node_realm.cc Outdated Show resolved Hide resolved
src/node_realm-inl.h Outdated Show resolved Hide resolved
src/node_realm-inl.h Outdated Show resolved Hide resolved
src/node_realm-inl.h Outdated Show resolved Hide resolved
src/node_realm-inl.h Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

mcollina commented Mar 6, 2023

CI is has a relevant test failure:

node:assert:125
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ [
+   'BroadcastChannel'
+ ]
- []
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/test/parallel/test-shadow-realm-globals.js:27:8)
    at Module._compile (node:internal/modules/cjs/loader:1287:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1341:10)
    at Module.load (node:internal/modules/cjs/loader:1145:32)
    at Module._load (node:internal/modules/cjs/loader:984:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [ 'BroadcastChannel' ],
  expected: [],
  operator: 'deepStrictEqual'
}

Node.js v20.0.0-pre

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

Landed in d0153ae...e6b4d30

legendecas added a commit that referenced this pull request Mar 15, 2023
PR-URL: #46809
Refs: #42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
legendecas added a commit that referenced this pull request Mar 15, 2023
This is the initial work to bootstrap Web interfaces that are defined
with extended attributes `[Exposed=*]`.

The ShadowRealm instances are garbage-collected once it is
unreachable. However, V8 can not infer the reference cycles between
the per-realm strong persistent function handles and the realm's
context handle. To allow the context to be gc-ed once it is not
reachable, the per-realm persistent handles are attached to the
context's global object and the persistent handles are set as weak.

PR-URL: #46809
Refs: #42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@legendecas legendecas closed this Mar 15, 2023
@legendecas legendecas deleted the shadowrealm/builtin branch March 15, 2023 16:32
targos pushed a commit that referenced this pull request Mar 18, 2023
PR-URL: #46809
Refs: #42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Mar 18, 2023
This is the initial work to bootstrap Web interfaces that are defined
with extended attributes `[Exposed=*]`.

The ShadowRealm instances are garbage-collected once it is
unreachable. However, V8 can not infer the reference cycles between
the per-realm strong persistent function handles and the realm's
context handle. To allow the context to be gc-ed once it is not
reachable, the per-realm persistent handles are attached to the
context's global object and the persistent handles are set as weak.

PR-URL: #46809
Refs: #42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@RafaelGSS
Copy link
Member

This is breaking v19.x-staging. Could you please create a manual backport? See: #47441 (comment)

legendecas added a commit to legendecas/node that referenced this pull request Apr 12, 2023
PR-URL: nodejs#46809
Refs: nodejs#42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
legendecas added a commit to legendecas/node that referenced this pull request Apr 12, 2023
This is the initial work to bootstrap Web interfaces that are defined
with extended attributes `[Exposed=*]`.

The ShadowRealm instances are garbage-collected once it is
unreachable. However, V8 can not infer the reference cycles between
the per-realm strong persistent function handles and the realm's
context handle. To allow the context to be gc-ed once it is not
reachable, the per-realm persistent handles are attached to the
context's global object and the persistent handles are set as weak.

PR-URL: nodejs#46809
Refs: nodejs#42528
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. realm Issues and PRs related to the ShadowRealm API and node::Realm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants