"A promise resolved with" can unexpectedly run JavaScript

[ricea]

https://www.w3.org/2001/tag/doc/promises-guide#shorthand-creating says:

"A promise resolved with x" or "x resolved as a promise" is shorthand for the result of Promise.resolve(x), using the initial value of Promise.resolve.

If x is a promise this can cause JavaScript to run, due to the definition of PromiseResolve in ECMASCRIPT, step 2a. Specifically, it calls Get(x, "constructor"). If Promise.prototype.constructor has been set on the global object, it will be consulted. If a getter has been set, it will be executed.

This is a problem for the CreateReadableStream() operation in the streams standard. It has a note that "CreateReadableStream throws an exception if and only if the supplied startAlgorithm throws.". This will not be true if startAlgorithm returns a Promise and the global Promise.prototype.constructor has been messed with, which is surprising.

It is not clear whether this should be fixed here or in the streams standard.

[annevk]

This should be fixed here (well, IDL, as I like to keep saying, as this is really not the right place). All of these should be C++^WRust safe.

[domenic]

This should probably be fixed in Streams. It'd be bad to introduce a new way of resolving promises which is unlike anything else in JS. (I.e., one which skips the .constructor check, so that subclasses get turned into Promise instances.)

Alternately it could be fixed in JS, to return a rejected promise instead of rethrowing the getter exception.

[annevk]

So creating a new promise and then resolving it always requires a round trip through the JavaScript engine? And if so, resolving in general? That seems sad, but maybe it's okayish as it's always the last step in an algorithm anyway. Might still be surprising though.

[ricea]

I discovered we have the same problem with "Upon fulfillment", "Upon rejection" and "Transforming", which means any "nothrow" operation which performs these can actually throw, transitively breaking large chunks of the streams standard.

We really need a way to say "I promise this is a real Promise, please skip the check".

[domenic]

I mean, promises are JS objects, so creating them always requires a trip through the JS engine anyway.

I discovered we have the same problem with "Upon fulfillment", "Upon rejection" and "Transforming"

Grounding these in more correct things would use PerformPromiseThen, which shouldn't have this problem.

[annevk]

@domenic an implementation could use a similar strategy to platform objects that are allocated in C++^WRust, and whenever JavaScript needs access it gets a wrapper.

[domenic]

That's theoretically true, but in practice I'm not aware of any JS engines which have the wrapper/impl distinction for their objects. Regardless, it's best to stick to discussing observable consequences, and I maintain it'd be unfortunate to have a way of resolving promises which behave differently on subclasses than Promise.resolve() does.

[ricea]

The streams standard has a need to resolve promises it created internally, without being exposed to user script. It knows the promises are not subclasses, because it created them itself. Maybe it is unique in that respect, but IIUC the service worker standard has similar concerns.

Anyway, as a stopgap I can add operations to the streams standard which avoid touching Promise.prototype.constructor.

Maybe the long-term solution is to add an asynchronous algorithm primitive to INFRA to get a clear separation between the things we want to run user script and the things we don't.

[annevk]

Maybe it is unique in that respect, but IIUC the service worker standard has similar concerns.

It seems worth sorting this out as apparently there are different expectations at TC39 and we're apparently using hooks engines are not meant to expose to implement this.

[domenic]

We could resolve this by separating "A promise resolved with" and "A promise resolved with an arbitrary value". The former would have a step 1 that does "Assert: IsPromise(value) is false."

Better name suggestions welcome. In particular my names are not great because I think a lot of specs are currently using "a promise resolved with" on arbitrary values already...

[ricea]

I think we still need the existing semantics in the implementation of PromiseCall().

Other cases from the Streams Standard:

• 13 cases of "a promise resolved with undefined". These can use the new semantics of "A promise resolved with".
• 5 cases of "a promise resolved with ! ReadableStreamCreateReadResult". These can also use the new semantics of "A promise resolved with".
• 3 cases of "a promise resolved with startResult". These are the problematic ones. If startResult came from an algo, we can't have it look up Promise.prototype.constructor. But if it came from user code, then it should still do that lookup. This is tricky. I think we need to handle it in the standard itself.

So I find myself leaning toward "the streams standard should be fixed". However, tightening up the definition here also has value in preventing future mistakes of the same kind.