Browse Source

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.
Timothy Flynn 1 year ago
parent
commit
1ffda6a805

+ 9 - 10
Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp

@@ -98,11 +98,10 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> 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<JS::NonnullGCPtr<DOM::Document>> 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<JS::NonnullGCPtr<DOM::Document>> 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<JS::NonnullGCPtr<DOM::Document>> 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<JS::NonnullGCPtr<DOM::Document>> 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<WebIDL::DOMException>) {}),
-        JS::NonnullGCPtr { realm.global_object() }));
+        JS::NonnullGCPtr { realm.global_object() });
 
     // 9. Return document.
     return document;

+ 1 - 1
Userland/Libraries/LibWeb/Fetch/Body.cpp

@@ -201,7 +201,7 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<JS::Promise>> 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.

+ 6 - 8
Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp

@@ -495,7 +495,7 @@ WebIDL::ExceptionOr<JS::GCPtr<PendingResponse>> 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<WebIDL::DOMException>) {
-                    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<JS::GCPtr<PendingResponse>> 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<JS::GCPtr<PendingResponse>> main_fetch(JS::Realm& realm, Inf
 }
 
 // https://fetch.spec.whatwg.org/#fetch-finale
-WebIDL::ExceptionOr<void> 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<void> 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

+ 1 - 1
Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.h

@@ -31,7 +31,7 @@ ENUMERATE_BOOL_PARAMS
 
 WebIDL::ExceptionOr<JS::NonnullGCPtr<Infrastructure::FetchController>> fetch(JS::Realm&, Infrastructure::Request&, Infrastructure::FetchAlgorithms const&, UseParallelQueue use_parallel_queue = UseParallelQueue::No);
 WebIDL::ExceptionOr<JS::GCPtr<PendingResponse>> main_fetch(JS::Realm&, Infrastructure::FetchParams const&, Recursive recursive = Recursive::No);
-WebIDL::ExceptionOr<void> fetch_response_handover(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&);
+void fetch_response_handover(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&);
 WebIDL::ExceptionOr<JS::NonnullGCPtr<PendingResponse>> scheme_fetch(JS::Realm&, Infrastructure::FetchParams const&);
 WebIDL::ExceptionOr<JS::NonnullGCPtr<PendingResponse>> http_fetch(JS::Realm&, Infrastructure::FetchParams const&, MakeCORSPreflight make_cors_preflight = MakeCORSPreflight::No);
 WebIDL::ExceptionOr<JS::GCPtr<PendingResponse>> http_redirect_fetch(JS::Realm&, Infrastructure::FetchParams const&, Infrastructure::Response&);

+ 17 - 17
Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp

@@ -62,17 +62,15 @@ JS::NonnullGCPtr<Body> Body::clone(JS::Realm& realm)
 }
 
 // https://fetch.spec.whatwg.org/#body-fully-read
-WebIDL::ExceptionOr<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
+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<Empty>());
     auto task_destination_object = task_destination.get<JS::NonnullGCPtr<JS::Object>>();
 
     // 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<void> {
+    auto success_steps = [&realm, process_body, task_destination_object = task_destination_object](ReadonlyBytes bytes) -> ErrorOr<void> {
         // 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<void> 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<WebIDL::DOMException> 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<WebIDL::DOMException> 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<ByteBuffer>()) {
-        TRY_OR_THROW_OOM(vm, success_steps(*byte_buffer));
-    } else if (auto const* blob_handle = m_source.get_pointer<JS::Handle<FileAPI::Blob>>()) {
-        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<FileAPI::Blob> 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

+ 1 - 1
Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h

@@ -41,7 +41,7 @@ public:
 
     [[nodiscard]] JS::NonnullGCPtr<Body> clone(JS::Realm&);
 
-    WebIDL::ExceptionOr<void> 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;
 

+ 1 - 1
Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp

@@ -539,7 +539,7 @@ WebIDL::ExceptionOr<void> 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))));

+ 1 - 1
Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp

@@ -1016,7 +1016,7 @@ WebIDL::ExceptionOr<void> 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))));

+ 1 - 1
Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp

@@ -204,7 +204,7 @@ WebIDL::ExceptionOr<void> HTMLVideoElement::determine_element_poster_frame(Optio
         VERIFY(response->body());
         auto empty_algorithm = JS::create_heap_function(heap(), [](JS::GCPtr<WebIDL::DOMException>) {});
 
-        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))));

+ 1 - 1
Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp

@@ -95,7 +95,7 @@ void SharedImageRequest::fetch_image(JS::Realm& realm, JS::NonnullGCPtr<Fetch::I
         });
 
         if (response->body())
-            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();
     };