LibWeb: Use a separate class for shared image requests

As it turns out, making everyone piggyback on HTML::ImageRequest had
some major flaws, as HTMLImageElement may decide to abort an ongoing
fetch or wipe out image data, even when someone else is using the same
image request.

To avoid this issue, this patch introduces SharedImageRequest, and then
implements ImageRequest on top of that.

Other clients of the ImageRequest API are moved to SharedImageRequest
as well, and ImageRequest is now only used by HTMLImageElement.

This fixes an issue with image data disappearing and leading to asserts
and/or visually absent images.
This commit is contained in:
Andreas Kling 2023-06-14 12:45:56 +02:00
parent cdec23a68c
commit 34591ff3d9
Notes: sideshowbarker 2024-07-16 20:31:50 +09:00
11 changed files with 287 additions and 174 deletions

View File

@ -360,6 +360,7 @@ set(SOURCES
HTML/Scripting/Script.cpp
HTML/Scripting/WindowEnvironmentSettingsObject.cpp
HTML/SessionHistoryEntry.cpp
HTML/SharedImageRequest.cpp
HTML/SourceSet.cpp
HTML/Storage.cpp
HTML/StructuredSerialize.cpp

View File

@ -31,7 +31,7 @@ void ImageStyleValue::load_any_resources(DOM::Document& document)
return;
m_document = &document;
m_image_request = HTML::ImageRequest::get_shareable_or_create(*document.page(), m_url).release_value_but_fixme_should_propagate_errors();
m_image_request = HTML::SharedImageRequest::get_or_create(*document.page(), m_url).release_value_but_fixme_should_propagate_errors();
m_image_request->add_callbacks(
[this, weak_this = make_weak_ptr()] {
if (!weak_this)
@ -54,13 +54,11 @@ void ImageStyleValue::load_any_resources(DOM::Document& document)
},
nullptr);
// If the image request is already available or fetching, no need to start another fetch.
if (m_image_request->is_available() || m_image_request->fetch_controller())
return;
auto request = HTML::create_potential_CORS_request(document.vm(), m_url, Fetch::Infrastructure::Request::Destination::Image, HTML::CORSSettingAttribute::NoCORS);
request->set_client(&document.relevant_settings_object());
m_image_request->fetch_image(document.realm(), request);
if (m_image_request->needs_fetching()) {
auto request = HTML::create_potential_CORS_request(document.vm(), m_url, Fetch::Infrastructure::Request::Destination::Image, HTML::CORSSettingAttribute::NoCORS);
request->set_client(&document.relevant_settings_object());
m_image_request->fetch_image(document.realm(), request);
}
}
void ImageStyleValue::animate()

View File

@ -43,7 +43,7 @@ public:
private:
ImageStyleValue(AK::URL const&);
RefPtr<HTML::ImageRequest> m_image_request;
RefPtr<HTML::SharedImageRequest> m_image_request;
void animate();
Gfx::Bitmap const* bitmap(size_t frame_index, Gfx::IntSize = {}) const;

View File

@ -403,6 +403,7 @@ class Path2D;
class Plugin;
class PluginArray;
class PromiseRejectionEvent;
class SharedImageRequest;
class Storage;
class SubmitEvent;
class TextMetrics;

View File

@ -456,8 +456,8 @@ after_step_7:
// multiple image elements (as well as CSS background-images, etc.)
// 16. Set image request to a new image request whose current URL is urlString.
// AD-HOC: Note that the ImageRequest "created" here may be an already existing one.
auto image_request = ImageRequest::get_shareable_or_create(*document().page(), url_string).release_value_but_fixme_should_propagate_errors();
auto image_request = ImageRequest::create(*document().page()).release_value_but_fixme_should_propagate_errors();
image_request->set_current_url(url_string);
// 17. If current request's state is unavailable or broken, then set the current request to image request.
// Otherwise, set the pending request to image request.
@ -476,7 +476,9 @@ after_step_7:
image_request->add_callbacks(
[this, image_request, maybe_omit_events, url_string, previous_url]() {
auto image_data = image_request->image_data();
VERIFY(image_request->shared_image_request());
auto image_data = image_request->shared_image_request()->image_data();
image_request->set_image_data(image_data);
ListOfAvailableImages::Key key;
key.url = url_string;
@ -492,6 +494,9 @@ after_step_7:
image_request->prepare_for_presentation(*this);
}
// 2. Set image request to the completely available state.
image_request->set_state(ImageRequest::State::CompletelyAvailable);
// 3. Add the image to the list of available images using the key key, with the ignore higher-layer caching flag set.
document().list_of_available_images().add(key, *image_data, true).release_value_but_fixme_should_propagate_errors();
@ -534,7 +539,7 @@ after_step_7:
});
// AD-HOC: If the image request is already available or fetching, no need to start another fetch.
if (image_request->is_available() || image_request->fetch_controller())
if (image_request->is_available() || image_request->is_fetching())
return;
// 18. Let request be the result of creating a potential-CORS request given urlString, "image",

View File

@ -318,7 +318,7 @@ void HTMLObjectElement::load_image()
// NOTE: This currently reloads the image instead of reusing the resource we've already downloaded.
auto data = attribute(HTML::AttributeNames::data);
auto url = document().parse_url(data);
m_image_request = HTML::ImageRequest::get_shareable_or_create(*document().page(), url).release_value_but_fixme_should_propagate_errors();
m_image_request = HTML::SharedImageRequest::get_or_create(*document().page(), url).release_value_but_fixme_should_propagate_errors();
m_image_request->add_callbacks(
[this] {
run_object_representation_completed_steps(Representation::Image);
@ -327,13 +327,11 @@ void HTMLObjectElement::load_image()
run_object_representation_fallback_steps();
});
// If the image request is already available or fetching, no need to start another fetch.
if (m_image_request->is_available() || m_image_request->fetch_controller())
return;
auto request = HTML::create_potential_CORS_request(vm(), url, Fetch::Infrastructure::Request::Destination::Image, HTML::CORSSettingAttribute::NoCORS);
request->set_client(&document().relevant_settings_object());
m_image_request->fetch_image(realm(), request);
if (m_image_request->needs_fetching()) {
auto request = HTML::create_potential_CORS_request(vm(), url, Fetch::Infrastructure::Request::Destination::Image, HTML::CORSSettingAttribute::NoCORS);
request->set_client(&document().relevant_settings_object());
m_image_request->fetch_image(realm(), request);
}
}
void HTMLObjectElement::update_layout_and_child_objects(Representation representation)

View File

@ -80,7 +80,7 @@ private:
RefPtr<DecodedImageData const> image_data() const;
RefPtr<ImageRequest> m_image_request;
RefPtr<SharedImageRequest> m_image_request;
};
}

View File

@ -19,37 +19,18 @@
namespace Web::HTML {
static HashTable<ImageRequest*>& shareable_image_requests()
{
static HashTable<ImageRequest*> requests;
return requests;
}
ErrorOr<NonnullRefPtr<ImageRequest>> ImageRequest::create(Page& page)
{
return adopt_nonnull_ref_or_enomem(new (nothrow) ImageRequest(page));
}
ErrorOr<NonnullRefPtr<ImageRequest>> ImageRequest::get_shareable_or_create(Page& page, AK::URL const& url)
{
for (auto& it : shareable_image_requests()) {
if (it->current_url() == url)
return *it;
}
auto request = TRY(create(page));
request->set_current_url(url);
return request;
}
ImageRequest::ImageRequest(Page& page)
: m_page(page)
{
shareable_image_requests().set(this);
}
ImageRequest::~ImageRequest()
{
shareable_image_requests().remove(this);
}
// https://html.spec.whatwg.org/multipage/images.html#img-available
@ -59,6 +40,11 @@ bool ImageRequest::is_available() const
return m_state == State::PartiallyAvailable || m_state == State::CompletelyAvailable;
}
bool ImageRequest::is_fetching() const
{
return m_shared_image_request && m_shared_image_request->is_fetching();
}
ImageRequest::State ImageRequest::state() const
{
return m_state;
@ -77,10 +63,12 @@ AK::URL const& ImageRequest::current_url() const
void ImageRequest::set_current_url(AK::URL url)
{
m_current_url = move(url);
if (m_current_url.is_valid())
m_shared_image_request = SharedImageRequest::get_or_create(m_page, m_current_url).release_value_but_fixme_should_propagate_errors();
}
// https://html.spec.whatwg.org/multipage/images.html#abort-the-image-request
void abort_the_image_request(JS::Realm& realm, ImageRequest* image_request)
void abort_the_image_request(JS::Realm&, ImageRequest* image_request)
{
// 1. If image request is null, then return.
if (!image_request)
@ -91,10 +79,7 @@ void abort_the_image_request(JS::Realm& realm, ImageRequest* image_request)
// 3. Abort any instance of the fetching algorithm for image request,
// discarding any pending tasks generated by that algorithm.
if (auto fetch_controller = image_request->fetch_controller())
fetch_controller->abort(realm, {});
image_request->set_fetch_controller(nullptr);
// AD-HOC: We simply don't do this, since our SharedImageRequest may be used by someone else.
}
RefPtr<DecodedImageData const> ImageRequest::image_data() const
@ -128,125 +113,16 @@ void ImageRequest::prepare_for_presentation(HTMLImageElement&)
// FIXME: 16. Update req's img element's presentation appropriately.
}
JS::GCPtr<Fetch::Infrastructure::FetchController> ImageRequest::fetch_controller()
{
return m_fetch_controller.ptr();
}
void ImageRequest::set_fetch_controller(JS::GCPtr<Fetch::Infrastructure::FetchController> fetch_controller)
{
m_fetch_controller = move(fetch_controller);
}
void ImageRequest::fetch_image(JS::Realm& realm, JS::NonnullGCPtr<Fetch::Infrastructure::Request> request)
{
Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {};
fetch_algorithms_input.process_response = [this, &realm, request](JS::NonnullGCPtr<Fetch::Infrastructure::Response> response) {
// FIXME: If the response is CORS cross-origin, we must use its internal response to query any of its data. See:
// https://github.com/whatwg/html/issues/9355
response = response->unsafe_response();
// 26. As soon as possible, jump to the first applicable entry from the following list:
// FIXME: - If the resource type is multipart/x-mixed-replace
// - If the resource type and data corresponds to a supported image format, as described below
// - The next task that is queued by the networking task source while the image is being fetched must run the following steps:
auto process_body = [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) {
handle_failed_fetch();
};
if (response->body().has_value())
response->body().value().fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }).release_value_but_fixme_should_propagate_errors();
};
// 25. Fetch the image: Fetch request.
// Return from this algorithm, and run the remaining steps as part of the fetch's processResponse for the response response.
auto fetch_controller = Fetch::Fetching::fetch(
realm,
request,
Fetch::Infrastructure::FetchAlgorithms::create(realm.vm(), move(fetch_algorithms_input)))
.release_value_but_fixme_should_propagate_errors();
set_fetch_controller(fetch_controller);
VERIFY(m_shared_image_request);
m_shared_image_request->fetch_image(realm, request);
}
void ImageRequest::add_callbacks(JS::SafeFunction<void()> on_finish, JS::SafeFunction<void()> on_fail)
{
if (is_available()) {
if (on_finish)
on_finish();
return;
}
if (state() == ImageRequest::State::Broken) {
if (on_fail)
on_fail();
return;
}
m_callbacks.append({ move(on_finish), move(on_fail) });
}
void ImageRequest::handle_successful_fetch(AK::URL const& url_string, StringView mime_type, ByteBuffer data)
{
// AD-HOC: At this point, things gets very ad-hoc.
// FIXME: Bring this closer to spec.
bool const is_svg_image = mime_type == "image/svg+xml"sv || url_string.basename().ends_with(".svg"sv);
RefPtr<DecodedImageData> image_data;
auto handle_failed_decode = [&] {
for (auto& callback : m_callbacks) {
if (callback.on_fail)
callback.on_fail();
}
};
if (is_svg_image) {
auto result = SVG::SVGDecodedImageData::create(m_page, url_string, data);
if (result.is_error())
return handle_failed_decode();
image_data = result.release_value();
} else {
auto result = Web::Platform::ImageCodecPlugin::the().decode_image(data.bytes());
if (!result.has_value())
return handle_failed_decode();
Vector<AnimatedBitmapDecodedImageData::Frame> frames;
for (auto& frame : result.value().frames) {
frames.append(AnimatedBitmapDecodedImageData::Frame {
.bitmap = frame.bitmap,
.duration = static_cast<int>(frame.duration),
});
}
image_data = AnimatedBitmapDecodedImageData::create(move(frames), result.value().loop_count, result.value().is_animated).release_value_but_fixme_should_propagate_errors();
}
set_image_data(image_data);
// 2. Set image request to the completely available state.
set_state(ImageRequest::State::CompletelyAvailable);
for (auto& callback : m_callbacks) {
if (callback.on_finish)
callback.on_finish();
}
}
void ImageRequest::handle_failed_fetch()
{
for (auto& callback : m_callbacks) {
if (callback.on_fail)
callback.on_fail();
}
VERIFY(m_shared_image_request);
m_shared_image_request->add_callbacks(move(on_finish), move(on_fail));
}
}

View File

@ -12,6 +12,7 @@
#include <LibGfx/Size.h>
#include <LibJS/Heap/Handle.h>
#include <LibWeb/Forward.h>
#include <LibWeb/HTML/SharedImageRequest.h>
namespace Web::HTML {
@ -19,7 +20,6 @@ namespace Web::HTML {
class ImageRequest : public RefCounted<ImageRequest> {
public:
static ErrorOr<NonnullRefPtr<ImageRequest>> create(Page&);
static ErrorOr<NonnullRefPtr<ImageRequest>> get_shareable_or_create(Page&, AK::URL const&);
~ImageRequest();
// https://html.spec.whatwg.org/multipage/images.html#img-req-state
@ -31,6 +31,7 @@ public:
};
bool is_available() const;
bool is_fetching() const;
State state() const;
void set_state(State);
@ -50,27 +51,16 @@ public:
// https://html.spec.whatwg.org/multipage/images.html#prepare-an-image-for-presentation
void prepare_for_presentation(HTMLImageElement&);
[[nodiscard]] JS::GCPtr<Fetch::Infrastructure::FetchController> fetch_controller();
void set_fetch_controller(JS::GCPtr<Fetch::Infrastructure::FetchController>);
void fetch_image(JS::Realm&, JS::NonnullGCPtr<Fetch::Infrastructure::Request>);
void add_callbacks(JS::SafeFunction<void()> on_finish, JS::SafeFunction<void()> on_fail);
SharedImageRequest const* shared_image_request() const { return m_shared_image_request; }
private:
explicit ImageRequest(Page&);
void handle_successful_fetch(AK::URL const&, StringView mime_type, ByteBuffer data);
void handle_failed_fetch();
Page& m_page;
struct Callbacks {
JS::SafeFunction<void()> on_finish;
JS::SafeFunction<void()> on_fail;
};
Vector<Callbacks> m_callbacks;
// https://html.spec.whatwg.org/multipage/images.html#img-req-state
// An image request's state is initially unavailable.
State m_state { State::Unavailable };
@ -91,7 +81,7 @@ private:
// which is either a struct consisting of a width and a height or is null. It must initially be null.
Optional<Gfx::FloatSize> m_preferred_density_corrected_dimensions;
JS::Handle<Fetch::Infrastructure::FetchController> m_fetch_controller;
RefPtr<SharedImageRequest> m_shared_image_request;
};
// https://html.spec.whatwg.org/multipage/images.html#abort-the-image-request

View File

@ -0,0 +1,178 @@
/*
* Copyright (c) 2023, Andreas Kling <kling@serenityos.org>
*
* SPDX-License-Identifier: BSD-2-Clause
*/
#include <AK/HashTable.h>
#include <LibGfx/Bitmap.h>
#include <LibWeb/Fetch/Fetching/Fetching.h>
#include <LibWeb/Fetch/Infrastructure/FetchAlgorithms.h>
#include <LibWeb/Fetch/Infrastructure/FetchController.h>
#include <LibWeb/Fetch/Infrastructure/HTTP/Responses.h>
#include <LibWeb/HTML/AnimatedBitmapDecodedImageData.h>
#include <LibWeb/HTML/DecodedImageData.h>
#include <LibWeb/HTML/SharedImageRequest.h>
#include <LibWeb/Platform/ImageCodecPlugin.h>
#include <LibWeb/SVG/SVGDecodedImageData.h>
namespace Web::HTML {
static HashMap<AK::URL, SharedImageRequest*>& shared_image_requests()
{
static HashMap<AK::URL, SharedImageRequest*> requests;
return requests;
}
ErrorOr<NonnullRefPtr<SharedImageRequest>> SharedImageRequest::get_or_create(Page& page, AK::URL const& url)
{
if (auto it = shared_image_requests().find(url); it != shared_image_requests().end())
return *it->value;
auto request = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) SharedImageRequest(page, url)));
shared_image_requests().set(url, request);
return request;
}
SharedImageRequest::SharedImageRequest(Page& page, AK::URL url)
: m_page(page)
, m_url(move(url))
{
}
SharedImageRequest::~SharedImageRequest()
{
shared_image_requests().remove(m_url);
}
RefPtr<DecodedImageData const> SharedImageRequest::image_data() const
{
return m_image_data;
}
JS::GCPtr<Fetch::Infrastructure::FetchController> SharedImageRequest::fetch_controller()
{
return m_fetch_controller.ptr();
}
void SharedImageRequest::set_fetch_controller(JS::GCPtr<Fetch::Infrastructure::FetchController> fetch_controller)
{
m_fetch_controller = move(fetch_controller);
}
void SharedImageRequest::fetch_image(JS::Realm& realm, JS::NonnullGCPtr<Fetch::Infrastructure::Request> request)
{
Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {};
fetch_algorithms_input.process_response = [this, &realm, request](JS::NonnullGCPtr<Fetch::Infrastructure::Response> response) {
// FIXME: If the response is CORS cross-origin, we must use its internal response to query any of its data. See:
// https://github.com/whatwg/html/issues/9355
response = response->unsafe_response();
auto process_body = [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) {
handle_failed_fetch();
};
if (response->body().has_value())
response->body().value().fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }).release_value_but_fixme_should_propagate_errors();
};
m_state = State::Fetching;
auto fetch_controller = Fetch::Fetching::fetch(
realm,
request,
Fetch::Infrastructure::FetchAlgorithms::create(realm.vm(), move(fetch_algorithms_input)))
.release_value_but_fixme_should_propagate_errors();
set_fetch_controller(fetch_controller);
}
void SharedImageRequest::add_callbacks(JS::SafeFunction<void()> on_finish, JS::SafeFunction<void()> on_fail)
{
if (m_state == State::Finished) {
if (on_finish)
on_finish();
return;
}
if (m_state == State::Failed) {
if (on_fail)
on_fail();
return;
}
m_callbacks.append({ move(on_finish), move(on_fail) });
}
void SharedImageRequest::handle_successful_fetch(AK::URL const& url_string, StringView mime_type, ByteBuffer data)
{
// AD-HOC: At this point, things gets very ad-hoc.
// FIXME: Bring this closer to spec.
bool const is_svg_image = mime_type == "image/svg+xml"sv || url_string.basename().ends_with(".svg"sv);
RefPtr<DecodedImageData> image_data;
auto handle_failed_decode = [&] {
m_state = State::Failed;
for (auto& callback : m_callbacks) {
if (callback.on_fail)
callback.on_fail();
}
};
if (is_svg_image) {
auto result = SVG::SVGDecodedImageData::create(m_page, url_string, data);
if (result.is_error())
return handle_failed_decode();
image_data = result.release_value();
} else {
auto result = Web::Platform::ImageCodecPlugin::the().decode_image(data.bytes());
if (!result.has_value())
return handle_failed_decode();
Vector<AnimatedBitmapDecodedImageData::Frame> frames;
for (auto& frame : result.value().frames) {
frames.append(AnimatedBitmapDecodedImageData::Frame {
.bitmap = frame.bitmap,
.duration = static_cast<int>(frame.duration),
});
}
image_data = AnimatedBitmapDecodedImageData::create(move(frames), result.value().loop_count, result.value().is_animated).release_value_but_fixme_should_propagate_errors();
}
m_image_data = move(image_data);
m_state = State::Finished;
for (auto& callback : m_callbacks) {
if (callback.on_finish)
callback.on_finish();
}
}
void SharedImageRequest::handle_failed_fetch()
{
m_state = State::Failed;
for (auto& callback : m_callbacks) {
if (callback.on_fail)
callback.on_fail();
}
}
bool SharedImageRequest::needs_fetching() const
{
return m_state == State::New;
}
bool SharedImageRequest::is_fetching() const
{
return m_state == State::Fetching;
}
}

View File

@ -0,0 +1,66 @@
/*
* Copyright (c) 2023, Andreas Kling <kling@serenityos.org>
*
* SPDX-License-Identifier: BSD-2-Clause
*/
#pragma once
#include <AK/Error.h>
#include <AK/OwnPtr.h>
#include <AK/URL.h>
#include <LibGfx/Size.h>
#include <LibJS/Heap/Handle.h>
#include <LibJS/SafeFunction.h>
#include <LibWeb/Forward.h>
namespace Web::HTML {
class SharedImageRequest : public RefCounted<SharedImageRequest> {
public:
static ErrorOr<NonnullRefPtr<SharedImageRequest>> get_or_create(Page&, AK::URL const&);
~SharedImageRequest();
AK::URL const& url() const { return m_url; }
[[nodiscard]] RefPtr<DecodedImageData const> image_data() const;
[[nodiscard]] JS::GCPtr<Fetch::Infrastructure::FetchController> fetch_controller();
void set_fetch_controller(JS::GCPtr<Fetch::Infrastructure::FetchController>);
void fetch_image(JS::Realm&, JS::NonnullGCPtr<Fetch::Infrastructure::Request>);
void add_callbacks(JS::SafeFunction<void()> on_finish, JS::SafeFunction<void()> on_fail);
bool is_fetching() const;
bool needs_fetching() const;
private:
explicit SharedImageRequest(Page&, AK::URL);
void handle_successful_fetch(AK::URL const&, StringView mime_type, ByteBuffer data);
void handle_failed_fetch();
enum class State {
New,
Fetching,
Finished,
Failed,
};
State m_state { State::New };
Page& m_page;
struct Callbacks {
JS::SafeFunction<void()> on_finish;
JS::SafeFunction<void()> on_fail;
};
Vector<Callbacks> m_callbacks;
AK::URL m_url;
RefPtr<DecodedImageData const> m_image_data;
JS::Handle<Fetch::Infrastructure::FetchController> m_fetch_controller;
};
}