system/serenity
system/serenity
1
0
Fork 0
mirror of https://github.com/SerenityOS/serenity synced 2024-07-24 03:25:02 +00:00
Code Issues Actions 4 Packages Projects Releases Wiki Activity

LibWeb: Fix GC leaks in Fetch::Infrastructure::Body::fully_read()

Browse source 
By making this function accept the success and error steps as
HeapFunction rather than SafeFunction, we break a bunch of strong
GC cycles.
This commit is contained in:
Andreas Kling 2024-04-23 10:25:20 +02:00
parent 0c84508f87
commit 184368285c
9 changed files with 67 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

46
Userland/Libraries/LibWeb/DOM/DocumentLoading.cpp
View file

@ -84,23 +84,23 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_markdown_docume
return create_document_for_inline_content(navigation_params.navigable.ptr(), navigation_params.id, [&](DOM::Document& document) {
auto& realm = document.realm();
auto process_body = [&document, url = navigation_params.response->url().value(), extra_head_contents](ByteBuffer data) {
auto process_body = JS::create_heap_function(realm.heap(), [&document, url = navigation_params.response->url().value(), extra_head_contents](ByteBuffer data) {
auto markdown_document = Markdown::Document::parse(data);
if (!markdown_document)
return;
auto parser = HTML::HTMLParser::create(document, markdown_document->render_to_html(extra_head_contents), "utf-8"sv);
parser->run(url);
};
});
auto process_body_error = [](auto) {
auto process_body_error = JS::create_heap_function(realm.heap(), [](JS::GCPtr<WebIDL::DOMException>) {
dbgln("FIXME: Load html page with an error if read of body failed.");
};
});
navigation_params.response->body()->fully_read(
realm,
move(process_body),
move(process_body_error),
process_body,
process_body_error,
JS::NonnullGCPtr { realm.global_object() })
.release_value_but_fixme_should_propagate_errors();
});
@ -162,19 +162,19 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_html_document(H
// causes a load event to be fired.
else {
// FIXME: Parse as we receive the document data, instead of waiting for the whole document to be fetched first.
auto process_body = [document, url = navigation_params.response->url().value()](ByteBuffer data) {
auto process_body = JS::create_heap_function(document->heap(), [document, url = navigation_params.response->url().value()](ByteBuffer data) {
Platform::EventLoopPlugin::the().deferred_invoke([document = document, data = move(data), url = url] {
auto parser = HTML::HTMLParser::create_with_uncertain_encoding(document, data);
parser->run(url);
});
};
});
auto process_body_error = [](auto) {
auto process_body_error = JS::create_heap_function(document->heap(), [](JS::GCPtr<WebIDL::DOMException>) {
dbgln("FIXME: Load html page with an error if read of body failed.");
};
});
auto& realm = document->realm();
TRY(navigation_params.response->body()->fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }));
TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }));
}
// 4. Return document.
@ -217,7 +217,7 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_xml_document(HT
if (auto maybe_encoding = type.parameters().get("charset"sv); maybe_encoding.has_value())
content_encoding = maybe_encoding.value();
auto process_body = [document, url = navigation_params.response->url().value(), content_encoding = move(content_encoding)](ByteBuffer data) {
auto process_body = JS::create_heap_function(document->heap(), [document, url = navigation_params.response->url().value(), content_encoding = move(content_encoding)](ByteBuffer data) {
Optional<TextCodec::Decoder&> decoder;
// The actual HTTP headers and other metadata, not the headers as mutated or implied by the algorithms given in this specification,
// are the ones that must be used when determining the character encoding according to the rules given in the above specifications.
@ -258,14 +258,14 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_xml_document(HT
// NOTE: XMLDocumentBuilder ensures that the `load` event gets fired. We don't need to do anything else here.
}
};
});
auto process_body_error = [](auto) {
auto process_body_error = JS::create_heap_function(document->heap(), [](JS::GCPtr<WebIDL::DOMException>) {
dbgln("FIXME: Load html page with an error if read of body failed.");
};
});
auto& realm = document->realm();
TRY(navigation_params.response->body()->fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }));
TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }));
return document;
}
@ -297,7 +297,7 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_text_document(H
// document's relevant global object to have the parser to process the implied EOF character, which eventually causes a
// load event to be fired.
// FIXME: Parse as we receive the document data, instead of waiting for the whole document to be fetched first.
auto process_body = [document, url = navigation_params.response->url().value()](ByteBuffer data) {
auto process_body = JS::create_heap_function(document->heap(), [document, url = navigation_params.response->url().value()](ByteBuffer data) {
auto encoding = run_encoding_sniffing_algorithm(document, data);
dbgln_if(HTML_PARSER_DEBUG, "The encoding sniffing algorithm returned encoding '{}'", encoding);
@ -321,14 +321,14 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_text_document(H
MUST(document->head()->append_child(title_element));
auto title_text = document->heap().allocate<DOM::Text>(document->realm(), document, title);
MUST(title_element->append_child(*title_text));
};
});
auto process_body_error = [](auto) {
auto process_body_error = JS::create_heap_function(document->heap(), [](JS::GCPtr<WebIDL::DOMException>) {
dbgln("FIXME: Load html page with an error if read of body failed.");
};
});
auto& realm = document->realm();
TRY(navigation_params.response->body()->fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }));
TRY(navigation_params.response->body()->fully_read(realm, process_body, process_body_error, JS::NonnullGCPtr { realm.global_object() }));
// 6. Return document.
return document;
@ -418,8 +418,8 @@ static WebIDL::ExceptionOr<JS::NonnullGCPtr<DOM::Document>> load_media_document(
auto& realm = document->realm();
TRY(navigation_params.response->body()->fully_read(
realm,
[document](auto) { HTML::HTMLParser::the_end(document); },
[](auto) {},
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() }));
// 9. Return document.

12
Userland/Libraries/LibWeb/Fetch/Body.cpp
View file

@ -164,17 +164,17 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<JS::Promise>> consume_body(JS::Realm& realm
// 3. Let errorSteps given error be to reject promise with error.
// NOTE: `promise` and `realm` is protected by JS::SafeFunction.
auto error_steps = [promise, &realm](JS::GCPtr<WebIDL::DOMException> error) {
auto error_steps = JS::create_heap_function(realm.heap(), [promise, &realm](JS::GCPtr<WebIDL::DOMException> error) {
// AD-HOC: An execution context is required for Promise's reject function.
HTML::TemporaryExecutionContext execution_context { Bindings::host_defined_environment_settings_object(realm) };
WebIDL::reject_promise(realm, promise, error);
};
});
// 4. Let successSteps given a byte sequence data be to resolve promise with the result of running convertBytesToJSValue
// with data. If that threw an exception, then run errorSteps with that exception.
// NOTE: `promise`, `realm` and `object` is protected by JS::SafeFunction.
// FIXME: Refactor this to the new version of the spec introduced with https://github.com/whatwg/fetch/commit/464326e8eb6a602122c030cd40042480a3c0e265
auto success_steps = [promise, &realm, &object, type](ByteBuffer const& data) {
auto success_steps = JS::create_heap_function(realm.heap(), [promise, &realm, &object, type](ByteBuffer data) {
auto& vm = realm.vm();
// AD-HOC: An execution context is required for Promise's reject function and JSON.parse.
@ -192,16 +192,16 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<JS::Promise>> consume_body(JS::Realm& realm
}
WebIDL::resolve_promise(realm, promise, value_or_error.release_value());
};
});
// 5. If object’s body is null, then run successSteps with an empty byte sequence.
auto const& body = object.body_impl();
if (!body) {
success_steps(ByteBuffer {});
success_steps->function()(ByteBuffer {});
}
// 6. Otherwise, fully read object’s body given successSteps, errorSteps, and object’s relevant global object.
else {
TRY(body->fully_read(realm, move(success_steps), move(error_steps), JS::NonnullGCPtr { HTML::relevant_global_object(object.as_platform_object()) }));
TRY(body->fully_read(realm, success_steps, error_steps, JS::NonnullGCPtr { HTML::relevant_global_object(object.as_platform_object()) }));
}
// 7. Return promise.

24
Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp
View file

@ -494,21 +494,21 @@ WebIDL::ExceptionOr<JS::GCPtr<PendingResponse>> main_fetch(JS::Realm& realm, Inf
if (!request->integrity_metadata().is_empty()) {
// 1. Let processBodyError be this step: run fetch response handover given fetchParams and a network
// error.
Infrastructure::Body::ProcessBodyErrorCallback process_body_error = [&realm, &vm, &fetch_params](auto) {
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)));
};
});
// 2. If response’s body is null, then run processBodyError and abort these steps.
if (!response->body()) {
process_body_error({});
process_body_error->function()({});
return;
}
// 3. Let processBody given bytes be these steps:
Infrastructure::Body::ProcessBodyCallback process_body = [&realm, request, response, &fetch_params, process_body_error = move(process_body_error)](ByteBuffer bytes) {
auto process_body = JS::create_heap_function(vm.heap(), [&realm, request, response, &fetch_params, process_body_error = move(process_body_error)](ByteBuffer bytes) {
// 1. If bytes do not match request’s integrity metadata, then run processBodyError and abort these steps.
if (!TRY_OR_IGNORE(SRI::do_bytes_match_metadata_list(bytes, request->integrity_metadata()))) {
process_body_error({});
process_body_error->function()({});
return;
}
@ -517,7 +517,7 @@ WebIDL::ExceptionOr<JS::GCPtr<PendingResponse>> main_fetch(JS::Realm& realm, Inf
// 3. Run fetch response handover given fetchParams and response.
TRY_OR_IGNORE(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()));
@ -661,27 +661,27 @@ WebIDL::ExceptionOr<void> fetch_response_handover(JS::Realm& realm, Infrastructu
if (fetch_params.algorithms()->process_response_consume_body()) {
// 1. Let processBody given nullOrBytes be this step: run fetchParams’s process response consume body given
// response and nullOrBytes.
auto process_body = [&fetch_params, &response](Variant<ByteBuffer, Empty> const& null_or_bytes) {
auto process_body = JS::create_heap_function(vm.heap(), [&fetch_params, &response](ByteBuffer null_or_bytes) {
(fetch_params.algorithms()->process_response_consume_body())(response, null_or_bytes);
};
});
// 2. Let processBodyError be this step: run fetchParams’s process response consume body given response and
// failure.
auto process_body_error = [&fetch_params, &response](auto) {
auto process_body_error = JS::create_heap_function(vm.heap(), [&fetch_params, &response](JS::GCPtr<WebIDL::DOMException>) {
(fetch_params.algorithms()->process_response_consume_body())(response, Infrastructure::FetchAlgorithms::ConsumeBodyFailureTag {});
};
});
// 3. If internalResponse's body is null, then queue a fetch task to run processBody given null, with
// fetchParams’s task destination.
if (!internal_response->body()) {
Infrastructure::queue_fetch_task(fetch_params.controller(), task_destination, JS::create_heap_function(vm.heap(), [process_body = move(process_body)]() {
process_body({});
process_body->function()({});
}));
}
// 4. Otherwise, fully read internalResponse body given processBody, processBodyError, and fetchParams’s task
// destination.
else {
TRY(internal_response->body()->fully_read(realm, move(process_body), move(process_body_error), fetch_params.task_destination()));
TRY(internal_response->body()->fully_read(realm, process_body, process_body_error, fetch_params.task_destination()));
}
}

12
Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.cpp
View file

@ -72,19 +72,19 @@ WebIDL::ExceptionOr<void> Body::fully_read(JS::Realm& realm, Web::Fetch::Infrast
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 = move(process_body), task_destination_object = JS::make_handle(task_destination_object)](ByteBuffer const& bytes) mutable -> ErrorOr<void> {
auto success_steps = [&realm, process_body, task_destination_object = task_destination_object](ByteBuffer const& bytes) mutable -> 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 = move(process_body), bytes_copy = move(bytes_copy)]() {
process_body(move(bytes_copy));
queue_fetch_task(*task_destination_object, JS::create_heap_function(realm.heap(), [process_body, bytes_copy = move(bytes_copy)]() {
process_body->function()(move(bytes_copy));
}));
return {};
};
// 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 = move(process_body_error), task_destination_object = JS::make_handle(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 = JS::make_handle(exception)]() {
process_body_error(*exception);
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]() {
process_body_error->function()(*exception);
}));
};

4
Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Bodies.h
View file

@ -28,9 +28,9 @@ class Body final : public JS::Cell {
public:
using SourceType = Variant<Empty, ByteBuffer, JS::Handle<FileAPI::Blob>>;
// processBody must be an algorithm accepting a byte sequence.
using ProcessBodyCallback = JS::SafeFunction<void(ByteBuffer)>;
using ProcessBodyCallback = JS::NonnullGCPtr<JS::HeapFunction<void(ByteBuffer)>>;
// processBodyError must be an algorithm optionally accepting an exception.
using ProcessBodyErrorCallback = JS::SafeFunction<void(JS::GCPtr<WebIDL::DOMException>)>;
using ProcessBodyErrorCallback = JS::NonnullGCPtr<JS::HeapFunction<void(JS::GCPtr<WebIDL::DOMException>)>>;
[[nodiscard]] static JS::NonnullGCPtr<Body> create(JS::VM&, JS::NonnullGCPtr<Streams::ReadableStream>);
[[nodiscard]] static JS::NonnullGCPtr<Body> create(JS::VM&, JS::NonnullGCPtr<Streams::ReadableStream>, SourceType, Optional<u64>);

10
Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp
View file

@ -518,15 +518,15 @@ WebIDL::ExceptionOr<void> HTMLLinkElement::load_fallback_favicon_if_needed(JS::N
auto& realm = document->realm();
auto global = JS::NonnullGCPtr { realm.global_object() };
auto process_body = [document, request](auto body) {
auto process_body = JS::create_heap_function(realm.heap(), [document, request](ByteBuffer body) {
decode_favicon(body, request->url(), document->navigable());
};
auto process_body_error = [](auto) {
};
});
auto process_body_error = JS::create_heap_function(realm.heap(), [](JS::GCPtr<WebIDL::DOMException>) {
});
// 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, move(process_body), move(process_body_error), global).release_value_but_fixme_should_propagate_errors();
body->fully_read(realm, process_body, process_body_error, global).release_value_but_fixme_should_propagate_errors();
};
TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))));

8
Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp
View file

@ -986,7 +986,7 @@ WebIDL::ExceptionOr<void> HTMLMediaElement::fetch_resource(URL::URL const& url_r
// 2. Let updateMedia be to queue a media element task given the media element to run the first appropriate steps from the media data processing
// steps list below. (A new task is used for this so that the work described below occurs relative to the appropriate media element event task
// source rather than using the networking task source.)
auto update_media = [this, failure_callback = move(failure_callback)](auto media_data) mutable {
auto update_media = JS::create_heap_function(heap(), [this, failure_callback = move(failure_callback)](ByteBuffer media_data) mutable {
// 6. Update the media data with the contents of response's unsafe response obtained in this fashion. response can be CORS-same-origin or
// CORS-cross-origin; this affects whether subtitles referenced in the media data are exposed in the API and, for video elements, whether
// a canvas gets tainted when the video is drawn on it.
@ -1005,7 +1005,7 @@ WebIDL::ExceptionOr<void> HTMLMediaElement::fetch_resource(URL::URL const& url_r
if (m_ready_state == ReadyState::HaveMetadata)
set_ready_state(ReadyState::HaveEnoughData);
});
};
});
// FIXME: 3. Let processEndOfMedia be the following step: If the fetching process has completes without errors, including decoding the media data,
// and if all of the data is available to the user agent without network access, then, the user agent must move on to the final step below.
@ -1015,12 +1015,12 @@ WebIDL::ExceptionOr<void> HTMLMediaElement::fetch_resource(URL::URL const& url_r
// 5. Otherwise, incrementally read response's body given updateMedia, processEndOfMedia, an empty algorithm, and global.
VERIFY(response->body());
auto empty_algorithm = [](auto) {};
auto empty_algorithm = JS::create_heap_function(heap(), [](JS::GCPtr<WebIDL::DOMException>) {});
// 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, move(update_media), move(empty_algorithm), JS::NonnullGCPtr { global }).release_value_but_fixme_should_propagate_errors();
response->body()->fully_read(realm, update_media, empty_algorithm, JS::NonnullGCPtr { global }).release_value_but_fixme_should_propagate_errors();
};
m_fetch_controller = TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))));

8
Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp
View file

@ -178,7 +178,7 @@ WebIDL::ExceptionOr<void> HTMLVideoElement::determine_element_poster_frame(Optio
response = filtered_response.internal_response();
}
auto on_image_data_read = [this](auto image_data) mutable {
auto on_image_data_read = JS::create_heap_function(heap(), [this](ByteBuffer image_data) mutable {
m_fetch_controller = nullptr;
// 6. If an image is thus obtained, the poster frame is that image. Otherwise, there is no poster frame.
@ -187,12 +187,12 @@ WebIDL::ExceptionOr<void> HTMLVideoElement::determine_element_poster_frame(Optio
return;
m_poster_frame = move(image.release_value().frames[0].bitmap);
};
});
VERIFY(response->body());
auto empty_algorithm = [](auto) {};
auto empty_algorithm = JS::create_heap_function(heap(), [](JS::GCPtr<WebIDL::DOMException>) {});
response->body()->fully_read(realm, move(on_image_data_read), move(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 }).release_value_but_fixme_should_propagate_errors();
};
m_fetch_controller = TRY(Fetch::Fetching::fetch(realm, request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))));

10
Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp
View file

@ -85,17 +85,17 @@ void SharedImageRequest::fetch_image(JS::Realm& realm, JS::NonnullGCPtr<Fetch::I
// https://github.com/whatwg/html/issues/9355
response = response->unsafe_response();
auto process_body = [this, request, response](ByteBuffer data) {
auto process_body = JS::create_heap_function(heap(), [this, request, response](ByteBuffer data) {
auto extracted_mime_type = response->header_list()->extract_mime_type().release_value_but_fixme_should_propagate_errors();
auto mime_type = extracted_mime_type.has_value() ? extracted_mime_type.value().essence().bytes_as_string_view() : StringView {};
handle_successful_fetch(request->url(), mime_type, move(data));
};
auto process_body_error = [this](auto) {
});
auto process_body_error = JS::create_heap_function(heap(), [this](JS::GCPtr<WebIDL::DOMException>) {
handle_failed_fetch();
};
});
if (response->body())
response->body()->fully_read(realm, move(process_body), move(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() }).release_value_but_fixme_should_propagate_errors();
else
handle_failed_fetch();
};