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

Wrapping or unwrapping already-wrapped functions #343

Closed
caitp opened this issue Dec 14, 2021 · 15 comments
Closed

Wrapping or unwrapping already-wrapped functions #343

caitp opened this issue Dec 14, 2021 · 15 comments

Comments

@caitp
Copy link

caitp commented Dec 14, 2021

Suppose you have the following scenario:

let r = new ShadowRealm;
let wrapped1 = r.evaluate("(function(a, b) { return a(b); })");
let wrapped2 = r.evaluate("(function() { return globalThis.specialSauce++; })"

let result = wrapped1(wrapped2, 7);

Invoking wrapped1 results in re-wrapping the already wrapped wrapped2, but did any wrapping really need to happen? Would it make sense to simply pass the unwrapped target function as a parameter?

This difference would be observable, but I don't think it enables an avenue of cross-realm information sharing, and it could provide some heap savings

@caitp
Copy link
Author

caitp commented Dec 14, 2021

Also, multiply-wrapped functions can be a pain to query, so it might be nice if we could guarantee that a wrapped function is at most singly wrapped

@mhofman
Copy link
Member

mhofman commented Dec 14, 2021

Besides the identity observability, which is better served by symbols, what would be the benefit to specifying an unwrapping or limiting the number of wrapping? The current spec doesn't prevent implementations from optimizing the calls of multi-wrapped functions and in effect flatten invocations.

See #293 for previous discussion on the topic.

@caitp
Copy link
Author

caitp commented Dec 14, 2021

I do believe identity observability of functions created within the same shadow realm is more intuitive and easier to understand, and simpler to work with, for users.

But that isn't really the point, on the runtime side, I'm not a huge fan of allocating a new function wrapper every time you want to pass a function returned from a shadow realm, as an argument to a function within that shadow realm. I'm of the opinion that wrapping should only be done for cross-realm operations, but when passed a wrapper of a function originating in the same shadow realm, adding another layer of indirection doesn't make sense

@ljharb
Copy link
Member

ljharb commented Dec 14, 2021

Can't the wrapper have properties added in the new realm?

@caitp
Copy link
Author

caitp commented Dec 14, 2021

Can't the wrapper have properties added in the new realm?

ShadowRealm provides a function to the Incubator realm (this is wrapped, the incubator realm can't operate on the inner function)
Incubator realm passes wrapped function back to ShadowRealm as an argument (This is unwrapped, the ShadowRealm doesn't see anything added to the wrapper object by the IncubatorRealm)

I don't see a way for properties on the wrapper to be exposed to the ShadowRealm, or properties on the original object to be exposed to the incubator realm or different ShadowRealm -- I think, as far as each realm is concerned, they all are operating with distinct objects representing the same underlying function, and there's no real mechanism for sharing information with it --- but as far as one realm is concerned, a reference to that realm's functions always matches the identity of that realm's functions

@ljharb
Copy link
Member

ljharb commented Dec 14, 2021

ok - so currently, if i have function F in the main realm, and i pass it into the shadow realm, and the shadow realm passes it back as G, and another round trip gives me H. F !== G && F !== H && G !== H.

With your proposal, would G === H?

@mhofman
Copy link
Member

mhofman commented Dec 14, 2021

Identity stability of function through the boundary is not a design goal of this API, and I believe the champions would be against it.

In particular passing the same function twice from realm A -> B would result in 2 different wrappers, and doing otherwise would require the implementation to keep a WeakMap of callables and their wrapper. Since there is already identity instability in that case, there is no reason to have identity stability in round trips.

@caitp
Copy link
Author

caitp commented Dec 14, 2021

If I understood your example:

let r = new ShadowRealm;
let wrap = r.evaluate("(function(f) { return f; })");

let f = function() {}
let g = wrap(f);
let h = wrap(g);

// f == g == h

@mhofman
Copy link
Member

mhofman commented Dec 14, 2021

My point is that you already have the following:

let f = function() {};
let r = new ShadowRealm;
const g = r.evaluate("(function(f1, f2) { return f1 === f2; })");

assert(g(f, f) === false);

@ljharb
Copy link
Member

ljharb commented Dec 14, 2021

@caitp and here:

let r = new ShadowRealm;
let wrap = r.evaluate("globalThis.cache ||= new Set(); (function(f) { globalThis.cache.add(f); return f; })");

let f = function() {}
let g = wrap(f);
let h = wrap(g);

r.evaluate('globalThis.cache.size'); // ?

what would be the result?

@caitp
Copy link
Author

caitp commented Dec 14, 2021

r.evaluate('globalThis.cache.size'); // ?

My thing here is concerned with the way things are treated which originate from and are returned to a particular realm --- those would preserve their identity, but wrappers would still be created when crossing the realm boundary, so the globalThis cache would have 2 items in it at the end, and you'd really want it to be a WeakMap 👀

@caitp
Copy link
Author

caitp commented Dec 14, 2021

My point is that you already have the following:

let f = function() {};
let r = new ShadowRealm;
const g = r.evaluate("(function(f1, f2) { return f1 === f2; })");

assert(g(f, f) === false);

I think this would still be the case with the idea I presented here, this doesn't change

@ljharb
Copy link
Member

ljharb commented Dec 14, 2021

WeakMaps don't have a size, otherwise i'd have used that :-)

@caitp
Copy link
Author

caitp commented Dec 14, 2021

so, it would enable a level of querying of the unwrapped identity of an object that wouldn't be possible currently, I guess there are some security implications. They do seem like the sorts of things which have other solutions though

@caridy
Copy link
Collaborator

caridy commented Feb 5, 2024

We have discussed the identity of wrapped functions extensibly in various forms. I will close this assuming that there are no more open questions.

@caridy caridy closed this as completed Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants