LibJS: Test passes run as part of LibJS, but fails when run embedded in HTML

[shannonbooth]

Split from: #3230 as while they are somewhat related, this issue is likely to be smaller in scope and could be fixed independently.

The following test-js test passes:

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 it fails when run as part of HTML, see:

<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>

Which outputs:

66210.638 WebContent(90273): (js log) [Promise]
  state: Pending
66210.638 WebContent(90273): (js log) 0
66210.638 WebContent(90273): (js log) Object{ "y": 5 }

cc @Hendiadyoin1

[shannonbooth]

After some more analysis, the original report was very confused. I don't know why this passes in test-js, but this issue is also reproducible with the following JS:

let test = 0;
let result = (async function () {
    try {
        return 5;
    } finally {
        test = 42;
    }
})();
console.log(test);

Which will log 0 instead of 42. The issue is potentially related to codegen, but maybe something else I am missing. We are currently generating the bytecode inside of the async function as:

[   0]    0: Yield continuation:@18, value:Undefined
[  18]    1: EnterUnwindContext entry:@60
[  20]    2: LeaveUnwindContext
[  28]       SetLexicalBinding test, src:Int32(42)
[  40]       ContinuePendingUnwind resume:@48
[  48]    3: Yield return value:Undefined
[  60]    4: Await argument:Int32(5), continuation:@70
[  70]    5: Mov dst:reg6, src:reg0
[  88]       GetById dst:reg7, base:reg6, type
[  b0]       GetById dst:reg8, base:reg6, value
[  d8]       JumpStrictlyEquals lhs:reg7, rhs:Int32(1), true:@f8, false:@148
[  f8]    6: PrepareYield dst:reg1, value:reg8
[ 110]       Mov dst:reg2, src:<Empty>
[ 128]       Mov dst:reg4, src:<Empty>
[ 140]       Jump @20
[ 148]    7: Throw src:reg8

The await appears problematic, as it allows the code outside of the asynchronous function to continue executing before hitting the finally block.

If we remove the await generation for this case here:

ladybird/Libraries/LibJS/Bytecode/ASTCodegen.cpp

Line 1846 in 3050768

if (generator.is_in_async_function()) {

And instead generate:

[   0]    0: Yield continuation:@18, value:Undefined
[  18]    1: EnterUnwindContext entry:@60
[  20]    2: LeaveUnwindContext
[  28]       SetLexicalBinding test, src:Int32(42)
[  40]       ContinuePendingUnwind resume:@48
[  48]    3: Yield return value:Undefined
[  60]    4: PrepareYield dst:reg1, value:Int32(5)
[  78]       Mov dst:reg2, src:<Empty>
[  90]       Mov dst:reg4, src:<Empty>
[  a8]       Jump @20

The test will work fine.

[shannonbooth]

This seems to indicate that the FIXME saying that there is a potential spec bug is not correct, adjusting that to spec makes this test work.

[shannonbooth]

This was originally introduced in SerenityOS/serenity#20465

To fix: SerenityOS/serenity#20275

That specific line does not appear to be necessary for that issue. However, making that change to follow spec breaks a single test in test262:

Summary:
    Diff Tests:
         -1 ✅   +1 ❌   

Diff Tests:
    test/built-ins/Array/fromAsync/non-iterable-input-with-thenable-async-mapped-awaits-callback-result-once.js ✅ -> ❌

But it breaks 3 test-js tests in async-await.js:

ladybird/Libraries/LibJS/Tests/syntax/async-await.js

Line 260 in 3050768

describe("await observably looks up constructor of Promise objects", () => {

ladybird/Libraries/LibJS/Tests/syntax/async-await.js

Line 205 in 3050768

test("async returning a thanable variable without fulfilling", () => {

ladybird/Libraries/LibJS/Tests/syntax/async-await.js

Line 246 in 3050768

test("async returning a thenable directly that fulfills", () => {

Which were also introduced as part of that change