LibWeb: Implement Web::Fetch::Body::fully_read() closer to spec

[gmta]

By actually using streams, they get marked as disturbed and the .bodyUsed API starts to work. Fixes at least 94 subtests in the WPT fetch/api/request test suite.

Pretty far out of my comfort zone for this so a thorough review is appreciated!

WPT results for fetch/api/request, before:

Ran 1065 checks (971 subtests, 94 tests)
Expected results: 813
Unexpected results: 252
  test: 35 (35 error)
  subtest: 217 (217 fail)

And after:

Ran 1065 checks (971 subtests, 94 tests)
Expected results: 907
Unexpected results: 158
  test: 35 (35 error)
  subtest: 123 (123 fail)

[trflynn89]

I had a branch some time ago where I implemented fully reading with streams:
https://github.com/trflynn89/serenity/commits/fetch_stream/

I didn't PR that because I ran into:
whatwg/fetch#1754

Is that still an issue? I don't remember all the details, but it was really 2 issues: one related to locking the stream here and not unlocking it (there's a workaround in the branch above for that), and another related to fetch requests that have integrity metadata (never came up with a workaround for that one).

[gmta]

@trflynn89

I had a branch some time ago

Oof, missed that you worked on this as well. You seem to have the more in-depth approach as well w/ regard to the callbacks.

Is that still an issue?

No idea, did you have a repro?

[trflynn89]

Oh no worries, I had completely forgotten about it and that spec issue hasn't gotten any traction recently, so we could definitely use some help coming up with a workaround for the second issue 😅

I found that discord.com uses fetch integrity metadata, and this is indeed still an issue:

VERIFICATION FAILED: !is_readable_stream_locked(source) at /Users/flynn/workspace/ladybird/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp:262
0   liblagom-ak.0.0.0.dylib             0x0000000100b52cf4 ak_verification_failed + 216
1   liblagom-web.0.0.0.dylib            0x0000000102bd0e90 Web::Streams::readable_stream_pipe_to(Web::Streams::ReadableStream&, Web::Streams::WritableStream&, bool, bool, bool, AK::Optional<JS::Value>) + 924
2   liblagom-web.0.0.0.dylib            0x0000000102959dc4 Web::Fetch::Fetching::fetch_response_handover(JS::Realm&, Web::Fetch::Infrastructure::FetchParams const&, Web::Fetch::Infrastructure::Response&) + 1236

[shannonbooth]

There is a potentially related crash for this WPT test: https://staging.wpt.fyi/results/streams/piping/multiple-propagation.any.html

[shannonbooth]

But I may be getting it completely wrong, I only started an initial investigation of that one today, and I thought it might be relating to these unimplemented steps (and FIXME just after): https://github.com/LadybirdBrowser/ladybird/blob/master/Userland/Libraries/LibWeb/Streams/AbstractOperations.cpp#L286

Which isn't the most straight forward to implement because it even involves likely GC allocating a bool to be tracked across various callbacks, and lots of callbacks in general

[gmta]

@trflynn89 FWIW, I've reapplied your commits in my branch here. I'll take a shot at the issues.

[gmta]

@shannonbooth

There is a potentially related crash

It seems to be related, but not caused by any of the changes in this PR. Let's track it as an issue if we did not open one already :)

[shannonbooth]

@shannonbooth

There is a potentially related crash

It seems to be related, but not caused by any of the changes in this PR. Let's track it as an issue if we did not open one already :)

Oh definitely, I was just thinking it could be helpful for debooging if it was same root cause for a reduced test case, thats all :^)

[trflynn89]

Turns out I did actually open a PR: SerenityOS/serenity#24396
There's a bit more detail on the issues we hit in comments there