ci: StarlingMonkey WPT debug build (#896)

Author: Guy Bedford

.github/workflows/main.yml

@@ -485,6 +485,62 @@ jobs:
       timeout-minutes: 20
       run: node ./tests/wpt-harness/run-wpt.mjs --starlingmonkey -vv
 
+  starlingmonkey-run_wpt-debug:
+    concurrency:
+      group: ${{ github.head_ref }}-${{ github.workflow}}-starlingmonkey-run_wpt-debug
+      cancel-in-progress: true
+    if: github.ref != 'refs/heads/main'
+    name: Run Web Platform Tests (starlingmonkey)
+    needs: [starlingmonkey-build, ensure_cargo_installs]
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+      with:
+        submodules: true
+    - uses: actions/setup-node@v3
+      with:
+        node-version: 20
+        cache: 'yarn'
+
+    - name: Download Engine
+      uses: actions/download-artifact@v3
+      with:
+        name: starling-debug
+
+    - name: Restore Viceroy from cache
+      uses: actions/cache@v3
+      with:
+        path: "/home/runner/.cargo/bin/viceroy"
+        key: crate-cache-viceroy-${{ env.viceroy_version }}
+    
+    - name: Restore wasm-tools from cache
+      uses: actions/cache@v3
+      id: wasm-tools
+      with:
+        path: "/home/runner/.cargo/bin/wasm-tools"
+        key: crate-cache-wasm-tools-${{ env.wasm-tools_version }}
+    
+    - name: "Check wasm-tools has been restored"
+      if: steps.wasm-tools.outputs.cache-hit != 'true'
+      run: |
+        echo "wasm-tools was not restored from the cache"
+        echo "bailing out from the build early"
+        exit 1
+    
+    - run: yarn install --frozen-lockfile
+
+    - name: Build WPT runtime
+      run: tests/wpt-harness/build-wpt-runtime.sh --debug-build
+
+    - name: Prepare WPT hosts
+      run: |
+        cd tests/wpt-harness/wpt
+        ./wpt make-hosts-file | sudo tee -a /etc/hosts
+
+    - name: Run tests
+      timeout-minutes: 20
+      run: node ./tests/wpt-harness/run-wpt.mjs --starlingmonkey -vv
+
   starlingmonkey-run_wpt-weval:
     concurrency:
       group: ${{ github.head_ref }}-${{ github.workflow}}-starlingmonkey-run_wpt-weval

package.json

@@ -34,6 +34,7 @@
     "test:cli": "brittle --bail integration-tests/cli/**.test.js",
     "test:integration": "node ./integration-tests/js-compute/test.js",
     "test:wpt": "tests/wpt-harness/build-wpt-runtime.sh && node ./tests/wpt-harness/run-wpt.mjs -vv",
+    "test:wpt:debug": "tests/wpt-harness/build-wpt-runtime.sh --debug-build && node ./tests/wpt-harness/run-wpt.mjs --starlingmonkey -vv",
     "test:types": "tsd",
     "build": "make -j8 -C runtime/js-compute-runtime && cp runtime/js-compute-runtime/js-compute-runtime.wasm .",
     "build:debug": "DEBUG=true make -j8 -C runtime/js-compute-runtime && cp runtime/js-compute-runtime/js-compute-runtime.wasm .",

runtime/StarlingMonkey

@@ -1,6 +1,7 @@
 #include "./headers.h"
 #include "../../../StarlingMonkey/runtime/encode.h"
 #include "../../../StarlingMonkey/runtime/sequence.hpp"
+#include "../../common/sequence.hpp"
 #include "../../host-api/host_api_fastly.h"
 #include "../fastly.h"
 #include "./request-response.h"
@@ -144,19 +145,30 @@ host_api::HostString normalize_header_value(JSContext *cx, JS::MutableHandleValu
     return nullptr;
   }
 
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-  if (!JS_DeprecatedStringHasLatin1Chars(value_str)) {
-#pragma clang diagnostic pop
-    JS::AutoCheckCannotGC nogc;
-    size_t length;
-    const char16_t *chars = JS_GetTwoByteStringCharsAndLength(cx, nogc, value_str, &length);
-    for (auto i = 0; i < length; i++) {
-      if (chars[i] > 255) {
-        JS_ReportErrorASCII(cx, "header value contains bytes greater than 255");
-        return nullptr;
+  if (!JS::StringHasLatin1Chars(value_str)) {
+    bool has_err = false;
+    // First ensure string is linear and not a rope or atom.
+    JSLinearString *lstr = JS_EnsureLinearString(cx, value_str);
+    if (!lstr) {
+      return nullptr;
+    }
+    {
+      JS::AutoCheckCannotGC nogc;
+      size_t length;
+      const char16_t *chars = JS_GetTwoByteStringCharsAndLength(cx, nogc, value_str, &length);
+      MOZ_ASSERT(chars);
+      for (auto i = 0; i < length; i++) {
+        if (chars[i] > 255) {
+          has_err = true;
+          break;
+        }
       }
     }
+    // Error must be reported outside of GC guard
+    if (has_err) {
+      JS_ReportErrorASCII(cx, "header value contains bytes greater than 255");
+      return nullptr;
+    }
   }
 
   host_api::HostString value;
@@ -576,8 +588,8 @@ JSObject *Headers::create(JSContext *cx, JS::HandleObject self, Headers::Mode mo
   }
 
   bool consumed = false;
-  if (!core::maybe_consume_sequence_or_record<Headers::append_header_value>(cx, initv, headers,
-                                                                            &consumed, "Headers")) {
+  if (!common::maybe_consume_sequence_or_record<Headers::append_header_value>(
+          cx, initv, headers, &consumed, "Headers")) {
     return nullptr;
   }
 

runtime/fastly/builtins/fetch/headers.cpp

@@ -386,23 +386,28 @@ bool RequestOrResponse::extract_body(JSContext *cx, JS::HandleObject self,
     char *buf;
     size_t length;
 
+    host_api::Result<host_api::Void> write_res;
+
+    host_api::HttpBody body{RequestOrResponse::body_handle(self)};
     if (body_obj && JS_IsArrayBufferViewObject(body_obj)) {
       // Short typed arrays have inline data which can move on GC, so assert
       // that no GC happens. (Which it doesn't, because we're not allocating
       // before `buf` goes out of scope.)
-      maybeNoGC.emplace(cx);
-      JS::AutoCheckCannotGC &noGC = maybeNoGC.ref();
+      JS::AutoCheckCannotGC noGC(cx);
       bool is_shared;
       length = JS_GetArrayBufferViewByteLength(body_obj);
       buf = (char *)JS_GetArrayBufferViewData(body_obj, &is_shared, noGC);
+      write_res = body.write_all_back(reinterpret_cast<uint8_t *>(buf), length);
     } else if (body_obj && JS::IsArrayBufferObject(body_obj)) {
       bool is_shared;
       JS::GetArrayBufferLengthAndData(body_obj, &length, &is_shared, (uint8_t **)&buf);
+      write_res = body.write_all_back(reinterpret_cast<uint8_t *>(buf), length);
     } else if (body_obj && URLSearchParams::is_instance(body_obj)) {
       auto slice = URLSearchParams::serialize(cx, body_obj);
       buf = (char *)slice.data;
       length = slice.len;
       content_type = "application/x-www-form-urlencoded;charset=UTF-8";
+      write_res = body.write_all_back(reinterpret_cast<uint8_t *>(buf), length);
     } else {
       {
         auto str = core::encode(cx, body_val);
@@ -415,15 +420,7 @@ bool RequestOrResponse::extract_body(JSContext *cx, JS::HandleObject self,
       }
       buf = text.get();
       content_type = "text/plain;charset=UTF-8";
-    }
-
-    host_api::HttpBody body{RequestOrResponse::body_handle(self)};
-    auto write_res = body.write_all_back(reinterpret_cast<uint8_t *>(buf), length);
-
-    // Ensure that the NoGC is reset, so throwing an error in HANDLE_ERROR
-    // succeeds.
-    if (maybeNoGC.isSome()) {
-      maybeNoGC.reset();
+      write_res = body.write_all_back(reinterpret_cast<uint8_t *>(buf), length);
     }
 
     if (auto *err = write_res.to_err()) {

runtime/fastly/builtins/fetch/request-response.cpp

@@ -0,0 +1,124 @@
+#ifndef FASTLY_SEQUENCE_HPP
+#define FASTLY_SEQUENCE_HPP
+
+// TODO: remove these once the warnings are fixed
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winvalid-offsetof"
+#include "jsapi.h"
+#include "jsfriendapi.h"
+#include "js/ForOfIterator.h"
+#pragma clang diagnostic pop
+
+namespace fastly::common {
+
+inline bool report_sequence_or_record_arg_error(JSContext *cx, const char *name,
+                                                const char *alt_text) {
+  JS_ReportErrorUTF8(cx,
+                     "Failed to construct %s object. If defined, the first "
+                     "argument must be either a [ ['name', 'value'], ... ] sequence, "
+                     "or a { 'name' : 'value', ... } record%s.",
+                     name, alt_text);
+  return false;
+}
+
+/**
+ * Extract <key,value> pairs from the given value if it is either a
+ * sequence<sequence<Value> or a record<Value, Value>.
+ */
+template <auto apply>
+bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::HandleObject target,
+                                      bool *consumed, const char *ctor_name,
+                                      const char *alt_text = "") {
+  if (initv.isUndefined()) {
+    *consumed = true;
+    return true;
+  }
+
+  JS::RootedValue key(cx);
+  JS::RootedValue value(cx);
+
+  // First, try consuming args[0] as a sequence<sequence<Value>>.
+  JS::ForOfIterator it(cx);
+  if (!it.init(initv, JS::ForOfIterator::AllowNonIterable))
+    return false;
+
+  // Note: this currently doesn't treat strings as iterable even though they
+  // are. We don't have any constructors that want to iterate over strings, and
+  // this makes things a lot easier.
+  if (initv.isObject() && it.valueIsIterable()) {
+    JS::RootedValue entry(cx);
+
+    while (true) {
+      bool done;
+      if (!it.next(&entry, &done))
+        return false;
+
+      if (done)
+        break;
+
+      if (!entry.isObject())
+        return report_sequence_or_record_arg_error(cx, ctor_name, alt_text);
+
+      JS::ForOfIterator entr_iter(cx);
+      if (!entr_iter.init(entry, JS::ForOfIterator::AllowNonIterable))
+        return false;
+
+      if (!entr_iter.valueIsIterable())
+        return report_sequence_or_record_arg_error(cx, ctor_name, alt_text);
+
+      {
+        bool done;
+
+        // Extract key.
+        if (!entr_iter.next(&key, &done))
+          return false;
+        if (done)
+          return report_sequence_or_record_arg_error(cx, ctor_name, alt_text);
+
+        // Extract value.
+        if (!entr_iter.next(&value, &done))
+          return false;
+        if (done)
+          return report_sequence_or_record_arg_error(cx, ctor_name, alt_text);
+
+        // Ensure that there aren't any further entries.
+        if (!entr_iter.next(&entry, &done))
+          return false;
+        if (!done)
+          return report_sequence_or_record_arg_error(cx, ctor_name, alt_text);
+
+        if (!apply(cx, target, key, value, ctor_name))
+          return false;
+      }
+    }
+    *consumed = true;
+  } else if (initv.isObject()) {
+    // init isn't an iterator, so if it's an object, it must be a record to be
+    // valid input.
+    JS::RootedObject init(cx, &initv.toObject());
+    JS::RootedIdVector ids(cx);
+    if (!js::GetPropertyKeys(cx, init, JSITER_OWNONLY | JSITER_SYMBOLS, &ids))
+      return false;
+
+    JS::RootedId curId(cx);
+    for (size_t i = 0; i < ids.length(); ++i) {
+      curId = ids[i];
+      key = js::IdToValue(curId);
+
+      if (!JS_GetPropertyById(cx, init, curId, &value))
+        return false;
+
+      if (!apply(cx, target, key, value, ctor_name))
+        return false;
+    }
+    *consumed = true;
+  } else {
+    *consumed = false;
+  }
+
+  return true;
+}
+
+}
+
+#endif

runtime/fastly/common/sequence.hpp

@@ -1,9 +1,9 @@
 {
   "Create headers giving an array having one string as init argument": {
-    "status": "PASS"
+    "status": "FAIL"
   },
   "Create headers giving an array having three strings as init argument": {
-    "status": "PASS"
+    "status": "FAIL"
   },
   "Create headers giving bad header name as init argument": {
     "status": "FAIL"