LibJS: Promise queue evaluated at incorrect timing / inconsistent with HTML integration

[shannonbooth]

Example test crashing as a result of this is the following JS:

Promise.all([
    import('./import.js'),
    import('./import.js'),
  ])
  .then(function(values) {
    console.log(values[0].default);
    console.log(values[1].default);
  });

With an import.js of:

export default 262;

Which causes an assertion of:

VERIFICATION FAILED: m_status == ModuleStatus::Unlinked || m_status == ModuleStatus::Linked || m_status == ModuleStatus::EvaluatingAsync || m_status == ModuleStatus::Evaluated at /home/shannon/personal/ladybird/Libraries/LibJS/CyclicModule.cpp:173
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-ak.so.0(ak_trap+0x35) [0x7f88d81e31a5]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-ak.so.0(+0x154cf) [0x7f88d81e34cf]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0 JS::CyclicModule::link(JS::VM&) 0x183) [0x7f88d8c86023]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0(+0x1cbd71) [0x7f88d8c88d71]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0 JS::NativeFunction::call() 0x55) [0x7f88d8e14b95]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0 JS::NativeFunction::internal_call(JS::Value, AK::Span<JS::Value const>) 0x183) [0x7f88d8e147c3]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0(+0x382d5f) [0x7f88d8e3fd5f]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0 JS::VM::run_queued_promise_jobs() 0xb3) [0x7f88d8f6a7c3]
/home/shannon/personal/ladybird/Build/debug/lib/liblagom-js.so.0 JS::Bytecode::Interpreter::run_executable(JS::Bytecode::Executable&, AK::Optional<unsigned long>, JS::Value) 0x286) [0x7f88d8c363e6]

This is a result of running an executable incorrectly running the queued promises:

ladybird/Libraries/LibJS/SourceTextModule.cpp

Line 723 in 19f9747

auto result_and_return_register = vm.bytecode_interpreter().run_executable(*executable, {});

Resulting in module linking on a module graph which has an incorrect / incomplete state.

This is only a known problem in the pure LibJS implementation due to it's implementation of the host hooks, see:

ladybird/Libraries/LibJS/Runtime/VM.cpp

Line 434 in 19f9747

void VM::enqueue_promise_job(GC::Ref<GC::Function<ThrowCompletionOr<Value>()>> job, Realm*)

Currently, we run the queued promise jobs during bytecode evaluation, see:

ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp

Line 271 in 19f9747

// FIXME: These three should be moved out of Interpreter::run and give the host an option to run these, as it's up to the host when these get run.

ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp

Line 760 in 19f9747

vm().run_queued_promise_jobs();

ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp

Line 300 in 19f9747

vm.run_queued_promise_jobs();

And also while awaiting a completion:

ladybird/Libraries/LibJS/Runtime/Completion.cpp

Line 116 in 19f9747

vm.run_queued_promise_jobs();

(Note that run_queued promise_jobs() only does something when LibJS is running standalone. I.e, that function call does nothing when run as part of LibWeb).

Somehow we need to implement the requirement of only running the promise queue in a manner that meets requirement 9.5 of https://tc39.es/ecma262/#sec-jobs. Ideally, this would also be done in a way that closer unifies the implementation of HTML promise queue with the JS one.

A hacky fix for the issue above is:

shannonbooth@8a57be5

Which gives the test 262 diff of:

+18 ✅ -18 💥️

An even more ugly change that fixes even more tests is:

shannonbooth@c359084

Which results in:
+31 ✅ -13 ❌ -18 💥

I've also made attempts of changing the pending promise queue to include the relevant script or module, realm, etc (per spec), and run those jobs only when the execution context is empty. But every attempt of that broke a lot of test262 tests. I'm unclear on at what points we actually need to be 'performing a microtask checkpoint' in our pure JS implementation.

One test-js test that I have had trouble with, for example, is:

test("return from async through finally", () => {
    let test = 0;
    let result = (async function () {
        try {
            return { y: 5 };
        } finally {
            test = 42;
        }
        expect.fail("unreachable");
    })();

    expect(result).toBeInstanceOf(Promise);
    expect(test).toBe(42);
    result.then(value => {
        expect(value).toEqual({ y: 5 });
    });
});

But test-262 catches a whole lot more cases.

The above test does not pass when run in HTML, which does indicate some deeper problems:

<script>
    let test = 0;
    let result = (async function () {
        try {
            return { y: 5 };
        } finally {
            test = 42;
        }
        console.log("FAIL: unreachable");
    })();

    console.log(result);
    console.log(test);
    result.then(value => {
        console.log(value);
    });
</script>