src: make realm binding data store weak #47688

legendecas · 2023-04-23T17:25:29Z

src: remove aliased buffer weak callback
An AliasedBuffer can be nested in a BaseObject and their weak callbacks
invoke order are not guaranteed. Prevent aliased buffer from being
accessed in its weak callback as its validness can be checked with the
emptiness of the persistent handle.

src: make realm binding data store weak
The binding data must be weak so that it won't keep the realm reachable
from strong GC roots indefinitely. The wrapper object of binding data
should be referenced from JavaScript, thus the binding data should be
reachable throughout the lifetime of the realm.

Fixes #47353

nodejs-github-bot · 2023-04-23T17:25:34Z

Review requested:

@nodejs/realm
@nodejs/startup
@nodejs/url

joyeecheung · 2023-04-24T15:44:04Z

src/node_shadow_realm.cc

+  // This is necessary to avoid cleaning up base objects before their scheduled
+  // weak callbacks are invoked, which can lead to accessing to v8 apis during
+  // the first pass of the weak callback.
+  realm->env()->SetImmediate([realm](Environment* env) { delete realm; });


I just remembered that technically shadow realms can outlive the environment...(which is why I added the if (env_ != nullptr) branch in the destructor of shadow realms, I think I ran into that before)

This is going to lead to crashes if the ream has access to any libuv handles within it. The problem is that any handles could emit data and ultimately call into JS.

IMHO the mechanism needs to be more sophisticated and we should track any native resource that the realm can spawn and let the ream be collected only after they have all been disposed.

Thanks for the ideas. Also, I noticed that the ASan build is failing for the URL binding data. I'm going to see how to address the failure too.

I've added a cleanup for ShadowRealms when the environment is being freed. This ensures that no shadow realms can outlive the lifetime of the environment. Similar to BaseObjects, no BaseObjects can outlive their creation realm (https://github.com/nodejs/node/blob/main/src/node_realm.cc#L27).

As for the ASan failure of the URL binding data, I've removed the aliased buffer's weak callback as the validness of the aliased buffer can be checked with the emptiness of the persistent handle.

Would you mind taking a look again? Thank you!

mcollina

lgtm

nodejs-github-bot · 2023-05-09T09:54:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51710/

src/env.cc

legendecas · 2023-05-22T16:46:09Z

@joyeecheung would you mind taking a look at this again? Thank you! <3

joyeecheung · 2023-06-06T08:24:33Z

src/env.cc

@@ -915,10 +928,6 @@ Environment::~Environment() {
      addon.Close();
    }
  }
-
-  for (auto realm : shadow_realms_) {
-    realm->OnEnvironmentDestruct();


Can you add a test that creates a bunch of shadow realms and then generates a heap snapshot? I think that's how I ran into the shadow realms outliving shadow realms issue and is why this is necessary.

Added a new test test/pummel/test-heapdump-shadow-realm.js.

nodejs-github-bot · 2023-06-08T08:47:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52160/

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle.

The binding data must be weak so that it won't keep the realm reachable from strong GC roots indefinitely. The wrapper object of binding data should be referenced from JavaScript, thus the binding data should be reachable throughout the lifetime of the realm.

legendecas · 2023-06-13T15:56:43Z

Rebased on the tip of the main branch to address CI coverage reports.

nodejs-github-bot · 2023-06-13T15:56:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52241/

nodejs-github-bot · 2023-06-13T17:54:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52242/

nodejs-github-bot · 2023-06-14T02:05:03Z

Landed in 8bc6e19...ac0853c

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle. PR-URL: #47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The binding data must be weak so that it won't keep the realm reachable from strong GC roots indefinitely. The wrapper object of binding data should be referenced from JavaScript, thus the binding data should be reachable throughout the lifetime of the realm. PR-URL: #47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle. PR-URL: #47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The binding data must be weak so that it won't keep the realm reachable from strong GC roots indefinitely. The wrapper object of binding data should be referenced from JavaScript, thus the binding data should be reachable throughout the lifetime of the realm. PR-URL: #47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle. PR-URL: nodejs#47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The binding data must be weak so that it won't keep the realm reachable from strong GC roots indefinitely. The wrapper object of binding data should be referenced from JavaScript, thus the binding data should be reachable throughout the lifetime of the realm. PR-URL: nodejs#47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle. PR-URL: nodejs#47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The binding data must be weak so that it won't keep the realm reachable from strong GC roots indefinitely. The wrapper object of binding data should be referenced from JavaScript, thus the binding data should be reachable throughout the lifetime of the realm. PR-URL: nodejs#47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

ruyadorno · 2023-09-08T18:13:21Z

These commits do not land cleanly on v18.x-staging and will need manual backport in case we want this to land on v18.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Apr 23, 2023

legendecas added the realm Issues and PRs related to the ShadowRealm API and node::Realm label Apr 23, 2023

legendecas marked this pull request as draft April 23, 2023 17:34

joyeecheung reviewed Apr 24, 2023

View reviewed changes

legendecas mentioned this pull request May 3, 2023

Node.js Technical Steering Committee (TSC) Meeting 2023-05-03 nodejs/TSC#1381

Closed

legendecas force-pushed the shadow-realm/binding-data branch 2 times, most recently from 2f14395 to eadbf52 Compare May 8, 2023 17:32

legendecas marked this pull request as ready for review May 8, 2023 17:42

mcollina approved these changes May 8, 2023

View reviewed changes

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023

jasnell reviewed May 10, 2023

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

joyeecheung reviewed Jun 6, 2023

View reviewed changes

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

legendecas added 2 commits June 13, 2023 17:23

src: remove aliased buffer weak callback

5aa1ab3

An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle.

legendecas force-pushed the shadow-realm/binding-data branch from 46ce6c0 to 952cabc Compare June 13, 2023 09:46

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2023

joyeecheung approved these changes Jun 13, 2023

View reviewed changes

github-actions bot mentioned this pull request Jun 14, 2023

CI Reliability 2023-06-14 nodejs/reliability#593

Open

20 tasks

legendecas added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jun 14, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 14, 2023

nodejs-github-bot closed this Jun 14, 2023

legendecas deleted the shadow-realm/binding-data branch June 14, 2023 02:28

github-actions bot mentioned this pull request Jun 15, 2023

CI Reliability 2023-06-15 nodejs/reliability#594

Open

22 tasks

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make realm binding data store weak #47688

src: make realm binding data store weak #47688

legendecas commented Apr 23, 2023 •

edited

Loading

nodejs-github-bot commented Apr 23, 2023

joyeecheung Apr 24, 2023 •

edited

Loading

mcollina Apr 24, 2023

legendecas Apr 25, 2023

legendecas May 8, 2023

mcollina left a comment

nodejs-github-bot commented May 9, 2023

legendecas commented May 22, 2023

joyeecheung Jun 6, 2023

legendecas Jun 8, 2023

nodejs-github-bot commented Jun 8, 2023

legendecas commented Jun 13, 2023

nodejs-github-bot commented Jun 13, 2023

nodejs-github-bot commented Jun 13, 2023

nodejs-github-bot commented Jun 14, 2023

ruyadorno commented Sep 8, 2023

src: make realm binding data store weak #47688

src: make realm binding data store weak #47688

Conversation

legendecas commented Apr 23, 2023 • edited Loading

nodejs-github-bot commented Apr 23, 2023

joyeecheung Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

mcollina Apr 24, 2023

Choose a reason for hiding this comment

legendecas Apr 25, 2023

Choose a reason for hiding this comment

legendecas May 8, 2023

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 9, 2023

legendecas commented May 22, 2023

joyeecheung Jun 6, 2023

Choose a reason for hiding this comment

legendecas Jun 8, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 8, 2023

legendecas commented Jun 13, 2023

nodejs-github-bot commented Jun 13, 2023

nodejs-github-bot commented Jun 13, 2023

nodejs-github-bot commented Jun 14, 2023

ruyadorno commented Sep 8, 2023

legendecas commented Apr 23, 2023 •

edited

Loading

joyeecheung Apr 24, 2023 •

edited

Loading