-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Normative: PromiseResolve check proto instead of constructor #3689
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Kevin Gibbons <[email protected]>
| 1. Let _xConstructor_ be ? Get(_x_, *"constructor"*). | ||
| 1. If SameValue(_xConstructor_, _C_) is *true*, return _x_. | ||
| 1. Let _xProto_ be ! _x_.[[GetPrototypeOf]](). | ||
| 1. Let _CPrototype_ be ? Get(_C_, *"prototype"*). | ||
| 1. If SameValue(_xProto_, _CPrototype_) is *true*, return _x_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, consider (realPromise => Reflect.apply(Promise.resolve, Object.defineProperty(function(){ throw Error("invoked"); }, "prototype", { value: Promise.prototype }), [realPromise]) === realPromise)(new Promise(() => {})) in an unmodified environment. With the current definition of PromiseResolve, it will invoke the supplied constructor (effectively attempting to return an instance from it), but with the proposed new definition, it will instead return realPromise and the entire expression will be true.
This is basically inverting the loophole that already exists, where instead of a real promise being able to fake association with an arbitrary constructor (and getting the chance to run code in the process), an arbitrary constructor can fake association with any real promise. But since resolve is a method of the constructor anyway, that power totally makes sense. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, .resolve trusts and works in the context of its "constructor" receiver.
Co-authored-by: Kevin Gibbons <[email protected]>
Highlight that %PromiseThenAction% is not accessible to user code when calling HostMakeJobCallback
| 1. Let _xConstructor_ be ? Get(_x_, *"constructor"*). | ||
| 1. If SameValue(_xConstructor_, _C_) is *true*, return _x_. | ||
| 1. Let _xProto_ be ! _x_.[[GetPrototypeOf]](). | ||
| 1. Let _CPrototype_ be ? Get(_C_, *"prototype"*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wanted to be really prescriptive about this, we could check if _C_ is a constructor/function. I don't believe the .prototype of a function can ever be an accessor? So we could turn this into ! instead of ?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some functions don't have a .prototype, which could be installed manually. Also C could be a proxy for a constructor / function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it’s not a constructor, then we can’t new C, so NewPromiseCapability(C) would just throw. I don’t think I we need to consider those.
I’m kinda ambivalent on whether we need to support proxies of the promise constructor. Up to you.
|
The
|
|
Added details on web compatibility and potential usage counters. |
Co-authored-by: Kevin Gibbons <[email protected]>
Assert promise resolution is an ordinary object
|
The rendered spec for this PR is available at https://tc39.es/ecma262/pr/3689. |
Assert promise resolution is an ordinary object
|
I have updated this PR to reduce the scope to only update |
This PR changes the bespoke "species" check that
PromiseResolvedoes on native promises to lookup whether the__proto__of the promise matches the.prototypevalue of the constructor, instead of matching the.constructorvalue of the promise. If the constructor value is%Promise%, the value used by the spec's abstract operations likeAwait, no user code executes duringPromiseResolvewhen the value is a native base promise. Closes #3662.Since
Awaitalready adopts a native promise (it does not trigger a.thenfor those type of values), this change guarantees that no user code is executed when adopting a base native promise (of the same realm), e.g. in async functions whenawait-ing the result value of a call to another async function.This PR does not change the way non promise thenables are handled, including when the resolution values are "unexpected thenables" (see thenable curtailment proposal).
PromiseResolvemay be called with a constructor other than%Promise%, e.g. when the user calls%Promise.resolve%with another receiver, in which case the constructor (receiver) may be able to observe the.prototypelookup.Effectively for native promises, this moves the trust of the "species" check from the native promise value being tested to the constructor being tested against.
Web compatibility
I cannot imagine a legitimate case where the current
.constructorcheck wouldn't be equivalent to the proposed.__proto__check. Cases where the check would not be equivalent:.constructorproperty on the promise instance%Promise.prototype%.constructorproperty%Promise.prototype%, but that maintains a.constructorof%Promise%.The latter case is the only remotely legitimate use case. However it is also the reverse case where a value that was previously passed through
PromiseResolvewould instead become wrapped in a new promise through its.then, continuing to work, albeit with extra ticks.If needed we could instrument an existing implementation to detect how often a Promise would now be passed through when it previously was wrapped. We could instrument the other direction too, but it doesn't seem worthwhile. If possible. we may also want to measure how often the
.constructorcheck results in user code execution, even if it does result in the same passthrough outcome.A note on user code execution in these web compat checks
It is possible that looking up
.constructormay result in proxy traps triggering, even if the receiver is a native promise, and its__proto__is%Promise.prototype%: the user code may have replaced the%Promise.prototype%.__proto__with a proxy, and deleted the originalconstructorproperty from%Promise.prototype%.Promise adoption through resolve functions
This PR previously updated the promise resolution steps of the resolve function to adopt native promises. The scope of this PR has been reduced and the promise adoption part has been spun off in its own proposal.