ajvincent / es-membrane

An ECMAScript implementation of a Membrane, allowing users to dynamically hide, override, or extend objects in JavaScript with controlled effects on the original objects.
ISC License
109 stars 13 forks source link

Possible memory leak of proxies by holding strong references to revoke functions? #237

Open ajvincent opened 3 years ago

ajvincent commented 3 years ago

The current ObjectGraphHandler code holds strong references to revoke() functions that Proxy.revocable() provides. I'm no longer sure that's a good idea. On the one hand, it definitely makes it easy to revoke an entire object graph in one action. On the other hand, that's potentially a lot of revoke functions to hold and if they internally hold strong references to their adjoining proxy, that's not great.

An alternative would be to adjust the ProxyHandler to watch a internal "I am dead" property, and store references from proxy to revoker in a WeakMap. Then, at the start of the ProxyHandler's traps, if the handler considers itself dead, it can immediately revoke the proxy, and reflect back a static revoked proxy's matching trap, throwing the exception that would normally throw for a revoked proxy.

@erights, am I right here to worry about revoke functions being a memory leak for a membrane?

const revokers = [];
{
  const {revoke} = Proxy.revocable({}, Reflect);
  revokers.push(revoke); // does this mean the proxy leaks?
}

https://262.ecma-international.org/11.0/#sec-proxy.revocable

ajvincent commented 3 years ago

A real-world membrane isn't supposed to hold strong references to the proxies. So while the above example initially seems contrived, there's the argument that if the revoker is the only thing left after other references have been dropped, then the proxy might remain alive. That's my question.

ajvincent commented 3 years ago

In the esmodules-0.10 branch, I have forcibly moved revokers into a "revocable multimap" (source/core/RevocableMultimap.mjs), and that is the only place they're kept. This means the memory leak is strictly the responsibility of the underlying engines, although it's worth auditing to see if custom revokers (specifically, those I create not from Proxy.revocable()) hold strong references.