LibWeb: Integrate TransformStream into fetch_response_handover() and Streams into XHR::send()

[kennethmyhra]

This implements:

• the concept incrementally read a body, and uses that implementation in XHR::send().
• the missing steps for TransformStream in fetch_response_handover().

Fixes a couple bugs:

• LibWeb: Provide an UInt8Array to AO writable_stream_default_writer_write
• LibWeb: Close acquired writer in AO readable_stream_pipe_to()

[kalenikaliaksandr]

could you add a test please?

[kennethmyhra]

Still struggling a bit with this one. Everything works as it did before this change. I have verified that this branch in fetch_response_handover() gets hit by tests as well as when browsing the web normally. But... identity_transform_algorithm and flush_algorithm does not seem to be called.

[kennethmyhra]

Drafting this until I can figure out why the callbacks are currently not invoked

[trflynn89]

I'm hitting a similar issue trying to get Fetch::Body::fully_read to use streams. On your branch, it looks like your transform stream ultimately hits ReadableStreamDefaultReader::read_all_bytes when you call Streams::readable_stream_pipe_to. Which is the same thing Fetch::Body::fully_read should do, and my callbacks are also never getting invoked (which actually breaks every test using fetch).

My WIP branch is here: https://github.com/trflynn89/serenity/tree/fetch_stream
(note in this branch, I've changed Streams::readable_stream_pipe_to to use a more purposeful ReadableStreamDefaultReader::read_all_chunks method, but effectively the same thing).

As far as I can tell, the calls to read_all_bytes never end up with anything calling Streams::readable_stream_enqueue, which is what would actually call the read requests's on_chunk method.

[kennethmyhra]

On your branch, it looks like your transform stream ultimately hits ReadableStreamDefaultReader::read_all_bytes when you call Streams::readable_stream_pipe_to.

Correct!

I have tried in the past to convert Fetch::Body::fully_read to be spec compliant, didn't realize back then that the cause for it not working was the callbacks not being invoked.

Going to re-apply this on top of your changes without the Fetch::Body::fully_read change to see if things works better.

[kennethmyhra]

Yeah, no. This still doesn't work as expected even with your changes. I don't get why the Streams::TransformStreamDefaultController::transform_algorithm() isn't invoked in this context.

Btw, your changes even without Fetch::Body::fully_read() would be nice to get merged, getting rid of the horrible Vectory<ByteBuffer> hack I added would be a nice bonus as well.

[Lubrsi]

Is it potentially because this step is unimplemented, where it's supposed to enqueue body chunks into the stream?

serenity/Userland/Libraries/LibWeb/Fetch/BodyInit.cpp

Line 139 in 452ffa5

// FIXME: 12. If action is non-null, then run these steps in parallel:

[kennethmyhra]

Is it potentially because this step is unimplemented, where it's supposed to enqueue body chunks into the stream?

serenity/Userland/Libraries/LibWeb/Fetch/BodyInit.cpp

Line 139 in 452ffa5

// FIXME: 12. If action is non-null, then run these steps in parallel:

That does indeed seem plausible

[kennethmyhra]

Is it potentially because this step is unimplemented, where it's supposed to enqueue body chunks into the stream?

serenity/Userland/Libraries/LibWeb/Fetch/BodyInit.cpp

Line 139 in 452ffa5

// FIXME: 12. If action is non-null, then run these steps in parallel:

That does indeed seem plausible

Tried to hack something together in BodyInit.cpp + applying Tim's changes from his fetch_stream branch. The identity_transform callback is at least called, but ending up with a process_body_error at some point. Seeing this output in my console: FIXME: Load html page with an error if read of body failed..

Don't have the time to debug it right now, pushed a messy branch here if anyone is curious: https://github.com/kennethmyhra/serenity/tree/integrate-transformstream-in-fetch_response_handover-3

[trflynn89]

FIXME: Load html page with an error if read of body failed..

That error turns out to be a bug in readable_stream_pipe_to. In chunk_steps, we are providing a JS::ArrayBuffer to writable_stream_default_writer_write, but turns out we need to provide a JS::Uint8Array (this is checked in ReadLoopReadRequest::on_chunk). So I've locally changed chunk_steps in readable_stream_pipe_to to be:

auto chunk_steps = [&realm, writer](ByteBuffer buffer) {
    auto array_buffer = JS::ArrayBuffer::create(realm, move(buffer));
    auto chunk = JS::Uint8Array::create(realm, array_buffer->byte_length(), *array_buffer);

    auto promise = writable_stream_default_writer_write(writer, chunk);
    WebIDL::resolve_promise(realm, promise, JS::js_undefined());
};

and that error goes away. But things still generally don't work after that 😅

FYI I found the real exception here by calling HTML::report_exception_to_console(exception, realm, HTML::ErrorInPromise::No); in Body::fully_read's error_steps, which logged "Chunk data is not Uint8Array".

[kennethmyhra]

That error turns out to be a bug in readable_stream_pipe_to. In chunk_steps, we are providing a JS::ArrayBuffer to writable_stream_default_writer_write, but turns out we need to provide a JS::Uint8Array (this is checked in ReadLoopReadRequest::on_chunk).

Right, I was going through some hoops in the tests back when I implemented pipeTo and pipeThrough, you can see I'm encoding the chunks using TextEncoder before enqueueing them: https://github.com/SerenityOS/serenity/blob/master/Tests/LibWeb/Text/input/Streams/ReadableStream-pipeTo.html

Still, with your changes I am not able to remove the hacks in those tests.

EDIT:
Resolved conflicts

[kennethmyhra]

Tests here will fail for XHR and a few other tests. Still not integrated Fetch::Body::fully_read() since everything then falls apart. Pulled the two other commits from Tim's https://github.com/trflynn89/serenity/tree/fetch_stream branch.

Implemented the concept IncrementalReadLoopReadRequest and wired that up in XHR::send() since that was the initial requirement for integrating TransformStream in fetch_response_handover().

Tried to implement as close to spec as I could interpret the missing steps from https://fetch.spec.whatwg.org/#concept-bodyinit-extract, which was suggested by Luke above. Still haven't found a proper way to close the stream as specified, since TransformStream in Fetch is then not executed properly. Not closing the stream is necessary to run the IncrementalReadLoopReadRequest::on_close step, and is probably the root cause why the XHR tests are failing right now.

[kennethmyhra]

This is now ready for review, Updated PR description to describe what is implemented and which bugs have been fixed.

[kennethmyhra]

Fixed up the commits according to the suggestions. As for the requests for tests, I have relied on the existing XHR-tests for this. I don't have a good idea of how to isolate that functionality. What I can say is that adressing the FIXMEs in XHR::send() that relied on Stream support required me to implement the FIXMEs for TransformStream in fetch.

[trflynn89]

Looks good! Thanks for seeing this through. Just one last dangling comment to fixup

[kennethmyhra]

Thanks! Glad to see this through myself, the XHR changes have been stashed around since last summer so pretty glad to finally land this.