LibJS: Clear exception after running each queued Promise job

It's not what the spec tells us to do. In fact, the spec tells us the
exact opposite:

    9.5 Jobs and Host Operations to Enqueue Jobs
    https://tc39.es/ecma262/#sec-jobs

    A Job is an Abstract Closure with no parameters that initiates an
    ECMAScript computation when no other ECMAScript computation is
    currently in progress.
    ...
    Their implementations must conform to the following requirements:
    - ...
    - The Abstract Closure must return a normal completion, implementing
      its own handling of errors.

However, this turned out to not be true in all cases. More specifically,
the NewPromiseReactionJob AO returns the completion result of calling a
user-provided function (PromiseCapability's [[Resolve]] / [[Reject]]),
which may be an abrupt completion:

    27.2.2.1 NewPromiseReactionJob ( reaction, argument )
    https://tc39.es/ecma262/#sec-newpromisereactionjob

    1. Let job be a new Job Abstract Closure with no parameters that
       captures reaction and argument and performs the following steps
       when called:
       ...
       h. If handlerResult is an abrupt completion, then
          i. Let status be Call(promiseCapability.[[Reject]],
             undefined, « handlerResult.[[Value]] »).
       i. Else,
          i. Let status be Call(promiseCapability.[[Resolve]],
             undefined, « handlerResult.[[Value]] »).
       j. Return Completion(status).

Interestingly, this case is explicitly handled in the HTML spec's
implementation of jobs as microtasks:

    8.1.5.3.3 HostEnqueuePromiseJob(job, realm)
    https://html.spec.whatwg.org/webappapis.html#hostenqueuepromisejob

    2. Queue a microtask on the surrounding agent's event loop to
       perform the following steps:
       ...
       5. If result is an abrupt completion, then report the exception
          given by result.[[Value]].

This is precisely what all the major engines do - but not only in
browsers; the provided code snippet in the test added in this commit
works just fine in Node.js, for example.

SpiderMonkey:
https://searchfox.org/mozilla-central/rev/25997ce8267ec9e3ea4b727e0973bd9ef02bba79/js/src/builtin/Promise.cpp#6292
https://searchfox.org/mozilla-central/rev/25997ce8267ec9e3ea4b727e0973bd9ef02bba79/js/src/builtin/Promise.cpp#1277
https://searchfox.org/mozilla-central/rev/25997ce8267ec9e3ea4b727e0973bd9ef02bba79/js/src/vm/JSContext.cpp#845

JavaScriptCore:
https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/builtins/PromiseOperations.js?rev=273718#L562
https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/JSMicrotask.cpp?rev=273718#L94

V8:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/promise-abstract-operations.tq;l=481;drc=a760f03a6e99bf4863d8d21c5f7896a74a0a39ea
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-microtask-queue-gen.cc;l=331;drc=65c9257f1777731d6d0669598f6fe6fe65fa61d3

This should probably be fixed in the ECMAScript spec to relax the rule
that Jobs may not return an abrupt completion, just like in the HTML
spec. The important bit is that those are not surfaced to user code in
any way.

2 files changed, 36 insertions, 1 deletions

diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp
index 6be945d8c5..c288c3a830 100644
--- a/Userland/Libraries/LibJS/Runtime/VM.cpp
+++ b/Userland/Libraries/LibJS/Runtime/VM.cpp
@@ -524,7 +524,7 @@ void VM::run_queued_promise_jobs()
     dbgln_if(PROMISE_DEBUG, "Running queued promise jobs");
     // Temporarily get rid of the exception, if any - job functions must be called
     // either way, and that can't happen if we already have an exception stored.
-    TemporaryClearException clear_exception(*this);
+    TemporaryClearException temporary_clear_exception(*this);
     while (!m_promise_jobs.is_empty()) {
         auto* job = m_promise_jobs.take_first();
         dbgln_if(PROMISE_DEBUG, "Calling promise job function @ {}", job);
@@ -541,10 +541,18 @@ void VM::run_queued_promise_jobs()
 
         [[maybe_unused]] auto result = call(*job, js_undefined());
 
+        // This doesn't match the spec, it actually defines that Job Abstract Closures must return
+        // a normal completion. In reality that's not the case however, and all major engines clear
+        // exceptions when running Promise jobs. See the commit where these two lines were initially
+        // added for a much more detailed explanation.
+        clear_exception();
+        stop_unwind();
+
         if (pushed_execution_context)
             pop_execution_context();
     }
     // Ensure no job has created a new exception, they must clean up after themselves.
+    // If they don't, we help a little (see above) so that this assumption remains valid.
     VERIFY(!m_exception);
 }
 
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Promise/Promise.prototype.then.js b/Userland/Libraries/LibJS/Tests/builtins/Promise/Promise.prototype.then.js
index 804d133865..7f41b12bbe 100644
--- a/Userland/Libraries/LibJS/Tests/builtins/Promise/Promise.prototype.then.js
+++ b/Userland/Libraries/LibJS/Tests/builtins/Promise/Promise.prototype.then.js
@@ -140,4 +140,31 @@ describe("normal behavior", () => {
         runQueuedPromiseJobs();
         expect(fulfillmentValue).toBe("Some value");
     });
+
+    test("PromiseCapability with throwing resolve function", () => {
+        class PromiseLike {
+            constructor(executor) {
+                executor(
+                    () => {
+                        throw new Error();
+                    },
+                    () => {}
+                );
+            }
+        }
+
+        let resolvePromise;
+        const p = new Promise(resolve => {
+            resolvePromise = resolve;
+        });
+        p.constructor = { [Symbol.species]: PromiseLike };
+
+        // Triggers creation of a PromiseCapability with the throwing resolve function passed to the executor above
+        p.then(() => {});
+
+        // Crashes when assuming there are no job exceptions (as per the spec)
+        // If we survive this, the exception has been silently ignored
+        resolvePromise();
+        runQueuedPromiseJobs();
+    });
 });