Implement Request.clone (#92)

* Cleanup: make `Request::method` infallible

It already is infallible internally, so might as well reflect that in the signature and clean up the callsites.

* Bugfix: always reify input headers in the `Request` initializer

I stumbled upon this while implementing `Request.clone`, and while this change doesn't cause any additional WPT tests to pass, I'm certain that it's required: otherwise, `new Request(incomingRequest)` will not apply `incomingRequest`'s header entries to the new request if `.headers` has never been read on `incomingRequest`.

* Implement `Request.clone`

Fixes #84

* add simple clone test

* fix headers clone

---------

Co-authored-by: Guy Bedford <gbedford@fastly.com>

Author: tschneidereit

builtins/web/fetch/fetch-api.cpp

@@ -35,11 +35,7 @@ bool fetch(JSContext *cx, unsigned argc, Value *vp) {
     return ReturnPromiseRejectedWithPendingError(cx, args);
   }
 
-  RootedString method_str(cx, Request::method(cx, request_obj));
-  if (!method_str) {
-    return ReturnPromiseRejectedWithPendingError(cx, args);
-  }
-
+  RootedString method_str(cx, Request::method(request_obj));
   host_api::HostString method = core::encode(cx, method_str);
   if (!method.ptr) {
     return ReturnPromiseRejectedWithPendingError(cx, args);

builtins/web/fetch/fetch-errors.h

@@ -12,6 +12,7 @@ DEF_ERR(InvalidInitArg, JSEXN_TYPEERR, "{0}: |init| parameter can't be converted
 DEF_ERR(NonBodyRequestWithBody, JSEXN_TYPEERR, "Request constructor: HEAD or GET Request cannot have a body", 0)
 DEF_ERR(NonBodyResponseWithBody, JSEXN_TYPEERR, "Response constructor: response with status {0} cannot have a body", 1)
 DEF_ERR(BodyStreamUnusable, JSEXN_TYPEERR, "Can't use a ReadableStream that's locked or has ever been read from or canceled", 0)
+DEF_ERR(BodyStreamTeeingFailed, JSEXN_ERR, "Cloning body stream failed", 0)
 DEF_ERR(InvalidStatus, JSEXN_RANGEERR, "{0}: invalid status {1}", 2)
 DEF_ERR(InvalidStreamChunk, JSEXN_TYPEERR, "ReadableStream used as a Request or Response body must produce Uint8Array values", 0)
 DEF_ERR(EmptyHeaderName, JSEXN_TYPEERR, "{0}: Header name can't be empty", 1)

builtins/web/fetch/request-response.cpp

@@ -1074,8 +1074,8 @@ bool RequestOrResponse::maybe_stream_body(JSContext *cx, JS::HandleObject body_o
 }
 
 JSObject *RequestOrResponse::create_body_stream(JSContext *cx, JS::HandleObject owner) {
-  MOZ_ASSERT(is_instance(owner));
   MOZ_ASSERT(!body_stream(owner));
+  MOZ_ASSERT(has_body(owner));
   JS::RootedObject source(cx, streams::NativeStreamSource::create(
                                   cx, owner, JS::UndefinedHandleValue, body_source_pull_algorithm,
                                   body_source_cancel_algorithm));
@@ -1144,19 +1144,15 @@ JSObject *Request::response_promise(JSObject *obj) {
               .toObject();
 }
 
-JSString *Request::method(JSContext *cx, JS::HandleObject obj) {
+JSString *Request::method(HandleObject obj) {
   MOZ_ASSERT(is_instance(obj));
   return JS::GetReservedSlot(obj, static_cast<uint32_t>(Slots::Method)).toString();
 }
 
 bool Request::method_get(JSContext *cx, unsigned argc, JS::Value *vp) {
   METHOD_HEADER(0)
 
-  JSString *method = Request::method(cx, self);
-  if (!method)
-    return false;
-
-  args.rval().setString(method);
+  args.rval().setString(Request::method(self));
   return true;
 }
 
@@ -1197,128 +1193,86 @@ bool Request::bodyUsed_get(JSContext *cx, unsigned argc, JS::Value *vp) {
 
 JSString *GET_atom;
 
-// bool Request::clone(JSContext *cx, unsigned argc, JS::Value *vp) {
-//   METHOD_HEADER(0);
-
-//   auto hasBody = RequestOrResponse::has_body(self);
-
-//   if (hasBody) {
-//     if (RequestOrResponse::body_unusable(self)) {
-//       return api::throw_error(cx, FetchErrors::BodyStreamUnusable);
-//     }
-
-//     // Here we get the current request's body stream and call ReadableStream.prototype.tee to
-//     return
-//     // two versions of the stream. Once we get the two streams, we create a new request handle
-//     and
-//     // attach one of the streams to the new handle and the other stream is attached to the
-//     request
-//     // handle that `clone()` was called upon.
-//     JS::RootedObject body_stream(cx, RequestOrResponse::body_stream(self));
-//     if (!body_stream) {
-//       body_stream = RequestOrResponse::create_body_stream(cx, self);
-//       if (!body_stream) {
-//         return false;
-//       }
-//     }
-//     JS::RootedValue tee_val(cx);
-//     if (!JS_GetProperty(cx, body_stream, "tee", &tee_val)) {
-//       return false;
-//     }
-//     JS::Rooted<JSFunction *> tee(cx, JS_GetObjectFunction(&tee_val.toObject()));
-//     if (!tee) {
-//       return false;
-//     }
-//     JS::RootedVector<JS::Value> argv(cx);
-//     JS::RootedValue rval(cx);
-//     if (!JS::Call(cx, body_stream, tee, argv, &rval)) {
-//       return false;
-//     }
-//     JS::RootedObject rval_array(cx, &rval.toObject());
-//     JS::RootedValue body1_val(cx);
-//     if (!JS_GetProperty(cx, rval_array, "0", &body1_val)) {
-//       return false;
-//     }
-//     JS::RootedValue body2_val(cx);
-//     if (!JS_GetProperty(cx, rval_array, "1", &body2_val)) {
-//       return false;
-//     }
-
-//     auto res = host_api::HttpBody::make(request_handle);
-//     if (auto *err = res.to_err()) {
-//       HANDLE_ERROR(cx, *err);
-//       return false;
-//     }
-
-//     auto body_handle = res.unwrap();
-//     if (!JS::IsReadableStream(&body1_val.toObject())) {
-//       return false;
-//     }
-//     body_stream.set(&body1_val.toObject());
-//     if (RequestOrResponse::body_unusable(cx, body_stream)) {
-//       return api::throw_error(cx, FetchErrors::BodyStreamUnusable);
-//     }
-
-//     JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::Body),
-//                         JS::Int32Value(body_handle.handle));
-//     JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::BodyStream), body1_val);
-
-//     JS::SetReservedSlot(self, static_cast<uint32_t>(Slots::BodyStream), body2_val);
-//     JS::SetReservedSlot(self, static_cast<uint32_t>(Slots::BodyUsed), JS::FalseValue());
-//     JS::SetReservedSlot(self, static_cast<uint32_t>(Slots::HasBody), JS::BooleanValue(true));
-//   }
-
-//   JS::RootedObject headers(cx);
-//   JS::RootedObject headers_obj(
-//       cx, RequestOrResponse::headers(cx, self));
-//   if (!headers_obj) {
-//     return false;
-//   }
-//   JS::RootedObject headersInstance(
-//       cx, JS_NewObjectWithGivenProto(cx, &Headers::class_, Headers::proto_obj));
-//   if (!headersInstance)
-//     return false;
-
-//   headers = Headers::create(cx, headersInstance, Headers::Mode::ProxyToRequest,
-//                                       requestInstance, headers_obj);
-
-//   if (!headers) {
-//     return false;
-//   }
-
-//   JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::Headers),
-//                       JS::ObjectValue(*headers));
+/// https://fetch.spec.whatwg.org/#dom-request-clone
+bool Request::clone(JSContext *cx, unsigned argc, JS::Value *vp) {
+  METHOD_HEADER(0);
+
+  // clone operation step 1.
+  // Let newRequest be a copy of request, except for its body.
+  // Note that the spec doesn't say what it means to copy a request, exactly.
+  // Since a request only has the fields "method", "url", and "headers", and the "Body" mixin,
+  // we copy those three fields in this step.
+  RootedObject new_request(cx, create(cx));
+  if (!new_request) {
+    return false;
+  }
+  init_slots(new_request);
 
-//   JSString *method = Request::method(cx, self);
-//   if (!method) {
-//     return false;
-//   }
+  RootedValue cloned_headers_val(cx, JS::NullValue());
+  RootedObject headers(cx, RequestOrResponse::maybe_headers(self));
+  if (headers) {
+    RootedValue headers_val(cx, ObjectValue(*headers));
+    JSObject *cloned_headers =
+        Headers::create(cx, headers_val, host_api::HttpHeadersGuard::Request);
+    if (!cloned_headers) {
+      return false;
+    }
+    cloned_headers_val.set(ObjectValue(*cloned_headers));
+  } else if (RequestOrResponse::handle(self)) {
+    auto handle =
+        RequestOrResponse::headers_handle_clone(cx, self, host_api::HttpHeadersGuard::Request);
+    JSObject *cloned_headers =
+        Headers::create(cx, handle.release(), host_api::HttpHeadersGuard::Request);
+    if (!cloned_headers) {
+      return false;
+    }
+    cloned_headers_val.set(ObjectValue(*cloned_headers));
+  }
+
+  SetReservedSlot(new_request, static_cast<uint32_t>(Slots::Headers), cloned_headers_val);
+  Value url_val = GetReservedSlot(self, static_cast<uint32_t>(Slots::URL));
+  SetReservedSlot(new_request, static_cast<uint32_t>(Slots::URL), url_val);
+  Value method_val = JS::StringValue(method(self));
+  ENGINE->dump_value(method_val, stderr);
+  SetReservedSlot(new_request, static_cast<uint32_t>(Slots::Method), method_val);
+
+  // clone operation step 2.
+  // If request’s body is non-null, set newRequest’s body to the result of cloning request’s body.
+  RootedObject new_body(cx);
+  auto has_body = RequestOrResponse::has_body(self);
+  if (!has_body) {
+    args.rval().setObject(*new_request);
+    return true;
+  }
 
-//   JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::Method),
-//                       JS::StringValue(method));
+  // Here we get the current request's body stream and call ReadableStream.prototype.tee to
+  // get two streams for the same content.
+  // One of these is then used to replace the current request's body, the other is used as
+  // the body of the clone.
+  JS::RootedObject body_stream(cx, RequestOrResponse::body_stream(self));
+  if (!body_stream) {
+    body_stream = RequestOrResponse::create_body_stream(cx, self);
+    if (!body_stream) {
+      return false;
+    }
+  }
 
-//   auto *request_handle_res = new host_api::HttpOutgoingRequest()
-//   if (auto *err = request_handle_res.to_err()) {
-//     HANDLE_ERROR(cx, *err);
-//     return false;
-//   }
+  if (RequestOrResponse::body_unusable(cx, body_stream)) {
+    return api::throw_error(cx, FetchErrors::BodyStreamUnusable);
+  }
 
-//   auto request_handle = request_handle_res.unwrap();
+  RootedObject self_body(cx);
+  if (!ReadableStreamTee(cx, body_stream, &self_body, &new_body)) {
+    return false;
+  }
 
-//   JS::RootedObject requestInstance(cx, Request::create_instance(cx));
-//   JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::Request),
-//                       JS::Int32Value(request_handle.handle));
-//   JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::BodyUsed),
-//                       JS::FalseValue());
-//   JS::SetReservedSlot(
-//       requestInstance, static_cast<uint32_t>(Slots::URL),
-//       JS::GetReservedSlot(self, static_cast<uint32_t>(Slots::URL)));
-//   JS::SetReservedSlot(requestInstance, static_cast<uint32_t>(Slots::HasBody),
-//                       JS::BooleanValue(hasBody));
+  SetReservedSlot(self, static_cast<uint32_t>(Slots::BodyStream), ObjectValue(*self_body));
+  SetReservedSlot(new_request, static_cast<uint32_t>(Slots::BodyStream), ObjectValue(*new_body));
+  SetReservedSlot(new_request, static_cast<uint32_t>(Slots::HasBody), JS::BooleanValue(true));
 
-//   args.rval().setObject(*requestInstance);
-//   return true;
-// }
+  args.rval().setObject(*new_request);
+  return true;
+}
 
 const JSFunctionSpec Request::static_methods[] = {
     JS_FS_END,
@@ -1333,7 +1287,7 @@ const JSFunctionSpec Request::methods[] = {
           JSPROP_ENUMERATE),
     JS_FN("json", Request::bodyAll<RequestOrResponse::BodyReadResult::JSON>, 0, JSPROP_ENUMERATE),
     JS_FN("text", Request::bodyAll<RequestOrResponse::BodyReadResult::Text>, 0, JSPROP_ENUMERATE),
-    // JS_FN("clone", Request::clone, 0, JSPROP_ENUMERATE),
+    JS_FN("clone", Request::clone, 0, JSPROP_ENUMERATE),
     JS_FS_END,
 };
 
@@ -1372,7 +1326,7 @@ void Request::init_slots(JSObject *requestInstance) {
  * Create a new Request object, roughly according to
  * https://fetch.spec.whatwg.org/#dom-request
  *
- * "Roughly" because not all aspects of Request handling make sense in C@E.
+ * "Roughly" because not all aspects of Request handling make sense in StarlingMonkey.
  * The places where we deviate from the spec are called out inline.
  */
 bool Request::initialize(JSContext *cx, JS::HandleObject request, JS::HandleValue input,
@@ -1416,10 +1370,7 @@ bool Request::initialize(JSContext *cx, JS::HandleObject request, JS::HandleValu
     url_str = RequestOrResponse::url(input_request).toString();
 
     // method: `request`’s method.
-    method_str = Request::method(cx, input_request);
-    if (!method_str) {
-      return false;
-    }
+    method_str = Request::method(input_request);
 
     // referrer: `request`’s referrer.
     // TODO: evaluate whether we want to implement support for setting the
@@ -1430,7 +1381,14 @@ bool Request::initialize(JSContext *cx, JS::HandleObject request, JS::HandleValu
 
     // header list: A copy of `request`’s header list.
     // Note: copying the headers is postponed, see step 32 below.
-    input_headers = RequestOrResponse::maybe_headers(input_request);
+    // Note: we're forcing reification of the input request's headers here. That is suboptimal,
+    // because we might end up not using them. Additionally, if the headers are represented
+    // internally as a handle (e.g. because the input is an incoming request), we would in
+    // principle not need to ever reify it just to get a clone.
+    // Applying these optimizations is somewhat complex though, so for now we're not doing so.
+    if (!(input_headers = RequestOrResponse::headers(cx, input_request))) {
+      return false;
+    }
 
     // The following properties aren't applicable:
     // unsafe-request flag: Set.

builtins/web/fetch/request-response.h

@@ -142,7 +142,7 @@ class Request final : public BuiltinImpl<Request> {
   };
 
   static JSObject *response_promise(JSObject *obj);
-  static JSString *method(JSContext *cx, JS::HandleObject obj);
+  static JSString *method(JS::HandleObject obj);
   static host_api::HttpRequest *request_handle(JSObject *obj);
   static host_api::HttpOutgoingRequest *outgoing_handle(JSObject *obj);
   static host_api::HttpIncomingRequest *incoming_handle(JSObject *obj);

tests/integration/fetch/fetch.js

@@ -0,0 +1,56 @@
+import { serveTest } from '../test-server.js';
+import { strictEqual, deepStrictEqual, throws } from '../assert.js';
+
+export const handler = serveTest(async (t) => {
+  await t.test('headers-non-ascii-latin1-field-value', async () => {
+    const response = await fetch("https://http-me.glitch.me/meow?header=cat:é");
+    strictEqual(response.headers.get('cat'), "é");
+  });
+
+  t.test('request-clone-bad-calls', () => {
+    throws(() => new Request.prototype.clone(), TypeError);
+    throws(() => new Request.prototype.clone.call(undefined), TypeError);
+  });
+
+  await t.test('request-clone-valid', async () => {
+    {
+      const request = new Request('https://www.fastly.com', {
+        headers: {
+            hello: 'world'
+        },
+        body: 'te',
+        method: 'post'
+      });
+      const newRequest = request.clone();
+      strictEqual(newRequest instanceof Request, true, 'newRequest instanceof Request');
+      strictEqual(newRequest.method, request.method, 'newRequest.method');
+      strictEqual(newRequest.url, request.url, 'newRequest.url');
+      deepStrictEqual([...newRequest.headers], [...request.headers], 'newRequest.headers');
+      strictEqual(request.bodyUsed, false, 'request.bodyUsed');
+      strictEqual(newRequest.bodyUsed, false, 'newRequest.bodyUsed');
+      strictEqual(newRequest.body instanceof ReadableStream, true, 'newRequest.body instanceof ReadableStream');
+    }
+
+    {
+      const request = new Request('https://www.fastly.com', {
+        method: 'get'
+      })
+      const newRequest = request.clone();
+
+      strictEqual(newRequest.bodyUsed, false, 'newRequest.bodyUsed');
+      strictEqual(newRequest.body, null, 'newRequest.body');
+    }
+  });
+
+  await t.test('request-clone-invalid', async () => {
+    const request = new Request('https://www.fastly.com', {
+      headers: {
+          hello: 'world'
+      },
+      body: 'te',
+      method: 'post'
+    });
+    await request.text();
+    throws(() => request.clone());
+  });
+});

tests/integration/handlers.js

@@ -2,4 +2,4 @@ export { handler as btoa } from './btoa/btoa.js';
 export { handler as performance } from './performance/performance.js';
 export { handler as crypto } from './crypto/crypto.js';
 export { handler as timers } from './timers/timers.js';
-export { handler as headers } from './headers/headers.js';
+export { handler as fetch } from './fetch/fetch.js';

tests/integration/headers/headers.js

@@ -38,5 +38,5 @@ test_e2e(tla-runtime-resolve)
 test_integration(btoa)
 test_integration(performance)
 test_integration(crypto)
-test_integration(headers)
+test_integration(fetch)
 test_integration(timers)