From 23beba030a29ec4f2471bd49d809132fa42409db Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 5 Jun 2024 14:13:37 -0700 Subject: [PATCH 1/4] feat: support task deadlines (#58) --- builtins/web/timers.cpp | 4 ++++ include/extension-api.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/builtins/web/timers.cpp b/builtins/web/timers.cpp index 340c852..1923d1e 100644 --- a/builtins/web/timers.cpp +++ b/builtins/web/timers.cpp @@ -68,6 +68,10 @@ class TimerTask final : public api::AsyncTask { return true; } + [[nodiscard]] uint64_t deadline() override { + return deadline_; + } + void trace(JSTracer *trc) override { TraceEdge(trc, &callback_, "Timer callback"); for (auto &arg : arguments_) { diff --git a/include/extension-api.h b/include/extension-api.h index f6558b4..98a4783 100644 --- a/include/extension-api.h +++ b/include/extension-api.h @@ -127,6 +127,10 @@ class AsyncTask { return handle_; } + [[nodiscard]] virtual uint64_t deadline() { + return 0; + } + virtual void trace(JSTracer *trc) = 0; /** From 81391c83d0cd355661c9a636fbc5b472425c5e8b Mon Sep 17 00:00:00 2001 From: Till Schneidereit Date: Mon, 17 Jun 2024 01:52:41 +0200 Subject: [PATCH 2/4] Steal and free buffers when writing chunks from a ReadableStream to an outgoing WASI stream --- builtins/web/fetch/request-response.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/builtins/web/fetch/request-response.cpp b/builtins/web/fetch/request-response.cpp index 76feaea..fb29898 100644 --- a/builtins/web/fetch/request-response.cpp +++ b/builtins/web/fetch/request-response.cpp @@ -979,17 +979,16 @@ bool reader_for_outgoing_body_then_handler(JSContext *cx, JS::HandleObject body_ return false; } - host_api::Result res; - { - JS::AutoCheckCannotGC nogc; - JSObject *array = &val.toObject(); - bool is_shared; - uint8_t *bytes = JS_GetUint8ArrayData(array, &is_shared, nogc); - size_t length = JS_GetTypedArrayByteLength(array); - // TODO: change this to write in chunks, respecting backpressure. - auto body = RequestOrResponse::outgoing_body_handle(body_owner); - res = body->write_all(bytes, length); - } + RootedObject array(cx, &val.toObject()); + size_t length = JS_GetTypedArrayByteLength(array); + bool is_shared; + RootedObject buffer(cx, JS_GetArrayBufferViewBuffer(cx, array, &is_shared)); + MOZ_ASSERT(!is_shared); + auto bytes = static_cast(StealArrayBufferContents(cx, buffer)); + // TODO: change this to write in chunks, respecting backpressure. + auto body = RequestOrResponse::outgoing_body_handle(body_owner); + auto res = body->write_all(bytes, length); + js_free(bytes); // Needs to be outside the nogc block in case we need to create an exception. if (auto *err = res.to_err()) { From 16b28eab975327a09b147a3615acaf46b8406797 Mon Sep 17 00:00:00 2001 From: Till Schneidereit Date: Mon, 17 Jun 2024 01:54:52 +0200 Subject: [PATCH 3/4] Don't special-case TransformStream readables when streaming outgoing bodies This special-casing used to make sense in the JS-Compute runtime, where certain TransformStreams were more optimizable. It doesn't anymore, and in any case would have to come with actually reading from the stream, which the code as written before this patch didn't :shrug: --- builtins/web/fetch/request-response.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/builtins/web/fetch/request-response.cpp b/builtins/web/fetch/request-response.cpp index fb29898..82d975a 100644 --- a/builtins/web/fetch/request-response.cpp +++ b/builtins/web/fetch/request-response.cpp @@ -1050,14 +1050,6 @@ bool RequestOrResponse::maybe_stream_body(JSContext *cx, JS::HandleObject body_o return false; } - if (streams::TransformStream::is_ts_readable(cx, stream)) { - JSObject *ts = streams::TransformStream::ts_from_readable(cx, stream); - if (streams::TransformStream::readable_used_as_body(ts)) { - *requires_streaming = true; - return true; - } - } - // If the body stream is backed by an HTTP body handle, we can directly pipe // that handle into the body we're about to send. if (streams::NativeStreamSource::stream_is_body(cx, stream)) { From 7631f4506909be013ae5f2265e50694eda35ad44 Mon Sep 17 00:00:00 2001 From: Till Schneidereit Date: Tue, 18 Jun 2024 15:13:45 +0200 Subject: [PATCH 4/4] Improve handling of GC managed array buffers Specifically, some of the `noGC` guards had issues that could lead to panics during exception throwing, and some of the loops over multiple buffers where overly complex. --- builtins/web/base64.cpp | 38 ++++++----- builtins/web/crypto/crypto.cpp | 1 + builtins/web/fetch/request-response.cpp | 71 +++++++-------------- builtins/web/streams/compression-stream.cpp | 2 +- 4 files changed, 42 insertions(+), 70 deletions(-) diff --git a/builtins/web/base64.cpp b/builtins/web/base64.cpp index 5b34dd2..956ad57 100644 --- a/builtins/web/base64.cpp +++ b/builtins/web/base64.cpp @@ -20,35 +20,33 @@ JS::Result convertJSValueToByteString(JSContext *cx, JS::Handle(JS::Error()); + } + + for (size_t i = 0; i < length; i++) { + if (chars[i] > 255) { + // Reset the nogc guard, since otherwise we can't throw errors. + nogc.reset(); JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_INVALID_CHARACTER_ERROR); return JS::Result(JS::Error()); } - - for (size_t i = 0; i < length; i++) { - if (chars[i] > 255) { - foundBadChar = true; - break; - } - } - } - - if (foundBadChar) { - JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_INVALID_CHARACTER_ERROR); - return JS::Result(JS::Error()); } + auto latin1_z = + JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, chars, length); + result = UniqueChars(reinterpret_cast(latin1_z.get())); } else { length = JS::GetStringLength(s); + result = JS_EncodeStringToLatin1(cx, s); } - UniqueChars result = JS_EncodeStringToLatin1(cx, s); if (!result) { return JS::Result(JS::Error()); } diff --git a/builtins/web/crypto/crypto.cpp b/builtins/web/crypto/crypto.cpp index 773f9bc..3504f9b 100644 --- a/builtins/web/crypto/crypto.cpp +++ b/builtins/web/crypto/crypto.cpp @@ -54,6 +54,7 @@ bool Crypto::get_random_values(JSContext *cx, unsigned argc, JS::Value *vp) { auto res = host_api::Random::get_bytes(byte_length); if (auto *err = res.to_err()) { + noGC.reset(); HANDLE_ERROR(cx, *err); return false; } diff --git a/builtins/web/fetch/request-response.cpp b/builtins/web/fetch/request-response.cpp index 82d975a..f565b50 100644 --- a/builtins/web/fetch/request-response.cpp +++ b/builtins/web/fetch/request-response.cpp @@ -621,71 +621,44 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand if (!JS::GetArrayLength(cx, contents, &contentsLength)) { return false; } - // TODO(performance): investigate whether we can infer the size directly from `contents` - size_t buf_size = HANDLE_READ_CHUNK_SIZE; - // TODO(performance): make use of malloc slack. - // https://github.com/fastly/js-compute-runtime/issues/217 - size_t offset = 0; - // In this loop we are finding the length of each entry in `contents` and resizing the `buf` - // until it is large enough to fit all the entries in `contents` - for (uint32_t index = 0; index < contentsLength; index++) { - JS::RootedValue val(cx); + + size_t total_length = 0; + RootedValue val(cx); + + for (size_t index = 0; index < contentsLength; index++) { if (!JS_GetElement(cx, contents, index, &val)) { return false; } - { - JS::AutoCheckCannotGC nogc; - MOZ_ASSERT(val.isObject()); - JSObject *array = &val.toObject(); - MOZ_ASSERT(JS_IsUint8Array(array)); - size_t length = JS_GetTypedArrayByteLength(array); - if (length) { - offset += length; - // if buf is not big enough to fit the next uint8array's bytes then resize - if (offset > buf_size) { - buf_size = - buf_size + (HANDLE_READ_CHUNK_SIZE * ((length / HANDLE_READ_CHUNK_SIZE) + 1)); - } - } - } + JSObject *array = &val.toObject(); + size_t length = JS_GetTypedArrayByteLength(array); + total_length += length; } - JS::UniqueChars buf{static_cast(JS_malloc(cx, buf_size + 1))}; + JS::UniqueChars buf{static_cast(JS_malloc(cx, total_length))}; if (!buf) { JS_ReportOutOfMemory(cx); return false; } - // reset the offset for the next loop - offset = 0; + + size_t offset = 0; // In this loop we are inserting each entry in `contents` into `buf` for (uint32_t index = 0; index < contentsLength; index++) { - JS::RootedValue val(cx); if (!JS_GetElement(cx, contents, index, &val)) { return false; } - { - JS::AutoCheckCannotGC nogc; - MOZ_ASSERT(val.isObject()); - JSObject *array = &val.toObject(); - MOZ_ASSERT(JS_IsUint8Array(array)); - bool is_shared; - size_t length = JS_GetTypedArrayByteLength(array); - if (length) { - static_assert(CHAR_BIT == 8, "Strange char"); - auto bytes = reinterpret_cast(JS_GetUint8ArrayData(array, &is_shared, nogc)); - memcpy(buf.get() + offset, bytes, length); - offset += length; - } - } - } - buf[offset] = '\0'; -#ifdef DEBUG - bool foundBodyParser; - if (!JS_HasElement(cx, catch_handler, 2, &foundBodyParser)) { - return false; + JSObject *array = &val.toObject(); + bool is_shared; + size_t length = JS_GetTypedArrayByteLength(array); + JS::AutoCheckCannotGC nogc(cx); + auto bytes = reinterpret_cast(JS_GetUint8ArrayData(array, &is_shared, nogc)); + memcpy(buf.get() + offset, bytes, length); + offset += length; } + + mozilla::DebugOnly foundBodyParser = false; + MOZ_ASSERT(JS_HasElement(cx, catch_handler, 2, &foundBodyParser)); MOZ_ASSERT(foundBodyParser); -#endif + // Now we can call parse_body on the result JS::RootedValue body_parser(cx); if (!JS_GetElement(cx, catch_handler, 2, &body_parser)) { diff --git a/builtins/web/streams/compression-stream.cpp b/builtins/web/streams/compression-stream.cpp index 7774f28..90fc328 100644 --- a/builtins/web/streams/compression-stream.cpp +++ b/builtins/web/streams/compression-stream.cpp @@ -129,7 +129,7 @@ bool deflate_chunk(JSContext *cx, JS::HandleObject self, JS::HandleValue chunk, { bool is_shared; - JS::AutoCheckCannotGC nogc; + JS::AutoCheckCannotGC nogc(cx); uint8_t *out_buffer = JS_GetUint8ArrayData(out_obj, &is_shared, nogc); memcpy(out_buffer, buffer, bytes); }