Skip to content

Conversation

@legendecas
Copy link
Member

@legendecaslegendecas 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-botnodejs-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
@legendecaslegendecas added the realm Issues and PRs related to the ShadowRealm API and node::Realm label Feb 24, 2023
Copy link
Member

@mcollinamcollina 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
MemberAuthor

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.

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@github-actionsgithub-actionsbot 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

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

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 

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2023
@github-actionsgithub-actionsbot 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
MemberAuthor

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]>
@legendecaslegendecas 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]>
This was referenced Apr 3, 2023
@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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.realmIssues 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

@legendecas@nodejs-github-bot@mcollina@RafaelGSS@jasnell@cjihrig@joyeecheung@danielleadams@aduh95