src: make realm binding data store weak#47688
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Apr 23, 2023
Review requested:
|
nodejs-github-bot added c++ 
legendecas added the realm | // 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}); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
legendecasforce-pushed the shadow-realm/binding-data branch 2 times, most recently from 2f14395 to eadbf52Compare
github-actionsbot removed the request-ci nodejs-github-bot commented May 9, 2023
Uh oh!
There was an error while loading. Please reload this page.
legendecas commented May 22, 2023
@joyeecheung would you mind taking a look at this again? Thank you! <3 |
| } | ||
| for (auto realm : shadow_realms_){ | ||
| realm->OnEnvironmentDestruct(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added a new test test/pummel/test-heapdump-shadow-realm.js.
github-actionsbot removed the request-ci nodejs-github-bot commented Jun 8, 2023
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.
legendecasforce-pushed the shadow-realm/binding-data branch from 46ce6c0 to 952cabcComparegithub-actionsbot removed the request-ci legendecas commented Jun 13, 2023
Rebased on the tip of the main branch to address CI coverage reports. |
nodejs-github-bot commented Jun 13, 2023
nodejs-github-bot commented Jun 13, 2023
legendecas added commit-queue nodejs-github-bot removed the commit-queue nodejs-github-bot commented Jun 14, 2023
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
ruyadorno commented Sep 8, 2023
These commits do not land cleanly on |
6 participants
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