Re: [whatwg/webidl] Use SafePromiseResolve to replace the promise resolution steps (PR #1645)

annevk left a comment (whatwg/webidl#1645)

<details>
<summary>AI feedback</summary>

Reviewed at 8dd16a0. CI passes and the branch merges cleanly with main. The three changed steps look right: `SafePromiseResolve` takes a PromiseCapability Record, the `!` assertions still hold because the built-in resolving functions can't throw, a thenable's `then()` is still called in the same microtask as before, and leaving the reject paths alone makes sense.

**Blockers**

- [HostObjectHasImpureLookupInternalMethods](https://tc39.es/proposal-thenable-curtailment/#sec-hosthasinternalmethodswhichcanrunusercode) isn't defined for Web IDL's objects, so it defaults to false. Legacy platform objects that support named properties need it: [LegacyPlatformObjectGetOwnProperty](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L14283) runs the [named property visibility algorithm](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L14217-L14221), which calls `[[GetPrototypeOf]]` and "has an own property" up a mutable prototype chain. For example, with `<form name=then>` and `Object.setPrototypeOf(HTMLCollection.prototype, new Proxy(…))`, resolving with `document.forms` (e.g., by returning it from a `pull()`) makes [PropertyAccessCouldRunUserCode](https://tc39.es/proposal-thenable-curtailment/#sec-has-property-which-could-run-user-code)'s `! o.[[GetOwnProperty]]("then")` run the Proxy's traps synchronously, which breaks its "do not run user code" assertion. There's an interop angle too: Gecko implements these objects and `WindowProxy` as engine-level proxies, and the [README's experiment](https://github.com/tc39/proposal-thenable-curtailment/blob/c162ea93667a61f4a07868a35e4964942ce885a1/README.md#experiment-webidl) already saw an extra tick when resolving with a `WindowProxy`, which the spec as written doesn't predict. The hook could return true for those that lack `[LegacyOverrideBuiltIns]` (with it, the visibility algorithm returns before walking the chain), or for all legacy platform objects. It would also be good to say (in HTML) whether `WindowProxy` and `Location` count. Either way this needs tests.
- web-platform-tests/wpt#64001 doesn't update [streams/readable-byte-streams/patched-global.any.js](https://github.com/web-platform-tests/wpt/blob/8a5805234c70967e448b07eb8ae86155f0bf4299/streams/readable-byte-streams/patched-global.any.js#L40), which asserts that an `Object.prototype.then` getter runs synchronously during `controller.enqueue()`. With this change the getter is deferred, so that test fails (the README lists it as a failure).

**Suggestions**

- Consider clarifying the scope in the PR description. It says async iteration is left alone, but the async sequence algorithms [get the next value](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L8385) and [close an async iterator](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L8417) use "a promise resolved with", so they change too. Only `next()` and `return()` on asynchronous iterator prototype objects ([1](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L13161), [2](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L13250)) keep calling `[[Resolve]]`. Also, the change to [converting to a promise type](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L8580) affects callback return values (e.g., Streams' `pull()`), not just arguments.
- Consider adding a note to those `next()` and `return()` steps saying they intentionally keep calling `[[Resolve]]` to match [AsyncGeneratorCompleteStep](https://tc39.es/ecma262/#sec-asyncgeneratorcompletestep), so it doesn't read as an oversight.
- Consider adding a note to [resolve](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L8651) stating the new guarantee: resolving a promise never synchronously runs JavaScript. It could also mention the edge case that a value with a `then` getter returning a non-callable now fulfills one microtask later.
- Consider which realm is used. `SafePromiseResolve` creates `deferredThen` in the current realm, and the job takes its realm from that function, where it used to come from the `then` function. HTML's [HostEnqueuePromiseJob](https://html.spec.whatwg.org/multipage/webappapis.html#hostenqueuepromisejob) uses that realm to decide whether the job runs at all. As [resolve](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L8657) takes no realm and is often invoked from tasks, this is underdefined (see #371 and #932). Creating `deferredThen` in the promise's realm (upstream), or having Web IDL specify the realm, would avoid that.
- As this appears to be your first contribution here, consider adding yourself to the [Acknowledgments](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L15325).

**Nits**

- [`url: sec-safe-resolve-promise`](https://github.com/whatwg/webidl/blob/8dd16a0258e95f9cf560531687f82b49b63d47db/index.bs#L96) points to the chapter heading; the operation is `sec-safe-promise-resolve` (as already suggested in review). The WPT comments link to the same chapter anchor.
- [fetch/api/response/response-body-read-task-handling.html](https://github.com/web-platform-tests/wpt/blob/8a5805234c70967e448b07eb8ae86155f0bf4299/fetch/api/response/response-body-read-task-handling.html) now passes without testing what it was written for; consider updating it in web-platform-tests/wpt#64001 as well.
- Proposal FYI: the rendered draft still says "Stage 2 Draft", and [RequiresDeferredPromiseResolution](https://tc39.es/proposal-thenable-curtailment/#sec-requires-deferred-promise-resolution) calls `value.[[Get]]("then")` without a Receiver argument.

</details>


-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/webidl/pull/1645#issuecomment-6384949921
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/webidl/pull/1645/c6384949921@github.com>