From 1032ac2453e7c276152db1a5c9f9a55b377d7419 Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Fri, 3 Mar 2023 18:02:43 +0000 Subject: LibWeb/Fetch: Store Response error message as a String{,View} variant The majority of error strings are StringView literals, and it seems silly to require heap-allocating strings for these. This idea is stolen from a recent change in fd1fbad :^) --- Userland/Libraries/LibWeb/Fetch/FetchMethod.cpp | 4 +- .../Libraries/LibWeb/Fetch/Fetching/Fetching.cpp | 44 +++++++++++----------- .../LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp | 22 +++++++---- .../LibWeb/Fetch/Infrastructure/HTTP/Responses.h | 10 ++--- Userland/Libraries/LibWeb/Fetch/Response.cpp | 2 +- 5 files changed, 45 insertions(+), 37 deletions(-) (limited to 'Userland/Libraries/LibWeb/Fetch') diff --git a/Userland/Libraries/LibWeb/Fetch/FetchMethod.cpp b/Userland/Libraries/LibWeb/Fetch/FetchMethod.cpp index 1fd1e04b1e..ed4fa016ae 100644 --- a/Userland/Libraries/LibWeb/Fetch/FetchMethod.cpp +++ b/Userland/Libraries/LibWeb/Fetch/FetchMethod.cpp @@ -99,8 +99,8 @@ JS::NonnullGCPtr fetch_impl(JS::VM& vm, RequestInfo const& input, R // 3. If response is a network error, then reject p with a TypeError and abort these steps. if (response->is_network_error()) { - auto message = response->network_error_message().value_or("Response is a network error"_string.release_value_but_fixme_should_propagate_errors()); - WebIDL::reject_promise(vm, promise_capability, JS::TypeError::create(relevant_realm, move(message))); + auto message = response->network_error_message().value_or("Response is a network error"sv); + WebIDL::reject_promise(vm, promise_capability, JS::TypeError::create(relevant_realm, message).release_allocated_value_but_fixme_should_propagate_errors()); return; } diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp index 17b1636c19..b30960cf30 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp @@ -214,7 +214,7 @@ WebIDL::ExceptionOr>> main_fetch(JS:: // 3. If request’s local-URLs-only flag is set and request’s current URL is not local, then set response to a // network error. if (request->local_urls_only() && !Infrastructure::is_local_url(request->current_url())) - response = Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'local-URLs-only' flag must have a local URL"_string)); + response = Infrastructure::Response::network_error(vm, "Request with 'local-URLs-only' flag must have a local URL"sv); // FIXME: 4. Run report Content Security Policy violations for request. // FIXME: 5. Upgrade request to a potentially trustworthy URL, if appropriate. @@ -225,7 +225,7 @@ WebIDL::ExceptionOr>> main_fetch(JS:: || false // FIXME: "should fetching request be blocked as mixed content" || false // FIXME: "should request be blocked by Content Security Policy returns blocked" ) { - response = Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request was blocked"_string)); + response = Infrastructure::Response::network_error(vm, "Request was blocked"sv); } // 7. If request’s referrer policy is the empty string, then set request’s referrer policy to request’s policy @@ -300,13 +300,13 @@ WebIDL::ExceptionOr>> main_fetch(JS:: // -> request’s mode is "same-origin" else if (request->mode() == Infrastructure::Request::Mode::SameOrigin) { // Return a network error. - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'same-origin' mode must have same URL and request origin"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'same-origin' mode must have same URL and request origin"sv)); } // -> request’s mode is "no-cors" else if (request->mode() == Infrastructure::Request::Mode::NoCORS) { // 1. If request’s redirect mode is not "follow", then return a network error. if (request->redirect_mode() != Infrastructure::Request::RedirectMode::Follow) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'no-cors' mode must have redirect mode set to 'follow'"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'no-cors' mode must have redirect mode set to 'follow'"sv)); // 2. Set request’s response tainting to "opaque". request->set_response_tainting(Infrastructure::Request::ResponseTainting::Opaque); @@ -320,7 +320,7 @@ WebIDL::ExceptionOr>> main_fetch(JS:: VERIFY(request->mode() == Infrastructure::Request::Mode::CORS); // Return a network error. - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'cors' mode must have URL with HTTP or HTTPS scheme"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'cors' mode must have URL with HTTP or HTTPS scheme"sv)); } // -> request’s use-CORS-preflight flag is set // -> request’s unsafe-request flag is set and either request’s method is not a CORS-safelisted method or @@ -479,7 +479,7 @@ WebIDL::ExceptionOr>> main_fetch(JS:: // 1. Let processBodyError be this step: run fetch response handover given fetchParams and a network // error. auto process_body_error = [&]() -> WebIDL::ExceptionOr { - return fetch_response_handover(realm, fetch_params, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Response body could not be processed"_string))); + return 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. @@ -668,7 +668,7 @@ WebIDL::ExceptionOr> scheme_fetch(JS::Realm& r // 1. If fetchParams is canceled, then return the appropriate network error for fetchParams. if (fetch_params.is_canceled()) - return PendingResponse::create(vm, fetch_params.request(), TRY_OR_THROW_OOM(vm, Infrastructure::Response::appropriate_network_error(vm, fetch_params))); + return PendingResponse::create(vm, fetch_params.request(), Infrastructure::Response::appropriate_network_error(vm, fetch_params)); // 2. Let request be fetchParams’s request. auto request = fetch_params.request(); @@ -693,7 +693,7 @@ WebIDL::ExceptionOr> scheme_fetch(JS::Realm& r // -> "blob" else if (request->current_url().scheme() == "blob"sv) { // FIXME: Support 'blob://' URLs - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request has 'blob:' URL which is currently unsupported"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request has 'blob:' URL which is currently unsupported"sv)); } // -> "data" else if (request->current_url().scheme() == "data"sv) { @@ -705,7 +705,7 @@ WebIDL::ExceptionOr> scheme_fetch(JS::Realm& r // 2. If dataURLStruct is failure, then return a network error. if (data_or_error.is_error()) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request has invalid base64 'data:' URL"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request has invalid base64 'data:' URL"sv)); // 3. Let mimeType be dataURLStruct’s MIME type, serialized. auto const& mime_type = url.data_mime_type(); @@ -724,7 +724,7 @@ WebIDL::ExceptionOr> scheme_fetch(JS::Realm& r // For now, unfortunate as it is, file: URLs are left as an exercise for the reader. // When in doubt, return a network error. // FIXME: Support 'file://' URLs - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request has 'file:' URL which is currently unsupported"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request has 'file:' URL which is currently unsupported"sv)); } // -> HTTP(S) scheme else if (Infrastructure::is_http_or_https_scheme(request->current_url().scheme())) { @@ -806,7 +806,7 @@ WebIDL::ExceptionOr> http_fetch(JS::Realm& rea // - request’s redirect mode is not "follow" and response’s URL list has more than one item. || (request->redirect_mode() != Infrastructure::Request::RedirectMode::Follow && response->url_list().size() > 1)) { // then return a network error. - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Invalid request/response state combination"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Invalid request/response state combination"sv)); } } } @@ -984,17 +984,17 @@ WebIDL::ExceptionOr> http_redirect_fetch(JS::R // 5. If locationURL is failure, then return a network error. if (location_url_or_error.is_error()) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request redirect URL is invalid"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request redirect URL is invalid"sv)); auto location_url = location_url_or_error.release_value().release_value(); // 6. If locationURL’s scheme is not an HTTP(S) scheme, then return a network error. if (!Infrastructure::is_http_or_https_scheme(location_url.scheme())) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request redirect URL must have HTTP or HTTPS scheme"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request redirect URL must have HTTP or HTTPS scheme"sv)); // 7. If request’s redirect count is 20, then return a network error. if (request->redirect_count() == 20) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request has reached maximum redirect count of 20"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request has reached maximum redirect count of 20"sv)); // 8. Increase request’s redirect count by 1. request->set_redirect_count(request->redirect_count() + 1); @@ -1005,20 +1005,20 @@ WebIDL::ExceptionOr> http_redirect_fetch(JS::R && location_url.includes_credentials() && request->origin().has() && !request->origin().get().is_same_origin(URL::url_origin(location_url))) { - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'cors' mode and different URL and request origin must not include credentials in redirect URL"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'cors' mode and different URL and request origin must not include credentials in redirect URL"sv)); } // 10. If request’s response tainting is "cors" and locationURL includes credentials, then return a network error. // NOTE: This catches a cross-origin resource redirecting to a same-origin URL. if (request->response_tainting() == Infrastructure::Request::ResponseTainting::CORS && location_url.includes_credentials()) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'cors' response tainting must not include credentials in redirect URL"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'cors' response tainting must not include credentials in redirect URL"sv)); // 11. If actualResponse’s status is not 303, request’s body is non-null, and request’s body’s source is null, then // return a network error. if (actual_response->status() != 303 && !request->body().has() && request->body().get().source().has()) { - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request has body but no body source"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request has body but no body source"sv)); } // 12. If one of the following is true @@ -1380,7 +1380,7 @@ WebIDL::ExceptionOr> http_network_or_cache_fet // 9. If aborted, then return the appropriate network error for fetchParams. if (aborted) - return PendingResponse::create(vm, request, TRY_OR_THROW_OOM(vm, Infrastructure::Response::appropriate_network_error(vm, fetch_params))); + return PendingResponse::create(vm, request, Infrastructure::Response::appropriate_network_error(vm, fetch_params)); JS::GCPtr pending_forward_response; @@ -1388,7 +1388,7 @@ WebIDL::ExceptionOr> http_network_or_cache_fet if (!response) { // 1. If httpRequest’s cache mode is "only-if-cached", then return a network error. if (http_request->cache_mode() == Infrastructure::Request::CacheMode::OnlyIfCached) - return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Request with 'only-if-cached' cache mode doesn't have a cached response"_string))); + return PendingResponse::create(vm, request, Infrastructure::Response::network_error(vm, "Request with 'only-if-cached' cache mode doesn't have a cached response"sv)); // 2. Let forwardResponse be the result of running HTTP-network fetch given httpFetchParams, includeCredentials, // and isNewConnectionFetch. @@ -1485,7 +1485,7 @@ WebIDL::ExceptionOr> http_network_or_cache_fet if (!request->use_url_credentials() || is_authentication_fetch == IsAuthenticationFetch::Yes) { // 1. If fetchParams is canceled, then return the appropriate network error for fetchParams. if (fetch_params.is_canceled()) { - returned_pending_response->resolve(TRY_OR_IGNORE(Infrastructure::Response::appropriate_network_error(vm, fetch_params))); + returned_pending_response->resolve(Infrastructure::Response::appropriate_network_error(vm, fetch_params)); return; } @@ -1522,7 +1522,7 @@ WebIDL::ExceptionOr> http_network_or_cache_fet // 3. If fetchParams is canceled, then return the appropriate network error for fetchParams. if (fetch_params.is_canceled()) { - returned_pending_response->resolve(TRY_OR_IGNORE(Infrastructure::Response::appropriate_network_error(vm, fetch_params))); + returned_pending_response->resolve(Infrastructure::Response::appropriate_network_error(vm, fetch_params)); return; } @@ -1548,7 +1548,7 @@ WebIDL::ExceptionOr> http_network_or_cache_fet ) { // 1. If fetchParams is canceled, then return the appropriate network error for fetchParams. if (fetch_params.is_canceled()) { - returned_pending_response->resolve(TRY_OR_IGNORE(Infrastructure::Response::appropriate_network_error(vm, fetch_params))); + returned_pending_response->resolve(Infrastructure::Response::appropriate_network_error(vm, fetch_params)); return; } // 2. Set response to the result of running HTTP-network-or-cache fetch given fetchParams, diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp index a0bec94dbf..f7511be576 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.cpp @@ -37,16 +37,16 @@ JS::NonnullGCPtr Response::create(JS::VM& vm) // A network error is a response whose status is always 0, status message is always // the empty byte sequence, header list is always empty, and body is always null. -ErrorOr> Response::aborted_network_error(JS::VM& vm) +JS::NonnullGCPtr Response::aborted_network_error(JS::VM& vm) { - auto response = network_error(vm, TRY("Fetch has been aborted"_string)); + auto response = network_error(vm, "Fetch has been aborted"sv); response->set_aborted(true); return response; } -JS::NonnullGCPtr Response::network_error(JS::VM& vm, String message) +JS::NonnullGCPtr Response::network_error(JS::VM& vm, Variant message) { - dbgln_if(WEB_FETCH_DEBUG, "Fetch: Creating network error response with message: {}", message); + dbgln_if(WEB_FETCH_DEBUG, "Fetch: Creating network error response with message: {}", message.visit([](auto const& s) -> StringView { return s; })); auto response = Response::create(vm); response->set_status(0); response->set_type(Type::Error); @@ -56,15 +56,15 @@ JS::NonnullGCPtr Response::network_error(JS::VM& vm, String message) } // https://fetch.spec.whatwg.org/#appropriate-network-error -ErrorOr> Response::appropriate_network_error(JS::VM& vm, FetchParams const& fetch_params) +JS::NonnullGCPtr Response::appropriate_network_error(JS::VM& vm, FetchParams const& fetch_params) { // 1. Assert: fetchParams is canceled. VERIFY(fetch_params.is_canceled()); // 2. Return an aborted network error if fetchParams is aborted; otherwise return a network error. return fetch_params.is_aborted() - ? TRY(aborted_network_error(vm)) - : network_error(vm, TRY("Fetch has been terminated"_string)); + ? aborted_network_error(vm) + : network_error(vm, "Fetch has been terminated"sv); } // https://fetch.spec.whatwg.org/#concept-aborted-network-error @@ -170,6 +170,14 @@ WebIDL::ExceptionOr> Response::clone(JS::VM& vm) cons return new_response; } +// Non-standard +Optional Response::network_error_message() const +{ + if (!m_network_error_message.has_value()) + return {}; + return m_network_error_message->visit([](auto const& s) -> StringView { return s; }); +} + FilteredResponse::FilteredResponse(JS::NonnullGCPtr internal_response, JS::NonnullGCPtr header_list) : Response(header_list) , m_internal_response(internal_response) diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.h b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.h index 4ed881d88f..1738b2ed55 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.h +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Responses.h @@ -50,9 +50,9 @@ public: }; [[nodiscard]] static JS::NonnullGCPtr create(JS::VM&); - static ErrorOr> aborted_network_error(JS::VM&); - [[nodiscard]] static JS::NonnullGCPtr network_error(JS::VM&, String message); - static ErrorOr> appropriate_network_error(JS::VM&, FetchParams const&); + [[nodiscard]] static JS::NonnullGCPtr aborted_network_error(JS::VM&); + [[nodiscard]] static JS::NonnullGCPtr network_error(JS::VM&, Variant message); + [[nodiscard]] static JS::NonnullGCPtr appropriate_network_error(JS::VM&, FetchParams const&); virtual ~Response() = default; @@ -109,7 +109,7 @@ public: [[nodiscard]] WebIDL::ExceptionOr> clone(JS::VM&) const; // Non-standard - Optional const& network_error_message() const { return m_network_error_message; } + [[nodiscard]] Optional network_error_message() const; protected: explicit Response(JS::NonnullGCPtr); @@ -177,7 +177,7 @@ private: bool m_has_cross_origin_redirects { false }; // Non-standard - Optional m_network_error_message; + Optional> m_network_error_message; }; // https://fetch.spec.whatwg.org/#concept-filtered-response diff --git a/Userland/Libraries/LibWeb/Fetch/Response.cpp b/Userland/Libraries/LibWeb/Fetch/Response.cpp index 1d4698dff2..0173679bdb 100644 --- a/Userland/Libraries/LibWeb/Fetch/Response.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Response.cpp @@ -164,7 +164,7 @@ WebIDL::ExceptionOr> Response::error(JS::VM& vm) { // The static error() method steps are to return the result of creating a Response object, given a new network error, "immutable", and this’s relevant Realm. // FIXME: How can we reliably get 'this', i.e. the object the function was called on, in IDL-defined functions? - return Response::create(*vm.current_realm(), Infrastructure::Response::network_error(vm, TRY_OR_THROW_OOM(vm, "Response created via `Response.error()`"_string)), Headers::Guard::Immutable); + return Response::create(*vm.current_realm(), Infrastructure::Response::network_error(vm, "Response created via `Response.error()`"sv), Headers::Guard::Immutable); } // https://fetch.spec.whatwg.org/#dom-response-redirect -- cgit v1.2.3