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

InnerModuleEvaluation handles rejected promises of non-CyclicModuleRecords without HostPromiseRejectionTracker hook #3533

Open
woess opened this issue Feb 7, 2025 · 2 comments · May be fixed by #3535

Comments

@woess
Copy link
Contributor

woess commented Feb 7, 2025

I believe there's a problem with how rejected promises for evaluation errors of non-Cyclic (i.e. Synthetic) Module Records are handled w.r.t. HostPromiseRejectionTracker in
InnerModuleEvaluation ( module, stack, index ):

1. If _module_ is not a Cyclic Module Record, then
  a. Let _promise_ be ! _module_.Evaluate().
  b. Assert: _promise_.[[PromiseState]] is not ~pending~.
  c. If _promise_.[[PromiseState]] is ~rejected~, then
    i. Return ThrowCompletion(_promise_.[[PromiseResult]]).

In case of an evaluation error, this code will just unwrap the promise and throw the error, not notifying any HostPromiseRejectionTracker hook that the promise has been handled. OTOH, Evaluate() (presumably) will have already performed RejectPromise (via IfAbruptRejectPromise) and called HostPromiseRejectionTracker(promise, "reject") since [[PromiseIsHandled]] is false. If my understanding is correct, the promise rejection would thus falsely appear to be unhandled.

This code path isn't actually reachable in the spec as of now, but will be when JSON Modules, and thus Synthetic Module Records, land (i.e. #3391).

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 7, 2025

Non-cyclic module records are all synchronous, and the fact that they return a promise is purely a spec artifact (hence why the assertion that the promise is already settled).

Maybe we should just remove the "fake promise" from there.

@woess
Copy link
Contributor Author

woess commented Feb 7, 2025

It might still make sense to have the promise for ContinueDynamicImport, step 6.c. Let evaluatePromise be module.Evaluate().
Maybe this could be layered differently, though, idk.

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

Successfully merging a pull request may close this issue.

2 participants