tc39 / proposal-shadowrealm

ECMAScript Proposal, specs, and reference implementation for Realms
https://tc39.es/proposal-shadowrealm/
1.43k stars 67 forks source link

[DON'T MERGE THIS] Normative: Allow transferring SharedArrayBuffer objects #336

Closed leobalter closed 2 years ago

leobalter commented 2 years ago

This PR is to explore the possibility of allowing SharedArrayBuffers to be transferred cross shadow realms as discussed internally by @rwaldron and @caridy. They might expand the use cases and we also have #298 and #322.

The feature only accounts for the buffer data and size, reusing the intrinsic SharedArrayBuffer. Object properties are not transferred here.

This code example quickly shows up it is expected to work:

const r = new ShadowRealm();
const sab = new SharedArrayBuffer(8);
const getFoo = r.evaluate('(clonedSab) => clonedSab.foo');

sab.foo = 2;

// The ShadowRealm creates a new SAB reusing the already allocated sharedarraybuffer data.
getFoo(sab); // undefined

const cloneBack = r.evaluate(`
  (clonedSab) => {
    clonedSab[0] = 1;
    return clonedSab;
  };
`);

const otherSab = cloneBack(sab);

otherSab === sab; // false;
sab[0]; // 1
otherSab[0]; // 1

This might be considered as a Stage 3 change or a follow up proposal.

mhofman commented 2 years ago

Thinking about this, this introduces a mechanism in ECMA262 to create a new SAB object from an existing data block (clone seem to have the semantics of copy in the spec), where before this was a capability left to hosts.

leobalter commented 2 years ago

as additional info, the main difference here from the Structured Cloning, this proposed change ignore any additional aspects of the sab objects like extra properties or subclassing.

Jack-Works commented 2 years ago

I think this is interesting and hoping to see it ship at the first release. But isn't it a big change after stage 3?

leobalter commented 2 years ago

I intend to present this at the next TC39 meeting. I'd be fine with it being a stage 3 change but we need to have all the delegates onboard with the motivation and proposed solution. I believe some delegates might want more time to think through so a follow up proposal seems pragmatic.

leobalter commented 2 years ago

f2f487e avoids the noise from an abstraction I previously introduced and removes unnecessary steps to clone the SharedArrayBuffer and avoids the confusion of using any intrinsic name inside a string.

Jamesernator commented 2 years ago

I intend to present this at the next TC39 meeting. I'd be fine with it being a stage 3 change but we need to have all the delegates onboard with the motivation and proposed solution. I believe some delegates might want more time to think through so a follow up proposal seems pragmatic.

Post-MVP should be fine, although still definitely raising it with the TC39 earlier in case it changes how implementors implement the rest of the proposal.

annevk commented 2 years ago

I have two concerns:

  1. Why does this not reuse serialize and deserialize semantics? Especially as those will be defined by JS, this discrepancy could turn out to be odd in hindsight.
  2. This syntax does not really allow for the transfer semantics you might want for non-shared buffers.
leobalter commented 2 years ago

We identified objections for the full serialization.

The shared array buffer is the only thing necessary for resolving the use cases we identified to motivate this PR.

Keep in mind, we intend to propose this change as a follow up proposal to avoid blocking ShadowRealms.

annevk commented 2 years ago

I would love to learn more, but if this becomes a separate (follow-up) proposal that goes through the stages there's no rush.

leobalter commented 2 years ago

@annevk sure! I'll make sure we keep this in touch with you!

@rwaldron and @caridy might present this to the TC39 meeting next week and identify the next steps.

I understand there are some contention points to smooth out here and we plan to work through all of it.

caridy commented 2 years ago

I have two concerns:

  1. Why does this not reuse serialize and deserialize semantics? Especially as those will be defined by JS, this discrepancy could turn out to be odd in hindsight.

@annevk one of the big issues with serialize is that it does not support all the features of the language. E.g.: Proxies. We are certainly looking at serialization, and currently looking at explicit vs implicit serialization mechanism, but for this to get any traction, we will need to figure how to reform the serialize mechanism to support all features of the language.

annevk commented 2 years ago

Maybe, but serialization exists today. Adding a new mechanism that does roughly the same thing, but not exactly, does not seem like a step in the right direction.

mhofman commented 2 years ago

@annevk, I believe that @caridy's point is that structure clone as specified by HTML would face objections to be exposed as-is by JavaScript. While moving the algorithm definition of serialize/deserialize is considered for inclusion in ecma262 for maintenance reasons (https://github.com/tc39/ecma262/issues/2555), these operations would likely not be used anywhere in the JavaScript spec with their current definition, in particular because they allow a program to directly test whether an object is a proxy.

While I'm hopeful we can find a cloning mechanism that is both backwards compatible with structured clone, and satisfies the current objections to structure clone being included in JavaScript, this is a much larger project. Because of that we have to start somewhere that allows us to "transmit" a subset of objects through the callable boundary, either explicitly or implicitly, while not painting ourselves in a corner regarding any future serialization/clone mechanism in JavaScript.

I am personally in favor of an explicit cloning mechanism, exactly because it's unclear how an implicit mechanism could be extended to non-shareable objects. However I also admit that the only concern here is future discrepancy where some specific objects are allowed implicitly and some need explicit handling. That wouldn't be a fatal flaw, just odd as you point out.

erights commented 2 years ago

Slight correction to what @mhofman said:

considered for inclusion in ecma262

should be "considered for moving under tc39".

Given the problems explained in the https://github.com/tc39/ecma262/issues/2555 thread, I do not consider it a candidate for inclusion in ecma262. It is too inconsistent with the JavaScript language. However, tc39 should maintain it as a separate report, likely by a separate TG.

annevk commented 2 years ago

@mhofman this would also face objections, for reasons stated above.

mhofman commented 2 years ago

@annevk, could you clarify exactly what would face objections?

annevk commented 2 years ago

Introducing an operation to move objects across "isolated" realms that's incompatible with StructuredSerialize and StructuredDeserialize. Changing those algorithms to accommodate more use cases is an option though. Although webcompat might put limits on that.

Other than proxies I didn't see a lot in https://github.com/tc39/ecma262/issues/2555 btw. E.g., where is the argument made that supports the model put forward here? (Also not entirely sure how you could realistically tackle proxies.)

mhofman commented 2 years ago

Changing those algorithms to accommodate more use cases is an option though. Although webcompat might put limits on that.

This is exactly my hope when I say I want to find a cloning mechanism that is backwards compatible with structure clone but that removes exiting objections from TC39 delegates. I don't think allowing new values that currently throw would be considered a compatibility risk, but maybe I'm mistaken?

Also a new cloning mechanism to support the ShadowRealm use case does not need to support all use cases of structure clone at first, as long as it doesn't diverge and there remains a way to build up to it in the future. For example starting with an explicit cloning of only SharedArrayBuffer objects would be totally fine.

Also not entirely sure how you could realistically tackle proxies

What I'm thinking about right now is that the cloning mechanism would have hooks, and that it would be acceptable to require users of proxies (membranes) to use those hooks if they want their proxies to behave transparently through cloning.

annevk commented 2 years ago

Removing exceptions (and following a different code path) is generally fine, indeed.

And yes, starting with SharedArrayBuffer might be okay, but the questions in https://github.com/tc39/proposal-shadowrealm/pull/336#issuecomment-989615318 stand. With the proposal here there is divergence and it's not clear how to take it further in the future.

mhofman commented 2 years ago

If the divergence is the inability to express transfer semantics in this proposal, I entirely agree, and that is why I'm in support of an alternative explicit cloning mechanism for Shadow realms, even for SAB, to avoid creating a special case precedent. I have a draft for such an explicit proposal, but it needs updating. However the shadow realm champions have expressed concerns with added complexity resulting from this explicit cloning. It's all very early, hopefully we can find a solution that answers everyone's requirements.

annevk commented 2 years ago

@leobalter pointed out the divergence in https://github.com/tc39/proposal-shadowrealm/pull/336#issuecomment-954175274. (And to be clear, I'm not sure how okay I am with duplicating part of HTML's algorithms and not reusing them explicitly. That creates a lot of risk for divergence going forward. I am okay with moving those algorithms elsewhere, as also stated in the other thread.)

(@erights btw, separate document/TC sounds very much like the old Appendix B wishful thinking. I also somewhat doubt anyone is willing to sign up for it in that form, nor does it seem like an improvement over the status quo.)

bakkot commented 2 years ago

this proposed change ignore any additional aspects of the sab objects like extra properties or subclassing

Does HTML's structured clone copy other properties of SABs? I'm almost certain it does not.

leobalter commented 2 years ago

@bakkot the functionality I observed for transferring a SAB throw workers does copy the properties. Maybe I took that would come in the package for structured cloning and I’d be happy if I’m corrected. I’d like to reuse the SAB internals, but I got no usage for coping the object properties.

Jamesernator commented 2 years ago

@bakkot the functionality I observed for transferring a SAB throw workers does copy the properties. Maybe I took that would come in the package for structured cloning and I’d be happy if I’m corrected. I’d like to reuse the SAB internals, but I got no usage for coping the object properties.

You can try out a compliant structuredClone in Chrome or Node, it doesn't clone properties on shared array buffers:

const buffer = new SharedArrayBuffer(10);
buffer.foo = 10;
const clonedBuffer = structuredClone(buffer);
'foo' in clonedBuffer; // false
clonedBuffer.foo === undefined; // true
bakkot commented 2 years ago

Yeah, I see the same behavior as @Jamesernator; that's also what I see in the spec (note that the [[ArrayBufferData]] branch does not set deep to true).

For anyone else looking to play with it in the browser, SAB is available on https://first-party-test.glitch.me/coep?coep=credentialless&coop=same-origin&

leobalter commented 2 years ago

@annevk I discussed with some people involved in this proposal, not limited to champions only, and I believe there is bigger challenge right now to reuse the Structured Cloning algorithms here, as they have their own set of challenges at TC39.

I think the current PR has steps that observed in user land like the steps from the HTML Specs' StructuredSerializeInternal steps 13.2. I actually crafted the current PR abstraction based on it, but they look different editorially.

If TC39 eventually agrees to bring Structured Cloning, I'm happy to reuse it. I'd say probably limiting it initially to SharedArrayBuffer, which is where our pain point remains, but open to discuss using the other aspects of the StructuredCloning.

WDYT if the observed behavior here doesn't differ and we commit to keep it this way? Of course, for now limiting this logic to SABs.

leobalter commented 2 years ago

@bakkot thanks for the clarification. I missed the deep value and this PR seems to end up closer than I imagined to that abstraction.

annevk commented 2 years ago

Our inclination is that the challenges with structured serializing/deserializing ought to be resolved before an equivalent mechanism is added elsewhere.

ljharb commented 2 years ago

Does that mean the web won’t be adding new things to structured cloning until these things can be resolved?

leobalter commented 2 years ago

To add clarity, we won't propose this change to the ongoing ShadowRealms proposal but bring it as a separate proposal following up ShadowRealms.

As discussed in the plenary today, we are considering exploring the alternatives to reuse the Structured serialization and so this work will be on hold for at least some Stage 3-like stage.

We can advance a few things that might not conflict, such as spinning a new repo and closing this PR, setting a specific explainer, and drafting a spec using the structured serialization parts, with what we estimate it would become. I understand this work might go stage 1. For stage 2 we need to resolve the contention with @mhofman who is proposing an alternative API.