Add implementation of `MultipartFormData` streaming encoder #200

andreiltd · 2025-01-21T14:21:31Z

I did a quick comparison of encoded FormData between SM and Chrome using following JS code:

Simple FormData

async function readStream(stream) {
 const reader = stream.getReader();
 const chunks = [];

 while (true) {
   const { done, value } = await reader.read();
   if (done) {
     break;
   }
   chunks.push(value);
 }

 let totalLen = 0;
 for (const chunk of chunks) {
   totalLen += chunk.length;
 }

 const result = new Uint8Array(totalLen);

 let offset = 0;
 for (const chunk of chunks) {
   result.set(chunk, offset);
   offset += chunk.length;
 }

 return result;
}

async function main() {
 const form = new FormData();
 form.append('field1', 'value1');
 form.append('field2', 'value2');

 const file = new File(['Hello World!'], 'dummy.txt', { type: 'foo' });
 form.append('file1', file);

 const req = new Request('https:example.com', {
   method: 'POST',
   body: form,
 });

 const type = req.headers.get('Content-Type');
 const boundary = type.split('boundary=')[1];

 // assert(type.startsWith('multipart/form-data; boundary='), "Content-Type is multipart/form-data");
 const data = await readStream(req.body);
 const dec = new TextDecoder();
 const bodyStr = dec.decode(data);

 console.log(bodyStr);
}

main();

The results are:

Chrome:

------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="field1"

value1
------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="field2"

value2
------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="file1"; filename="dummy.txt"
Content-Type: foo

Hello World!
------WebKitFormBoundaryhpShnP1JqrBTVTnC--

SM:

----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="field1"

value1
----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="field2"

value2
----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="file1"; filename="dummy.txt"
Content-Type: foo

Hello World!
----BoundaryjXo5N4HEAXWcKrw7--

I also did some manual fuzzing to test the streaming logic by varying the buffer sizes the encoder writes into. Specifically, I tested these buffer sizes in bytes: 1, 2, 4, 8, 32, 1024, and 8192 by changing the size in the implementation. Though, having some unit test framework would be nice.

Relevant RFC: https://www.rfc-editor.org/rfc/rfc2046#section-5.1.1

andreiltd · 2025-01-22T14:04:46Z

~~It looks like the CI failure is unrelated to this PR (I've observed the same failure on main branch). This is a problematic test:~~

    const response = await fetch("https://http-me.glitch.me/meow?header=cat:é");
    strictEqual(response.headers.get('cat'), "é");

tschneidereit · 2025-01-24T14:11:59Z

@andreiltd, is this ready to review, or are you still expecting to make changes to it?

andreiltd · 2025-01-24T16:38:53Z

Yes, this is ready to review :)

tschneidereit

Either I'm missing something in looking at the diff, or there is some code missing—see the inline comment on form-data-encoder.h.

One thing that I can't fully evaluate, because it'll presumably be part of the implementation of encode_stream: IIUC, field names, will always be first have their newlines normalized to CRLF, and then have those escaped. It'd be good to fold those into one operation, if possible.

Otherwise, I left a few comments and suggestions. I'm a bit concerned about allocation and general failure handling, so it'd be good to go over those aspects in some detail.

builtins/web/form-data/form-data-encoder.cpp

tschneidereit · 2025-01-25T16:24:03Z

builtins/web/form-data/form-data-encoder.h

+  static size_t query_length(JSContext *cx, HandleObject self);
+  static JSObject *encode_stream(JSContext *cx, HandleObject self);
+  static JSObject *create(JSContext *cx, HandleObject form_data);


The implementation of these seems to be missing? (And in the case of query_length I also can't find any uses.)

Hmm... that's weird:

create function is here: https://github.com/bytecodealliance/StarlingMonkey/pull/200/files#diff-1794d7449a6c94d4fb4e898ac1d44ebd87c84c0b52e2ddc2c9e1c522cfaae801R446

encode_stream function is here: https://github.com/bytecodealliance/StarlingMonkey/pull/200/files#diff-1794d7449a6c94d4fb4e898ac1d44ebd87c84c0b52e2ddc2c9e1c522cfaae801R434

I totally forgot about query_length, I'm sorry about that! The reason for this is that the fetch spec doesn't say what to do with Content-Length in case of FormData.

Set length to unclear, see html/6424 for improving this.

See here: https://fetch.spec.whatwg.org/#bodyinit-unions. It technically should be possible to calculate an encoded size beforehand without doing an actual encoding, but it's unclear to me if this is needed or not.

The query_length function is now implemented. Without setting the Content-Length header uploading FormData tests were failing.

tschneidereit · 2025-01-25T16:28:17Z

builtins/web/form-data/form-data-encoder.cpp

+bool MultipartFormDataImpl::handle_entry_header(JSContext *cx, StreamContext &stream) {
+  auto entry = stream.entries->begin()[chunk_idx_];
+  auto header = fmt::memory_buffer();
+  auto name = escape_newlines(entry.name).value();


I think this needs a null check?

The function is overloaded but the version that doesn't take JSContetext is infallible. I can change this to separate function like escape_name and escape_name_val if we prefer explicit return value. Anyway, I've added the assertion for now.

builtins/web/form-data/form-data-encoder.cpp

tschneidereit · 2025-01-25T16:32:59Z

builtins/web/form-data/form-data-encoder.cpp

+  // Hex encode bytes to string
+  auto bytes = std::move(res.unwrap());
+  auto bytes_str = std::string_view((char *)(bytes.ptr.get()), bytes.size());
+  auto base64_str = base64::forgivingBase64Encode(bytes_str, base64::base64EncodeTable);


Does this need a null check?

This returns just a std::string.

builtins/web/form-data/form-data-encoder.cpp

tschneidereit · 2025-01-25T16:43:11Z

builtins/web/form-data/form-data-encoder.cpp

+  auto leftover = datasz - written;
+  if (leftover > 0) {
+    MOZ_ASSERT(remainder_.empty());
+    remainder_.assign(first + written, last);


Is this an infallible operation? It seems like it should need to allocate memory, so I'm not sure I understand how that works. And ideally, if it does allocate, we should make that fallible—or at least assert that it didn't fail and abort execution.

Well, it technically can throw bad_alloc because it is internally calling std::string::reserve if needed - I guess this will abort excution.

https://cplusplus.com/reference/string/string/assign/

A bad_alloc exception is thrown if the function needs to allocate storage and fails.

andreiltd · 2025-01-27T10:04:32Z

Hi @tschneidereit, as always, thank you so much for a through review!

One thing that I can't fully evaluate, because it'll presumably be part of the implementation of encode_stream: IIUC, field names, will always be first have their newlines normalized to CRLF, and then have those escaped. It'd be good to fold those into one operation, if possible.

So the spec says this:

For each entry of entry list:

Replace every occurrence of U+000D (CR) not followed by U+000A (LF), and every occurrence of U+000A (LF) not preceded by U+000D (CR), in entry's name, by a string consisting of a U+000D (CR) and U+000A (LF).

If entry's value is not a File object, then replace every occurrence of U+000D (CR) not followed by U+000A (LF), and every occurrence of U+000A (LF) not preceded by U+000D (CR), in entry's value, by a string consisting of a U+000D (CR) and U+000A (LF).

It's really hard to follow the spec closely because the implementation is state-machine-based, and a lot of actions that the spec calls for correspond to different states in the implementation and cannot be done together. For example, processing both names and values: names are part of the entry-header state, whereas values are handled in the entry-body state.

andreiltd · 2025-02-06T13:22:43Z

I've adopted a bunch of wpt tests here as an integration test. Those should ensure the we serialize FormData according to the spec.

Unfortunately, I wasn't able to run those in wpt tests because of few issues:

it looks like meta script functions are not visible from the test, e.g.: functions from META: script=../support/send-file-formdata-helper.js are missing when running this test
the wpt is hosting a bunch of python scripts and make the calls from within the tests, like this one:

    const formDataText = await (await fetch(
      `/fetch/api/resources/echo-content.py`,
      {
        method: "POST",
        body: formData,
      },
    )).text();

This returns an empty string.

tschneidereit · 2025-02-06T14:32:24Z

I've adopted here a bunch of wpt tests here as an integration test. Those should ensure the we serialize FormData according to the spec.

That's a great idea!

Unfortunately, I wasn't able to run those in wpt tests because of few issues:

Can you say more about how you're trying to run the test? I can't see where in the PR the META: script line appears, or where the fetch to the python scripts is happening.

Taking a step back though, to fully adapt a test from WPT requires at least these steps:

Remove all META: script lines with inlined (or imported versions) of that code, because these are only interpreted in the special WPT harness I wrote
Remove all fetch requests to python scripts, because that server is only up while the WPT tests are running. The best way to do this is probably to change the fetch event handler to check for the url, e.g. /fetch/api/resources/echo-content.py from your example, and implement the logic of the script in JS. (Which in this case I guess means simply returning the incoming body as the outgoing one.)
Ensuring that we retain the proper license. WPT code is licensed under 3-clause BSD, and we need to keep that. Probably the best way to do this is to have all tests derived from WPT in a separate folder, and add the WPT LICENSE.md file to that folder, with line 3 changed to Copyright © web-platform-tests contributors, StarlingMonkey contributors

andreiltd · 2025-02-06T16:43:38Z

Can you say more about how you're trying to run the test?

Oh, I was just trying to run wpt tests directly by whitelisting them in the tests.json file. Those are the tests: https://github.com/web-platform-tests/wpt/tree/master/FileAPI/file

The failures that I described were from running wpt-tests so I ended up rewriting them, exactly as yo said, and include in our integration tests.

Ensuring that we retain the proper license.

That's a good point. Would tests/integration/wpt be a good directory for maintaining the wpt derived tests?

tschneidereit · 2025-02-06T18:52:26Z

That's a good point. Would tests/integration/wpt be a good directory for maintaining the wpt derived tests?

It would be, yes. Though if you're only doing this to work around weird issues with running WPT tests, then I would overall prefer figuring out what causes them. Can you give me steps to reproduce for the issue you were seeing with this test?

andreiltd · 2025-02-06T20:16:34Z

I added the test like this:

diff --git a/tests/wpt-harness/tests.json b/tests/wpt-harness/tests.json
index 349baa1..fb757b9 100644
--- a/tests/wpt-harness/tests.json
+++ b/tests/wpt-harness/tests.json
@@ -175,6 +175,7 @@
   "FileAPI/blob/Blob-stream.any.js",
   "FileAPI/blob/Blob-text.any.js",
   "FileAPI/file/File-constructor.any.js",
+  "FileAPI/file/send-file-formdata.any.js",
   "hr-time/basic.any.js",
   "hr-time/idlharness.any.js",
   "hr-time/monotonic-clock.any.js",

and then running

just wpt-test FileAPI/file/send-file-formdata.any.js

gives me this error:

1: Running test FileAPI/file/send-file-formdata.any.js
1: Sending request to http://127.0.0.1:7676/FileAPI/file/send-file-formdata.any.js
1: wasmtime stderr: stderr [0] :: Warning: Using a DEBUG build. Expect things to be SLOW.
1: wasmtime stdout: stdout [0] :: Log: running test /FileAPI/file/send-file-formdata.any.js
1: wasmtime stdout: stdout [0] :: Log: error: ReferenceError: formDataPostFileUploadTest is not defined, stack:
1: stdout [0] :: @/FileAPI/file/send-file-formdata.any.js:5:3
1: stdout [0] :: [email protected]:5043:5
1: stdout [0] :: [email protected]:5024:19
1: stdout [0] :: async*@wpt-test-runner.js:5073:54
1: stdout [0] ::

andreiltd · 2025-02-06T20:20:11Z

Just looking at how we import meta scripts here it looks like the meta scripts are evaluated separately from the test, shouldn't the meta and the test be concatenated instead?

tschneidereit · 2025-02-06T20:41:36Z

Just looking at how we import meta scripts here it looks like the meta scripts are evaluated separately from the test, shouldn't the meta and the test be concatenated instead?

I don't entirely remember why I landed on this exact way of loading META scripts. I do remember that it was non-trivial to get things working, and included this weird thing where we replace let and const with var bindings. If instead we could just concatenate things, that'd be nice. It's possible that the key reason was to ensure that we run the test itself without changing the lines numbers: if we concatenate everything, we can't do that anymore. But as I said, I don't entirely remember

andreiltd · 2025-02-07T15:05:53Z

Yeah, it would be really nice to have those tests working, it's worth dozens of wpt passes. Let me know if you plan to investigate otherwise I will allocate some time to it for next week. 🙂

andreiltd · 2025-02-10T18:38:40Z

Merging #209 Should fix failing wpt tests in this PR.

andreiltd marked this pull request as draft January 21, 2025 14:21

andreiltd force-pushed the form-data-request branch from 591475c to 1c57fa0 Compare January 22, 2025 13:17

andreiltd marked this pull request as ready for review January 22, 2025 14:02

andreiltd mentioned this pull request Jan 22, 2025

Add just recipe for serving the js script #202

Merged

Add FormData streaming encoder

655b57d

andreiltd force-pushed the form-data-request branch from 1c57fa0 to 655b57d Compare January 23, 2025 08:04

andreiltd added 3 commits January 23, 2025 10:46

Fix setting content type and use base64 for boundary encoding

493a46c

Update wpt expectations

0243ac2

Remove unused header

58e0404

tschneidereit requested changes Jan 25, 2025

View reviewed changes

Address code review feedback

ace478d

andreiltd mentioned this pull request Feb 5, 2025

Support FormData, Request.formData #116

Open

Add integration tests

b739d56

Move form-data-encode test from fetch to form-data

ed556fc

andreiltd mentioned this pull request Feb 7, 2025

Implement multipart parser #207

Open

tschneidereit mentioned this pull request Feb 9, 2025

Fix loading of WPT META scripts #209

Open

Implement query_length function

f6fd729

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implementation of `MultipartFormData` streaming encoder #200

Add implementation of `MultipartFormData` streaming encoder #200

andreiltd commented Jan 21, 2025 •

edited

Loading

andreiltd commented Jan 22, 2025 •

edited

Loading

tschneidereit commented Jan 24, 2025

andreiltd commented Jan 24, 2025

tschneidereit left a comment

tschneidereit Jan 25, 2025

andreiltd Jan 27, 2025 •

edited

Loading

andreiltd Feb 11, 2025

tschneidereit Jan 25, 2025

andreiltd Jan 27, 2025

tschneidereit Jan 25, 2025

andreiltd Jan 27, 2025

tschneidereit Jan 25, 2025

andreiltd Jan 27, 2025

andreiltd commented Jan 27, 2025 •

edited

Loading

andreiltd commented Feb 6, 2025 •

edited

Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 6, 2025 •

edited

Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 6, 2025

andreiltd commented Feb 6, 2025 •

edited

Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 7, 2025

andreiltd commented Feb 10, 2025

Add implementation of MultipartFormData streaming encoder #200

Are you sure you want to change the base?

Add implementation of MultipartFormData streaming encoder #200

Conversation

andreiltd commented Jan 21, 2025 • edited Loading

andreiltd commented Jan 22, 2025 • edited Loading

tschneidereit commented Jan 24, 2025

andreiltd commented Jan 24, 2025

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreiltd Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreiltd commented Jan 27, 2025 • edited Loading

andreiltd commented Feb 6, 2025 • edited Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 6, 2025 • edited Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 6, 2025

andreiltd commented Feb 6, 2025 • edited Loading

tschneidereit commented Feb 6, 2025

andreiltd commented Feb 7, 2025

andreiltd commented Feb 10, 2025

Add implementation of `MultipartFormData` streaming encoder #200

Add implementation of `MultipartFormData` streaming encoder #200

andreiltd commented Jan 21, 2025 •

edited

Loading

andreiltd commented Jan 22, 2025 •

edited

Loading

andreiltd Jan 27, 2025 •

edited

Loading

andreiltd commented Jan 27, 2025 •

edited

Loading

andreiltd commented Feb 6, 2025 •

edited

Loading

andreiltd commented Feb 6, 2025 •

edited

Loading

andreiltd commented Feb 6, 2025 •

edited

Loading