LibWeb/Fetch: Improve aborting fetch #1871
Conversation
This allows DOMExceptions to be serialized.
skyz1
force-pushed
the
fetch-abort-error
branch
from
7ceb454
to
ad071da
Compare
|
Hello! One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the |
skyz1
force-pushed
the
fetch-abort-error
branch
2 times, most recently
from
ea8530f
to
e181578
Compare
The reason provided to AbortControllers now gets returned by fetch.
When aborting fetch, the request stream now gets closed and the response stream errors.
skyz1
force-pushed
the
fetch-abort-error
branch
from
e181578
to
9737b61
Compare
|
I'm not sure if I did the forward declarations correctly, I'm new to C++. Feel free to suggest improvements :^) |
| // If that threw an exception or returned undefined, then set deserializedError to fallbackError. | ||
| if (m_serialized_abort_reason.has_value()) { | ||
| auto deserialized_error_or_exception = HTML::structured_deserialize(realm.vm(), m_serialized_abort_reason.value(), realm, {}); | ||
| if (!deserialized_error_or_exception.is_exception() && !deserialized_error_or_exception.value().is_undefined()) { |
There was a problem hiding this comment.
| if (!deserialized_error_or_exception.is_exception() && !deserialized_error_or_exception.value().is_undefined()) { | |
| if (!deserialized_error_or_exception.is_exception() || !deserialized_error_or_exception.value().is_undefined()) { |
Spec says or returned undefined.
There was a problem hiding this comment.
No, the code as written is correct. the spec says that when it is an exception or undefined, we should set deserializedError to fallbackError. This condition and statement is saying that when the result is not an exception and not undefined, set deserialized error to StructuredDeserialize(abortReason, realm)
There was a problem hiding this comment.
Whoops. Didn't consider earlier context
There was a problem hiding this comment.
This seems pretty reasonable. It would be nice if we could add a test for this. fetch() tests are tricky because we don't quite have a real http/https server in our test infra to use. But we can probably fetch some resource we know won't exist and check that the abort reason is set to the expected network error?
| @@ -38,3 +39,35 @@ void DOMException::initialize(JS::Realm& realm) | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| // https://webidl.spec.whatwg.org/#ref-for-serialization-steps%E2%91%A2 | |||
| Web::WebIDL::ExceptionOr<void> Web::WebIDL::DOMException::serialization_steps(HTML::SerializationRecord& record, bool, HTML::SerializationMemory&) | |||
There was a problem hiding this comment.
This definition should be within the opened Web::WebIDL namespace, rather than outside of it with the fully qualified name in the function name. and return type.
| auto deserialized_error = JS::js_undefined(); | ||
| // 1. Let deserializedError be the result of deserialize a serialized abort reason given controller’s | ||
| // serialized abort reason and relevantRealm. | ||
| auto deserialized_error = controller->deserialize_a_serialized_abort_reason(relevant_realm); |
There was a problem hiding this comment.
This is actually kind of tricky. We cannot take controller by reference here. The variable controller lives on the stack of this method. It is assigned a value on line 119 when calling fetch, so in theory ™️ your change here is correct, as the process_response fetch algorithm will be called after that assignment occurs. However, calling fetch() is not synchronous, and will happen over several subsequent HTML event loop invocations.
So this is actually a stack-use-after-scope error, and we need to play some games to let this algorithm use the fetch controller that is created for the fetch operation.
There's basically two options:
- refactor fetch to allow passing in a fetch controller we made ourselves before calling it. which may or may not be a good idea
- create a silly GC struct to hold the controller pointer that we can hold onto in this lambda (by value) and assign an actual value to when calling fetch.
| // 4. Let serializedError be StructuredSerialize(error). If that threw an exception, catch it, and let serializedError be StructuredSerialize(fallbackError). | ||
| auto serialized_error_result = HTML::structured_serialize(realm.vm(), error.value()); | ||
| if (serialized_error_result.is_exception()) { | ||
| m_serialized_abort_reason = HTML::structured_serialize(realm.vm(), fallback_error).value(); |
There was a problem hiding this comment.
this should use the MUST() macro here, instead of grabbing the .value from the WebIDL::ExceptionOr.
| // If that threw an exception or returned undefined, then set deserializedError to fallbackError. | ||
| if (m_serialized_abort_reason.has_value()) { | ||
| auto deserialized_error_or_exception = HTML::structured_deserialize(realm.vm(), m_serialized_abort_reason.value(), realm, {}); | ||
| if (!deserialized_error_or_exception.is_exception() && !deserialized_error_or_exception.value().is_undefined()) { |
There was a problem hiding this comment.
No, the code as written is correct. the spec says that when it is an exception or undefined, we should set deserializedError to fallbackError. This condition and statement is saying that when the result is not an exception and not undefined, set deserialized error to StructuredDeserialize(abortReason, realm)
This PR improves fetch so that the reason for an abort is returned and the request/response streams are properly handled.