Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LibWeb/HTML: Resolve image decoding promises inside tasks #2229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+80 −35
Open

LibWeb/HTML: Resolve image decoding promises inside tasks #2229

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 80 additions & 35 deletions Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<WebIDL::Promise>> HTMLImageElement::decode(

// 2. Queue a microtask to perform the following steps:
queue_a_microtask(&document(), JS::create_heap_function(realm.heap(), [this, promise, &realm]() mutable {
// 1. Let global be this's relevant global object.
auto& global = relevant_global_object(*this);

auto reject_if_document_not_fully_active = [this, promise, &realm]() -> bool {
if (this->document().is_fully_active())
return false;
Expand All @@ -311,55 +314,97 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<WebIDL::Promise>> HTMLImageElement::decode(
return true;
};

// 2.1 If any of the following are true:
// 2.1.1 this's node document is not fully active;
// 2.1.1 then reject promise with an "EncodingError" DOMException.
if (reject_if_document_not_fully_active())
return;

// 2.1.2 or this's current request's state is broken,
// 2.1.2 then reject promise with an "EncodingError" DOMException.
if (reject_if_current_request_state_broken())
// 2. If any of the following are true:
// - this's node document is not fully active;
// - or this's current request's state is broken,
// then reject promise with an "EncodingError" DOMException.
if (reject_if_document_not_fully_active() || reject_if_current_request_state_broken()) {
return;
}

// 2.2 Otherwise, in parallel wait for one of the following cases to occur, and perform the corresponding actions:
Platform::EventLoopPlugin::the().deferred_invoke(JS::create_heap_function(heap(), [this, promise, &realm, reject_if_document_not_fully_active, reject_if_current_request_state_broken] {
// 3. Otherwise, in parallel wait for one of the following cases to occur, and perform the corresponding actions:
Platform::EventLoopPlugin::the().deferred_invoke(JS::create_heap_function(heap(), [this, promise, &realm, &global] {
enum class ImageState {
DocumentNoLongerFullyActive,
CurrentRequestChanged,
CurrentRequestBroken,
CurrentRequestCompletelyAvailable,
};
ImageState image_state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/ not initialized at declaration.

auto previous_state = current_request().state();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This double event loop spin has always been truly puzzling. ... why do we need it?

Platform::EventLoopPlugin::the().spin_until(JS::create_heap_function(heap(), [&] {
auto state = this->current_request().state();

return !this->document().is_fully_active() || state == ImageRequest::State::Broken || state == ImageRequest::State::CompletelyAvailable;
}));
if (state != previous_state) {
image_state = ImageState::CurrentRequestChanged;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the state of the current request changing really mean that the current request changed? What about ABA? Should we be storing the "Current request" and making sure the image element still has the same request instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"or is mutated" 🤔 How do we detect that?

return true;
}
previous_state = state;

// 2.2.1 This img element's node document stops being fully active
// 2.2.1 Reject promise with an "EncodingError" DOMException.
if (reject_if_document_not_fully_active())
return;
if (!document().is_fully_active()) {
image_state = ImageState::DocumentNoLongerFullyActive;
return true;
}

// FIXME: 2.2.2 This img element's current request changes or is mutated
// FIXME: 2.2.2 Reject promise with an "EncodingError" DOMException.
if (state == ImageRequest::State::Broken) {
image_state = ImageState::CurrentRequestBroken;
return true;
}

// 2.2.3 This img element's current request's state becomes broken
// 2.2.3 Reject promise with an "EncodingError" DOMException.
if (reject_if_current_request_state_broken())
if (state == ImageRequest::State::CompletelyAvailable) {
image_state = ImageState::CurrentRequestCompletelyAvailable;
return true;
}

return false;
}));

switch (image_state) {
// -> This img element's node document stops being fully active
// -> This img element's current request changes or is mutated
// -> This img element's current request's state becomes broken
case ImageState::DocumentNoLongerFullyActive:
case ImageState::CurrentRequestChanged:
case ImageState::CurrentRequestBroken: {
// Queue a global task on the DOM manipulation task source with global to reject promise with an "EncodingError" DOMException.
queue_global_task(Task::Source::DOMManipulation, global, JS::create_heap_function(realm.heap(), [&realm, &promise, image_state] {
auto message = [image_state]() -> String {
switch (image_state) {
case ImageState::DocumentNoLongerFullyActive:
return "Node document not fully active"_string;
case ImageState::CurrentRequestChanged:
return "Current request state changed"_string;
case ImageState::CurrentRequestBroken:
return "Current request state is broken"_string;
default:
VERIFY_NOT_REACHED();
}
}();
auto exception = WebIDL::EncodingError::create(realm, message);
HTML::TemporaryExecutionContext context(realm);
WebIDL::reject_promise(realm, promise, exception);
}));
return;
}

// -> This img element's current request's state becomes completely available
case ImageState::CurrentRequestCompletelyAvailable: {
// FIXME: Decode the image.
// FIXME: If decoding does not need to be performed for this image (for example because it is a vector graphic) or the decoding process completes successfully, then queue a global task on the DOM manipulation task source with global to resolve promise with undefined.
// FIXME: If decoding fails (for example due to invalid image data), then queue a global task on the DOM manipulation task source with global to reject promise with an "EncodingError" DOMException.

// 2.2.4 This img element's current request's state becomes completely available
if (this->current_request().state() == ImageRequest::State::CompletelyAvailable) {
// 2.2.4.1 FIXME: Decode the image.
// 2.2.4.2 FIXME: If decoding does not need to be performed for this image (for example because it is a vector graphic), resolve promise with undefined.
// 2.2.4.3 FIXME: If decoding fails (for example due to invalid image data), reject promise with an "EncodingError" DOMException.
// 2.2.4.4 FIXME: If the decoding process completes successfully, resolve promise with undefined.
// 2.2.4.5 FIXME: User agents should ensure that the decoded media data stays readily available until at least the end of the next successful update
// the rendering step in the event loop. This is an important part of the API contract, and should not be broken if at all possible.
// (Typically, this would only be violated in low-memory situations that require evicting decoded image data, or when the image is too large
// to keep in decoded form for this period of time.)

HTML::TemporaryExecutionContext context(realm);
WebIDL::resolve_promise(realm, promise, JS::js_undefined());
// NOTE: For now we just resolve it.
queue_global_task(Task::Source::DOMManipulation, global, JS::create_heap_function(realm.heap(), [&realm, &promise] {
HTML::TemporaryExecutionContext context(realm);
WebIDL::resolve_promise(realm, promise, JS::js_undefined());
}));
break;
}
}
}));
}));

// 3. Return promise.
return promise;
}

Expand Down
Loading