[karthik2804]

This commit moves the existing TextDecoder and TextEncoder implementations into a new encoding crate, and adds the corresponding stream classes.

[tschneidereit]

Thank you for this! I left a whole bunch of comments, a few of which definitely need addressing. Please push changes addressing them as new commits instead of force-pushing changes to the existing commit, so I can see what changed.

(Note: I'll probably make sense to start reading the comments in text_encoder_stream.rs first, because I started commenting there first, and some comments in text_decoder_stream.rs might make less sense otherwise.)

[tschneidereit]

no_trace doesn't have a name parameter. This doesn't cause a compile error only because the parameter is ignored.

Suggested change

	#[no_trace(name = "ignoreBOM")]
	#[no_trace]

[tschneidereit]

Function::new_callback takes impl ToJSVal, so no need to convert to a value here. You can just pass self to new_callback

Suggested change

let self_payload = scope.root_value(self.as_value());

[tschneidereit]

Suggested change

	self_payload,
	self,

[tschneidereit]

Suggested change

	self_payload,
	self,

[tschneidereit]

This is unfortunately unsafe: it allows code to intercept the set_property calls by installing setters on Object.prototype, and thus modify internal behavior of the builtin.

Additionally, if Object.prototype has start or cancel functions or a readableType member, those would all be used in setting up the TransformStream, and expose / enable modifying internal behavior.

This can be worked around by creating an object without a prototype. However, in this case we should do something entirely different to begin with: use the native transform and flush algorithms directly.

The way that could work is by mirroring ReadableStream::new_native, with a new function with this signature:

impl TransformStream<'_> {
    /// <https://streams.spec.whatwg.org/#transformstream-set-up>
    pub fn new_native<'r>(
        scope: &'r Scope<'_>,
        transform: AlgorithmArg<'r>,
        flush: AlgorithmArg<'r>,
        cancel: AlgorithmArg<'r>,
    ) -> Result<TransformStream<'r>, ExnThrown>;
}

The body would roughly be:

• js::class::create_instance_with::<TransformStreamImpl>, as create_readable_stream does.
• initialize_transform_stream with a start promise resolved with undefined, writable high-water mark 1, readable high-water mark 0, and no size algorithms.
• TransformStreamDefaultController::new.
• set_up_transform_stream_default_controller, passing AlgorithmArg::Native for the algorithms.

You could then avoid creating JSFunctions for transform and flush entirely.

[tschneidereit]

This is unsafe, because it retains the data() borrow across the calls into JS that enqueue can trigger. Those could then call back into the EncoderStream and take a data_mut, causing a mutable borrow to coexist with an immutable one.

[tschneidereit]

This step doesn't exist in the spec at all. Make sure to double-check that the step comments are fully accurate throughout the implementation, please. (The actual operation also happens to be redundant, since leading_surrogate is initialized to its default None value anyway.)

[tschneidereit]

I think you should be able to return an &'static str here, but that might be wrong. Would be good to try though, to avoid the allocation of a string we'll throw away immediately.

[karthik2804]

This works!

[tschneidereit]

This can also use cast_payload

[tschneidereit]

I'm afraid this needs another fairly involved rebase—my apologies for that!

I left a bunch of comments inline pointing out the most important implications of the rebase, as well as some unrelated things that should change before merging.

Additionally, some high-level comments:

• the decode_to_utf16 on main has a bunch of optimizations that reduce the amount of copying and calling into JS we do per chunk. Please turn that into a reusable helper used by both the streaming and non-streaming decoder, without losing the optimizations. In general, reuse / unify as much as possible between the streaming and non-streaming versions of the en/decoder. That will also make the new builtins inherit some work I did on catching memory allocation issues. Right now, there are a number of places where we allocate large buffers (e.g. using Vec!) in infallible ways, which would cause us to trap and kill the entire instance instead of throwing an error. For small allocations that's okay (since we're close to the limit anyway) but for large ones, such as buffers for large stream chunks, it's something we should avoid.
• most of the tests duplicate coverage by WPT. In the interest of minimizing CI time, let's make sure to deduplicate them. I had good success with telling Claude to identify and eliminate tests that duplicate WPT. You can also see examples of this kind of trimming I did to various builtins, including the encoding ones, here. (And incidentally that commit is also where most of the changes affecting this one come from.)
• please add the new web-encoding crate to the list at test-gc-zeal.sh:92, to ensure the moved and new builtins get proper GC testing coverage. (Yes, this is not ideal and we should improve the mechanism here.)

[tschneidereit]

let's call this web-encoding—both to stay with the naming scheme and to not have conflicts with the existing, if unmaintained encoding crate.

[tschneidereit]

bytes() doesn't exist anymore. Instead, there is now a new with_bytes() method that takes a callback. It's set up in a way that makes calling into JS (which would be incorrect while holding a reference to a buffer's underlying bytes because it could move them) impossible without extensive shenanigans.

[tschneidereit]

Same as the general comment: deduplicating this logic between streaming and non-streaming builtins would be good. Right now, we have differing error messages without a good reason, for example.

[tschneidereit]

No need to duplicate things from dependencies in dev-dependencies

Suggested change

js.workspace = true

[tschneidereit]

I think this is unused?

Suggested change

libstarling.workspace = true

[tschneidereit]

Same duplication as above

Suggested change

web-streams.workspace = true

[tschneidereit]

Very close! There's a bunch more feedback inline, some of which I swear I had posted in the last round, but either I forgot to click the green button on the drafts, or GitHub just ignored them—who's to say!

[tschneidereit]

please also change the directory name to web-encoding

[tschneidereit]

These are useful (and will be used) by other builtins, not just web-encoding.

Suggested change

	/// Helpers for the encoding crate — not part of the WebIDL interface.
	/// Helpers for native consumers of TransformStreams.

[tschneidereit]

I started leaving a bunch of feedback, and then accidentally spelled out a full suggested replacement. This version saves two full buffer copies, a call into JS for char_at, and an infallible allocation for the Vec:

Suggested change

	/// The <https://encoding.spec.whatwg.org/#encode-and-enqueue-a-chunk> algorithm.
	fn encoder_transform_cb(
	scope: &Scope<'_>,
	args: CallbackArgs<'_>,
	payload: HandleValue<'_>,
	) -> Result<Value, ExnThrown> {
	let stream = cast_payload::<TextEncoderStream>(scope, payload);
	// Step 1: Let input be the result of converting chunk to a DOMString.
	let js_string = JSString::from_value(scope, args.get(0))?;
	// Step 2: Convert input to an I/O queue of code units.
	// Step 3: Let output be the I/O queue of bytes « end-of-queue ».
	// Step 4: While true:
	// Step 4.1: Let item be the result of reading from input.
	// Step 4.2: If item is end-of-queue:
	// Step 4.2.1: Convert output into a byte sequence.
	// Step 4.2.2: If output is not empty:
	// Step 4.2.2.1: Let chunk be the result of creating a Uint8Array object given output and
	// encoder’s relevant realm.
	// Step 4.2.2.2: Enqueue chunk into encoder’s TransformStream.
	// Step 4.2.3: Return.
	// Step 4.3: Let result be the result of executing the convert code unit to scalar value
	// algorithm with encoder, item, and input.
	// Step 4.4: If result is not continue, then process an item with result, encoder’s encoder,
	// input, output, and "fatal".
	// Implementation note: a surrogate pair can only be split at the chunk's edges, so only the
	// first and last code units need the [[pending high surrogate]] handling. Everything between
	// is encoded in bulk by `to_utf8`, which replaces lone surrogates with U+FFFD exactly as
	// steps 4.3 and 4.4 do.
	let mut start = 0;
	let mut end = js_string.len();
	if end == 0 {
	return Ok(value::undefined());
	}
	let mut output = Vec::new();
	let pending = stream.data_mut().leading_surrogate.take();
	if let Some(leading) = pending {
	let first = js_string.char_at(scope, 0)?;
	if is_trailing_surrogate(first) {
	let c = 0x10000 + ((u32::from(leading) - 0xD800) << 10) + (u32::from(first) - 0xDC00);
	push_scalar_value(&mut output, c);
	start = 1;
	} else {
	push_scalar_value(&mut output, 0xFFFD);
	}
	}
	if start < end {
	let last = js_string.char_at(scope, end - 1)?;
	if is_leading_surrogate(last) {
	stream.data_mut().leading_surrogate = Some(last);
	end -= 1;
	}
	}
	if start < end {
	let middle = if start == 0 && end == js_string.len() {
	js_string
	} else {
	js_string.substring(scope, start, end - start)?
	};
	let utf8 = middle.to_utf8(scope)?.into_bytes();
	if output.is_empty() {
	output = utf8;
	} else {
	output
	.try_reserve(utf8.len())
	.map_err(|_| OutOfMemory.throw(scope))?;
	output.extend_from_slice(&utf8);
	}
	}
	if !output.is_empty() {
	let u8array = Uint8Array::with_data(scope, &output)?;
	enqueue_to_transform(scope, &stream, u8array)?;
	}
	Ok(value::undefined())
	}
	fn is_leading_surrogate(unit: u16) -> bool {
	(0xD800..=0xDBFF).contains(&unit)
	}
	fn is_trailing_surrogate(unit: u16) -> bool {
	(0xDC00..=0xDFFF).contains(&unit)
	}
	/// The <https://encoding.spec.whatwg.org/#encode-and-enqueue-a-chunk> algorithm.
	fn encoder_transform_cb(
	scope: &Scope<'_>,
	args: CallbackArgs<'_>,
	payload: HandleValue<'_>,
	) -> Result<Value, ExnThrown> {
	let stream = cast_payload::<TextEncoderStream>(scope, payload);
	// Step 1: Let input be the result of converting chunk to a DOMString.
	let input = JSString::from_value(scope, args.get(0))?;
	// Step 2: Convert input to an I/O queue of code units.
	// Step 3: Let output be the I/O queue of bytes « end-of-queue ».
	// Step 4: While true:
	// Step 4.1: Let item be the result of reading from input.
	// Step 4.2: If item is end-of-queue:
	// Step 4.2.1: Convert output into a byte sequence.
	// Step 4.2.2: If output is not empty:
	// Step 4.2.2.1: Let chunk be the result of creating a Uint8Array object given output and
	// encoder’s relevant realm.
	// Step 4.2.2.2: Enqueue chunk into encoder’s TransformStream.
	// Step 4.2.3: Return.
	// Step 4.3: Let result be the result of executing the convert code unit to scalar value
	// algorithm with encoder, item, and input.
	// Step 4.4: If result is not continue, then process an item with result, encoder’s encoder,
	// input, output, and "fatal".
	//
	// (`encoding_rs` runs this loop over the whole chunk, and replaces unpaired surrogates with
	// U+FFFD. It doesn't keep a high surrogate between calls, so we encode the pending one
	// together with the chunk's first code unit, and hold back a high surrogate that ends the
	// chunk.)
	if input.is_empty() {
	return Ok(value::undefined());
	}
	let pending = stream.data_mut().leading_surrogate.take();
	// SAFETY: the closure doesn't call into SpiderMonkey.
	let (output, pending) = unsafe {
	input.with_chars(scope, |chars| match chars {
	StrChars::Latin1(chars) => (encode_latin1(pending, chars), None),
	StrChars::TwoByte(chars) => encode_utf16(pending, chars),
	})
	}?;
	stream.data_mut().leading_surrogate = pending;
	let output = output.ok_or_else(|| OutOfMemory.throw(scope))?;
	if !output.is_empty() {
	let chunk = Uint8Array::with_data(scope, &output)?;
	enqueue_to_transform(scope, &stream, chunk)?;
	}
	Ok(value::undefined())
	}
	/// Returns the UTF-8 encoding of `chars`, preceded by U+FFFD if `pending` is set, or `None` if
	/// allocating it fails. Latin-1 characters aren't surrogates, so `pending` is always unpaired.
	fn encode_latin1(pending: Option<u16>, chars: &[u8]) -> Option<Vec<u8>> {
	let head = pending.as_slice();
	let mut output = zeroed_buffer(head.len() * 3 + chars.len() * 2)?;
	let mut written = encoding_rs::mem::convert_utf16_to_utf8(head, &mut output);
	written += encoding_rs::mem::convert_latin1_to_utf8(chars, &mut output[written..]);
	output.truncate(written);
	Some(output)
	}
	/// Returns the UTF-8 encoding of `chars` preceded by the high surrogate `pending`, or `None` if
	/// allocating it fails. A high surrogate at the end of `chars` isn't encoded. It is returned as
	/// the new pending high surrogate.
	fn encode_utf16(pending: Option<u16>, chars: &[u16]) -> (Option<Vec<u8>>, Option<u16>) {
	let mut head = [0; 2];
	let mut head_len = 0;
	let mut rest = chars;
	if let Some(high) = pending {
	head[0] = high;
	head_len = 1;
	if let Some((&first, tail)) = rest.split_first() {
	if is_trailing_surrogate(first) {
	head[1] = first;
	head_len = 2;
	rest = tail;
	}
	}
	}
	let mut new_pending = None;
	if let Some((&last, init)) = rest.split_last() {
	if is_leading_surrogate(last) {
	new_pending = Some(last);
	rest = init;
	}
	}
	let Some(mut output) = zeroed_buffer((head_len + rest.len()) * 3) else {
	return (None, new_pending);
	};
	let mut written = encoding_rs::mem::convert_utf16_to_utf8(&head[..head_len], &mut output);
	written += encoding_rs::mem::convert_utf16_to_utf8(rest, &mut output[written..]);
	output.truncate(written);
	(Some(output), new_pending)
	}
	/// Returns `len` zero bytes, or `None` if allocating them fails.
	fn zeroed_buffer(len: usize) -> Option<Vec<u8>> {
	if len == 0 {
	return Some(Vec::new());
	}
	let layout = std::alloc::Layout::array::<u8>(len).ok()?;
	// SAFETY: `layout` has a non-zero size.
	let ptr = unsafe { std::alloc::alloc_zeroed(layout) };
	// SAFETY: `ptr` was allocated by the global allocator with `layout`, and its `len` bytes are
	// initialized to zero.
	(!ptr.is_null()).then(|| unsafe { Vec::from_raw_parts(ptr, len, len) })
	}
	fn is_leading_surrogate(unit: u16) -> bool {
	(0xD800..=0xDBFF).contains(&unit)
	}
	fn is_trailing_surrogate(unit: u16) -> bool {
	(0xDC00..=0xDFFF).contains(&unit)
	}

[tschneidereit]

This is functionally identical to core_runtime::test_util::eval_out_with_setup, so let's use that instead.