From 1ffda6a805c86c82817276b7695f3eacd8ae08ed Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 26 Apr 2024 14:57:40 -0400 Subject: [PATCH] LibWeb: Propagate OOM in Body::fully_read() through its error callback Fetched bodies can be on the order of gigabytes, so rather than crashing when we hit OOM here, we can simply invoke the error callback with a DOM exception. We use "UnknownError" here as the spec directly supports this for OOM errors: UnknownError: The operation failed for an unknown transient reason (e.g. out of memory). This is still an ad-hoc implementation. We should be using streams, and we do have the AOs available to do so. But they need to be massaged to be compatible with callers of Body::fully_read. And once we do use streams, this function will become infallible - so making it infallible here is at least a step in the right direction. --- .../Libraries/LibWeb/DOM/DocumentLoading.cpp | 19 +++++------ Userland/Libraries/LibWeb/Fetch/Body.cpp | 2 +- .../LibWeb/Fetch/Fetching/Fetching.cpp | 14 ++++---- .../LibWeb/Fetch/Fetching/Fetching.h | 2 +- .../Fetch/Infrastructure/HTTP/Bodies.cpp | 34 +++++++++---------- .../LibWeb/Fetch/Infrastructure/HTTP/Bodies.h | 2 +- .../Libraries/LibWeb/HTML/HTMLLinkElement.cpp | 2 +- .../LibWeb/HTML/HTMLMediaElement.cpp | 2 +- .../LibWeb/HTML/HTMLVideoElement.cpp | 2 +- .../LibWeb/HTML/SharedImageRequest.cpp | 2 +- 10 files changed, 39 insertions(+), 42 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp b/Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp index 941f21037a3..22dab53bf6e 100644 --- a/Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp +++ b/Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp @@ -98,11 +98,10 @@ static WebIDL::ExceptionOr> load_markdown_docume }); navigation_params.response->body()->fully_read( - realm, - process_body, - process_body_error, - JS::NonnullGCPtr { realm.global_object() }) - .release_value_but_fixme_should_propagate_errors(); + realm, + process_body, + process_body_error, + JS::NonnullGCPtr { realm.global_object() }); }); } @@ -174,7 +173,7 @@ static WebIDL::ExceptionOr> load_html_document(H }); auto& realm = document->realm(); - TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() })); + navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }); } // 4. Return document. @@ -265,7 +264,7 @@ static WebIDL::ExceptionOr> load_xml_document(HT }); auto& realm = document->realm(); - TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() })); + navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }); return document; } @@ -328,7 +327,7 @@ static WebIDL::ExceptionOr> load_text_document(H }); auto& realm = document->realm(); - TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() })); + navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }); // 6. Return document. return document; @@ -416,11 +415,11 @@ static WebIDL::ExceptionOr> load_media_document( // However, if we don't, then we get stuck in HTMLParser::the_end() waiting for the media file to load, which // never happens. auto& realm = document->realm(); - TRY(navigation_params.response->body()->fully_read( + navigation_params.response->body()->fully_read( realm, JS::create_heap_function(document->heap(), [document](ByteBuffer) { HTML::HTMLParser::the_end(document); }), JS::create_heap_function(document->heap(), [](JS::GCPtr) {}), - JS::NonnullGCPtr { realm.global_object() })); + JS::NonnullGCPtr { realm.global_object() }); // 9. Return document. return document; diff --git a/Userland/Libraries/LibWeb/Fetch/Body.cpp b/Userland/Libraries/LibWeb/Fetch/Body.cpp index 077d5e2517d..5c8c259e5d0 100644 --- a/Userland/Libraries/LibWeb/Fetch/Body.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Body.cpp @@ -201,7 +201,7 @@ WebIDL::ExceptionOr> consume_body(JS::Realm& realm } // 6. Otherwise, fully read object’s body given successSteps, errorSteps, and object’s relevant global object. else { - TRY(body->fully_read(realm, success_steps, error_steps, JS::NonnullGCPtr { HTML::relevant_global_object(object.as_platform_object()) })); + body->fully_read(realm, success_steps, error_steps, JS::NonnullGCPtr { HTML::relevant_global_object(object.as_platform_object()) }); } // 7. Return promise. diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp index 059cd7e85e2..adf7cacc937 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp @@ -495,7 +495,7 @@ WebIDL::ExceptionOr> main_fetch(JS::Realm& realm, Inf // 1. Let processBodyError be this step: run fetch response handover given fetchParams and a network // error. auto process_body_error = JS::create_heap_function(vm.heap(), [&realm, &vm, &fetch_params](JS::GCPtr) { - TRY_OR_IGNORE(fetch_response_handover(realm, fetch_params, Infrastructure::Response::network_error(vm, "Response body could not be processed"sv))); + fetch_response_handover(realm, fetch_params, Infrastructure::Response::network_error(vm, "Response body could not be processed"sv)); }); // 2. If response’s body is null, then run processBodyError and abort these steps. @@ -516,15 +516,15 @@ WebIDL::ExceptionOr> main_fetch(JS::Realm& realm, Inf response->set_body(TRY_OR_IGNORE(Infrastructure::byte_sequence_as_body(realm, bytes))); // 3. Run fetch response handover given fetchParams and response. - TRY_OR_IGNORE(fetch_response_handover(realm, fetch_params, *response)); + fetch_response_handover(realm, fetch_params, *response); }); // 4. Fully read response’s body given processBody and processBodyError. - TRY_OR_IGNORE(response->body()->fully_read(realm, move(process_body), move(process_body_error), fetch_params.task_destination())); + response->body()->fully_read(realm, process_body, process_body_error, fetch_params.task_destination()); } // 23. Otherwise, run fetch response handover given fetchParams and response. else { - TRY_OR_IGNORE(fetch_response_handover(realm, fetch_params, *response)); + fetch_response_handover(realm, fetch_params, *response); } }); }); @@ -533,7 +533,7 @@ WebIDL::ExceptionOr> main_fetch(JS::Realm& realm, Inf } // https://fetch.spec.whatwg.org/#fetch-finale -WebIDL::ExceptionOr fetch_response_handover(JS::Realm& realm, Infrastructure::FetchParams const& fetch_params, Infrastructure::Response& response) +void fetch_response_handover(JS::Realm& realm, Infrastructure::FetchParams const& fetch_params, Infrastructure::Response& response) { dbgln_if(WEB_FETCH_DEBUG, "Fetch: Running 'fetch response handover' with: fetch_params @ {}, response @ {}", &fetch_params, &response); @@ -681,11 +681,9 @@ WebIDL::ExceptionOr fetch_response_handover(JS::Realm& realm, Infrastructu // 4. Otherwise, fully read internalResponse body given processBody, processBodyError, and fetchParams’s task // destination. else { - TRY(internal_response->body()->fully_read(realm, process_body, process_body_error, fetch_params.task_destination())); + internal_response->body()->fully_read(realm, process_body, process_body_error, fetch_params.task_destination()); } } - - return {}; } // https://fetch.spec.whatwg.org/#concept-scheme-fetch diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.h b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.h index 0bd2e925093..641416c0666 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.h +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.h @@ -31,7 +31,7 @@ ENUMERATE_BOOL_PARAMS WebIDL::ExceptionOr> fetch(JS::Realm&, Infrastructure::Request&, Infrastructure::FetchAlgorithms const&, UseParallelQueue use_parallel_queue = UseParallelQueue::No); WebIDL::ExceptionOr> main_fetch(JS::Realm&, Infrastructure::FetchParams const&, Recursive recursive = Recursive::No); -WebIDL::ExceptionOr fetch_response_handover(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&); +void fetch_response_handover(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&); WebIDL::ExceptionOr> scheme_fetch(JS::Realm&, Infrastructure::FetchParams const&); WebIDL::ExceptionOr> http_fetch(JS::Realm&, Infrastructure::FetchParams const&, MakeCORSPreflight make_cors_preflight = MakeCORSPreflight::No); WebIDL::ExceptionOr> http_redirect_fetch(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&); diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp index 1363fb32c28..27d7f2033d7 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp @@ -62,17 +62,15 @@ JS::NonnullGCPtr Body::clone(JS::Realm& realm) } // https://fetch.spec.whatwg.org/#body-fully-read -WebIDL::ExceptionOr Body::fully_read(JS::Realm& realm, Web::Fetch::Infrastructure::Body::ProcessBodyCallback process_body, Web::Fetch::Infrastructure::Body::ProcessBodyErrorCallback process_body_error, TaskDestination task_destination) const +void Body::fully_read(JS::Realm& realm, Web::Fetch::Infrastructure::Body::ProcessBodyCallback process_body, Web::Fetch::Infrastructure::Body::ProcessBodyErrorCallback process_body_error, TaskDestination task_destination) const { - auto& vm = realm.vm(); - // FIXME: 1. If taskDestination is null, then set taskDestination to the result of starting a new parallel queue. // FIXME: Handle 'parallel queue' task destination VERIFY(!task_destination.has()); auto task_destination_object = task_destination.get>(); // 2. Let successSteps given a byte sequence bytes be to queue a fetch task to run processBody given bytes, with taskDestination. - auto success_steps = [&realm, process_body, task_destination_object = task_destination_object](ByteBuffer const& bytes) mutable -> ErrorOr { + auto success_steps = [&realm, process_body, task_destination_object = task_destination_object](ReadonlyBytes bytes) -> ErrorOr { // Make a copy of the bytes, as the source of the bytes may disappear between the time the task is queued and executed. auto bytes_copy = TRY(ByteBuffer::copy(bytes)); queue_fetch_task(*task_destination_object, JS::create_heap_function(realm.heap(), [process_body, bytes_copy = move(bytes_copy)]() mutable { @@ -82,25 +80,27 @@ WebIDL::ExceptionOr Body::fully_read(JS::Realm& realm, Web::Fetch::Infrast }; // 3. Let errorSteps optionally given an exception exception be to queue a fetch task to run processBodyError given exception, with taskDestination. - auto error_steps = [&realm, process_body_error, task_destination_object](JS::GCPtr exception) mutable { - queue_fetch_task(*task_destination_object, JS::create_heap_function(realm.heap(), [process_body_error = move(process_body_error), exception]() { + auto error_steps = [&realm, process_body_error, task_destination_object](JS::GCPtr exception) { + queue_fetch_task(*task_destination_object, JS::create_heap_function(realm.heap(), [process_body_error, exception]() { process_body_error->function()(*exception); })); }; // 4. Let reader be the result of getting a reader for body’s stream. If that threw an exception, then run errorSteps with that exception and return. // 5. Read all bytes from reader, given successSteps and errorSteps. - // FIXME: Implement the streams spec - this is completely made up for now :^) - if (auto const* byte_buffer = m_source.get_pointer()) { - TRY_OR_THROW_OOM(vm, success_steps(*byte_buffer)); - } else if (auto const* blob_handle = m_source.get_pointer>()) { - auto byte_buffer = TRY_OR_THROW_OOM(vm, ByteBuffer::copy((*blob_handle)->bytes())); - TRY_OR_THROW_OOM(vm, success_steps(move(byte_buffer))); - } else { - // Empty, Blob, FormData - error_steps(WebIDL::DOMException::create(realm, "DOMException"_fly_string, "Reading from Blob, FormData or null source is not yet implemented"_fly_string)); - } - return {}; + // FIXME: Use streams for these steps. + m_source.visit( + [&](ByteBuffer const& byte_buffer) { + if (auto result = success_steps(byte_buffer); result.is_error()) + error_steps(WebIDL::UnknownError::create(realm, "Out-of-memory"_fly_string)); + }, + [&](JS::Handle const& blob) { + if (auto result = success_steps(blob->bytes()); result.is_error()) + error_steps(WebIDL::UnknownError::create(realm, "Out-of-memory"_fly_string)); + }, + [&](Empty) { + error_steps(WebIDL::DOMException::create(realm, "DOMException"_fly_string, "Reading from Blob, FormData or null source is not yet implemented"_fly_string)); + }); } // https://fetch.spec.whatwg.org/#byte-sequence-as-a-body diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h index e3dbb09d17e..2b95957e6b7 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h @@ -41,7 +41,7 @@ public: [[nodiscard]] JS::NonnullGCPtr clone(JS::Realm&); - WebIDL::ExceptionOr fully_read(JS::Realm&, ProcessBodyCallback process_body, ProcessBodyErrorCallback process_body_error, TaskDestination task_destination) const; + void fully_read(JS::Realm&, ProcessBodyCallback process_body, ProcessBodyErrorCallback process_body_error, TaskDestination task_destination) const; virtual void visit_edges(JS::Cell::Visitor&) override; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp index 6e26bbe6d73..088cb1d2526 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp @@ -539,7 +539,7 @@ WebIDL::ExceptionOr HTMLLinkElement::load_fallback_favicon_if_needed(JS::N // 3. Use response's unsafe response as an icon as if it had been declared using the icon keyword. if (auto body = response->unsafe_response()->body()) - body->fully_read(realm, process_body, process_body_error, global).release_value_but_fixme_should_propagate_errors(); + body->fully_read(realm, process_body, process_body_error, global); }; TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input)))); diff --git a/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp index 380179ba406..6e6b92c19c3 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp @@ -1016,7 +1016,7 @@ WebIDL::ExceptionOr HTMLMediaElement::fetch_resource(URL::URL const& url_r // FIXME: We are "fully" reading the response here, rather than "incrementally". Memory concerns aside, this should be okay for now as we are // always setting byteRange to "entire resource". However, we should switch to incremental reads when that is implemented, and then // implement the processEndOfMedia step. - response->body()->fully_read(realm, update_media, empty_algorithm, JS::NonnullGCPtr { global }).release_value_but_fixme_should_propagate_errors(); + response->body()->fully_read(realm, update_media, empty_algorithm, JS::NonnullGCPtr { global }); }; m_fetch_controller = TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input)))); diff --git a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp index 414499dbc31..ce2a03103b0 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp @@ -204,7 +204,7 @@ WebIDL::ExceptionOr HTMLVideoElement::determine_element_poster_frame(Optio VERIFY(response->body()); auto empty_algorithm = JS::create_heap_function(heap(), [](JS::GCPtr) {}); - response->body()->fully_read(realm, on_image_data_read, empty_algorithm, JS::NonnullGCPtr { global }).release_value_but_fixme_should_propagate_errors(); + response->body()->fully_read(realm, on_image_data_read, empty_algorithm, JS::NonnullGCPtr { global }); }; m_fetch_controller = TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input)))); diff --git a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp index 92d16f25592..119a3845271 100644 --- a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp +++ b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp @@ -95,7 +95,7 @@ void SharedImageRequest::fetch_image(JS::Realm& realm, JS::NonnullGCPtrbody()) - response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }).release_value_but_fixme_should_propagate_errors(); + response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }); else handle_failed_fetch(); };