From 3441b37de520a797c0d457081c66b03deaf210b7 Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Fri, 3 Nov 2023 18:54:12 -0600 Subject: [PATCH] LibWeb: Store Promise::wait_for_all state in a JS-heap allocated object The one current caller of this function always defers microtask checkpoints before calling wait_for_all, ensuring that the promise accept/reject handlers will always be called later in the Web event loop processing. We need to store all the state for the closures in a heap allocated object with HeapFunctions to keep it around while there are still promises to resolve. --- Userland/Libraries/LibWeb/HTML/Navigation.cpp | 25 ++++---- Userland/Libraries/LibWeb/WebIDL/Promise.cpp | 58 ++++++++++++++----- Userland/Libraries/LibWeb/WebIDL/Promise.h | 2 +- 3 files changed, 57 insertions(+), 28 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/Navigation.cpp b/Userland/Libraries/LibWeb/HTML/Navigation.cpp index 363a4e7506..383a2d8466 100644 --- a/Userland/Libraries/LibWeb/HTML/Navigation.cpp +++ b/Userland/Libraries/LibWeb/HTML/Navigation.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -1068,7 +1069,7 @@ bool Navigation::inner_navigate_event_firing_algorithm( // 31. Prepare to run script given navigation's relevant settings object. // NOTE: There's a massive spec note here - relevant_settings_object(*this).prepare_to_run_script(); + TemporaryExecutionContext execution_context { relevant_settings_object(*this) }; // 32. If event's interception state is not "none": if (event->interception_state() != NavigateEvent::InterceptionState::None) { @@ -1117,11 +1118,6 @@ bool Navigation::inner_navigate_event_firing_algorithm( for (auto const& handler : event->navigation_handler_list()) { // 1. Append the result of invoking handler with an empty arguments list to promisesList. auto result = WebIDL::invoke_callback(handler, {}); - if (result.is_abrupt()) { - // FIXME: https://github.com/whatwg/html/issues/9774 - report_exception(result.release_error(), realm); - continue; - } // This *should* be equivalent to converting a promise to a promise capability promises_list.append(WebIDL::create_resolved_promise(realm, result.value().value())); } @@ -1139,9 +1135,12 @@ bool Navigation::inner_navigate_event_firing_algorithm( // 4. Wait for all of promisesList, with the following success steps: WebIDL::wait_for_all( - realm, promises_list, [&](JS::MarkedVector const&) -> void { + realm, promises_list, [event, this, api_method_tracker](auto const&) -> void { + // FIXME: Spec issue: Event's relevant global objects' *associated document* // 1. If event's relevant global object is not fully active, then abort these steps. + auto& relevant_global_object = verify_cast(HTML::relevant_global_object(*event)); + auto& realm = event->realm(); if (!relevant_global_object.associated_document().is_fully_active()) return; @@ -1170,12 +1169,14 @@ bool Navigation::inner_navigate_event_firing_algorithm( m_transition = nullptr; // 9. If apiMethodTracker is non-null, then resolve the finished promise for apiMethodTracker. - if (api_method_tracker) + if (api_method_tracker != nullptr) resolve_the_finished_promise(*api_method_tracker); }, // and the following failure steps given reason rejectionReason: - [&](JS::Value rejection_reason) -> void { + [event, this, api_method_tracker](JS::Value rejection_reason) -> void { // FIXME: Spec issue: Event's relevant global objects' *associated document* // 1. If event's relevant global object is not fully active, then abort these steps. + auto& relevant_global_object = verify_cast(HTML::relevant_global_object(*event)); + auto& realm = event->realm(); if (!relevant_global_object.associated_document().is_fully_active()) return; @@ -1213,18 +1214,18 @@ bool Navigation::inner_navigate_event_firing_algorithm( m_transition = nullptr; // 9. If apiMethodTracker is non-null, then reject the finished promise for apiMethodTracker with rejectionReason. - if (api_method_tracker) + if (api_method_tracker != nullptr) reject_the_finished_promise(*api_method_tracker, rejection_reason); }); } // 34. Otherwise, if apiMethodTracker is non-null, then clean up apiMethodTracker. - else if (api_method_tracker) { + else if (api_method_tracker != nullptr) { clean_up(*api_method_tracker); } // 35. Clean up after running script given navigation's relevant settings object. - relevant_settings_object(*this).clean_up_after_running_script(); + // Handled by TemporaryExecutionContext destructor from step 31 // 36. If event's interception state is "none", then return true. // 37. Return false. diff --git a/Userland/Libraries/LibWeb/WebIDL/Promise.cpp b/Userland/Libraries/LibWeb/WebIDL/Promise.cpp index 92e30392ca..a3951cc0f0 100644 --- a/Userland/Libraries/LibWeb/WebIDL/Promise.cpp +++ b/Userland/Libraries/LibWeb/WebIDL/Promise.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -182,18 +183,46 @@ void mark_promise_as_handled(Promise const& promise) promise_object->set_is_handled(); } +struct WaitForAllResults : JS::Cell { + JS_CELL(WaitForAllResults, JS::Cell); + + WaitForAllResults(JS::NonnullGCPtr const&)>> s, size_t t) + : success_steps(s) + , total(t) + { + // 8. Let result be a list containing total null values. + result.ensure_capacity(total); + for (size_t i = 0; i < total; ++i) + result.unchecked_append(JS::js_null()); + } + + virtual void visit_edges(JS::Cell::Visitor& visitor) override + { + Base::visit_edges(visitor); + visitor.visit(success_steps); + for (auto& value : result) { + visitor.visit(value); + } + } + + JS::NonnullGCPtr const&)>> success_steps; + Vector result; + size_t total = 0; + size_t fulfilled_count = 0; +}; + // https://webidl.spec.whatwg.org/#wait-for-all -void wait_for_all(JS::Realm& realm, JS::MarkedVector> const& promises, JS::SafeFunction const&)> success_steps, JS::SafeFunction failure_steps) +void wait_for_all(JS::Realm& realm, Vector> const& promises, Function const&)> success_steps, Function failure_steps) { // FIXME: Fix spec typo, fullfilled --> fulfilled // 1. Let fullfilledCount be 0. - size_t fulfilled_count = 0; + // Handled later in WaitForAllResults // 2. Let rejected be false. auto rejected = false; // 3. Let rejectionHandlerSteps be the following steps given arg: - auto rejection_handler_steps = [&rejected, failure_steps = move(failure_steps)](JS::VM& vm) -> JS::ThrowCompletionOr { + auto rejection_handler_steps = [rejected, failure_steps = JS::create_heap_function(realm.heap(), move(failure_steps))](JS::VM& vm) mutable -> JS::ThrowCompletionOr { // 1. If rejected is true, abort these steps. if (rejected) return JS::js_undefined(); @@ -202,7 +231,7 @@ void wait_for_all(JS::Realm& realm, JS::MarkedVector> rejected = true; // 3. Perform failureSteps given arg. - failure_steps(vm.argument(0)); + failure_steps->function()(vm.argument(0)); return JS::js_undefined(); }; @@ -216,8 +245,8 @@ void wait_for_all(JS::Realm& realm, JS::MarkedVector> // 6. If total is 0, then: if (total == 0) { // 1. Queue a microtask to perform successSteps given « ». - HTML::queue_a_microtask(nullptr, [&realm, success_steps = move(success_steps)] { - success_steps(JS::MarkedVector { realm.heap() }); + HTML::queue_a_microtask(nullptr, [success_steps = JS::create_heap_function(realm.heap(), move(success_steps))] { + success_steps->function()({}); }); // 2. Return. @@ -228,10 +257,9 @@ void wait_for_all(JS::Realm& realm, JS::MarkedVector> auto index = 0; // 8. Let result be a list containing total null values. - auto result = JS::MarkedVector(realm.heap()); - result.ensure_capacity(total); - for (size_t i = 0; i < total; ++i) - result.unchecked_append(JS::js_null()); + // Handled in WaitForAllResults + + auto results = realm.heap().allocate(realm, JS::create_heap_function(realm.heap(), move(success_steps)), total); // 9. For each promise of promises: for (auto const& promise : promises) { @@ -240,18 +268,18 @@ void wait_for_all(JS::Realm& realm, JS::MarkedVector> // FIXME: This should be fulfillmentHandlerSteps // 2. Let fulfillmentHandler be the following steps given arg: - auto fulfillment_handler_steps = [&](JS::VM& vm) -> JS::ThrowCompletionOr { + auto fulfillment_handler_steps = [results, promise_index](JS::VM& vm) -> JS::ThrowCompletionOr { auto arg = vm.argument(0); // 1. Set result[promiseIndex] to arg. - result[promise_index] = arg; + results->result[promise_index] = arg; // 2. Set fullfilledCount to fullfilledCount + 1. - ++fulfilled_count; + ++results->fulfilled_count; // 3. If fullfilledCount equals total, then perform successSteps given result. - if (fulfilled_count == total) - success_steps(result); + if (results->fulfilled_count == results->total) + results->success_steps->function()(results->result); return JS::js_undefined(); }; diff --git a/Userland/Libraries/LibWeb/WebIDL/Promise.h b/Userland/Libraries/LibWeb/WebIDL/Promise.h index ddc7ebe711..53b5a9d427 100644 --- a/Userland/Libraries/LibWeb/WebIDL/Promise.h +++ b/Userland/Libraries/LibWeb/WebIDL/Promise.h @@ -29,6 +29,6 @@ JS::NonnullGCPtr react_to_promise(Promise const&, Optional upon_fulfillment(Promise const&, ReactionSteps); JS::NonnullGCPtr upon_rejection(Promise const&, ReactionSteps); void mark_promise_as_handled(Promise const&); -void wait_for_all(JS::Realm&, JS::MarkedVector> const& promises, JS::SafeFunction const&)> success_steps, JS::SafeFunction failure_steps); +void wait_for_all(JS::Realm&, Vector> const& promises, Function const&)> success_steps, Function failure_steps); }