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.

Sign up for GitHub

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

LibJS: Only run queued promise jobs if there is no embedder #3231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −5
Open

LibJS: Only run queued promise jobs if there is no embedder #3231

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Libraries/LibJS/Runtime/Completion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,15 @@ ThrowCompletionOr<Value> await(VM& vm, Value value)
promise->perform_then(on_fulfilled, on_rejected, {});

// FIXME: Since we don't support context suspension, we attempt to "wait" for the promise to resolve
// by letting the event loop spin until our promise is no longer pending, and then synchronously
// running all queued promise jobs.
// Note: This is not used by LibJS itself, and is performed for the embedder (i.e. LibWeb).
// by syncronously running all queued promise jobs.
if (auto* custom_data = vm.custom_data()) {
// Embedder case (i.e. LibWeb). Runs all promise jobs by performing a microtask checkpoint.
custom_data->spin_event_loop_until(GC::create_function(vm.heap(), [success] {
Copy link
Contributor Author

@shannonbooth shannonbooth Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spinning the event loop like this technically also runs all promise jobs, but I'm not 100% sure if the spin itself is fully correct for all cases and interleaving other potential stuff to happen.

Copy link
Contributor Author

@shannonbooth shannonbooth Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The running of queued promise jobs in non-embedder case below is definitely less correct than spinning the event loop in the sense it doesn't do the clear of the execution context stack, but I haven't quite figured out the normative impact of that yet

return success.has_value();
}));
} else {
// No embbedder, standalone LibJS implementation
vm.run_queued_promise_jobs();
}

// 8. Remove asyncContext from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.
Expand All @@ -113,8 +115,6 @@ ThrowCompletionOr<Value> await(VM& vm, Value value)
// 10. Return NormalCompletion(unused).
// 11. NOTE: This returns to the evaluation of the operation that had most previously resumed evaluation of asyncContext.

vm.run_queued_promise_jobs();

// Make sure that the promise _actually_ resolved.
// Note that this is checked down the chain (result.is_empty()) anyway, but let's make the source of the issue more clear.
VERIFY(success.has_value());
Expand Down
Loading