Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions builtins/web/fetch/request-response.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2291,6 +2291,91 @@ bool Response::redirect(JSContext *cx, unsigned argc, Value *vp) {
// return true;
// }

/// https://fetch.spec.whatwg.org/#dom-response-clone
bool Response::clone(JSContext *cx, unsigned argc, JS::Value *vp) {
// If this is unusable, then throw a TypeError.
METHOD_HEADER(0);

// To clone a response response, run these steps:
// 1. If response is a filtered response, then return a new identical filtered response whose internal response is a clone of response’s internal response.
RootedObject new_response(cx, create(cx));
if (!new_response) {
return false;
}

init_slots(new_response);

// 2. Let newResponse be a copy of response, except for its body.
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, Headers::guard(headers));
if (!cloned_headers) {
return false;
}
cloned_headers_val.set(ObjectValue(*cloned_headers));
} else if (RequestOrResponse::maybe_handle(self)) {
auto handle = RequestOrResponse::headers_handle_clone(cx, self);
JSObject *cloned_headers =
Headers::create(cx, handle.release(),
RequestOrResponse::is_incoming(self) ? Headers::HeadersGuard::Immutable
: Headers::HeadersGuard::Response);
if (!cloned_headers) {
return false;
}
cloned_headers_val.set(ObjectValue(*cloned_headers));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to use RequestOrResponse::headers_handle_clone for this, reducing complexity a bunch. If that's not quite correct to use here, perhaps it can instead be tweaked?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to work out a simple way to use RequestOrResponse::headers_handle_clone for this, however the solution I've added in a recent commit that constructs a RootedObject based on RequestOrResponse::headers appears to do the trick. I'd love an extra set of eyes on that approach though as I'm still wrapping my head around internal object apis.

RootedObject cloned_headers(cx, RequestOrResponse::headers(cx, self));
if (!cloned_headers) {
  return false;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this isn't quite right: it will always return an instance of builtins::web::fetch::Headers tied to self, not a clone of that each time it's called. You should be able to see this if you create a response, clone it using Response.clone, and then do something like response.headers.set("foo", "bar"): this should now show up on both responses.

Can you say what didn't work when using headers_handle_clone?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it another look over and was able to get headers_handle_clone to work for this. I'll chalk it up to my own ignorance regarding the codebase. I've mirrored the changes to Request::clone as well and wrote up some changes to the tests to check that the headers are properly cloned rather than shared.

Once any further changes are worked through, do you prefer keeping the individual commits in this PR or would a force-push of squashed commits be preferred?


SetReservedSlot(new_response, static_cast<uint32_t>(Slots::Headers), cloned_headers_val);

Value status_val = GetReservedSlot(self, static_cast<uint32_t>(Slots::Status));
Value status_message_val = GetReservedSlot(self, static_cast<uint32_t>(Slots::StatusMessage));

// ENGINE->dump_value(status_val, stderr);
// ENGINE->dump_value(status_message_val, stderr);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this?

SetReservedSlot(new_response, static_cast<uint32_t>(Slots::Status), status_val);
SetReservedSlot(new_response, static_cast<uint32_t>(Slots::StatusMessage), status_message_val);

// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.
// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.

RootedObject new_body(cx);
auto has_body = RequestOrResponse::has_body(self);
if (!has_body) {
args.rval().setObject(*new_response);
return true;
}

// Here we get the current response'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 response'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;
}
}

if (RequestOrResponse::body_unusable(cx, body_stream)) {
return api::throw_error(cx, FetchErrors::BodyStreamUnusable);
}

RootedObject self_body(cx);
if (!ReadableStreamTee(cx, body_stream, &self_body, &new_body)) {
return false;
}

SetReservedSlot(self, static_cast<uint32_t>(Slots::BodyStream), ObjectValue(*self_body));
SetReservedSlot(new_response, static_cast<uint32_t>(Slots::BodyStream), ObjectValue(*new_body));
SetReservedSlot(new_response, static_cast<uint32_t>(Slots::HasBody), JS::BooleanValue(true));

// 4. Return newResponse.
args.rval().setObject(*new_response);
return true;
}

const JSFunctionSpec Response::static_methods[] = {
JS_FN("redirect", redirect, 1, JSPROP_ENUMERATE),
// JS_FN("json", json, 1, JSPROP_ENUMERATE),
Expand All @@ -2306,6 +2391,7 @@ const JSFunctionSpec Response::methods[] = {
JSPROP_ENUMERATE),
JS_FN("json", bodyAll<RequestOrResponse::BodyReadResult::JSON>, 0, JSPROP_ENUMERATE),
JS_FN("text", bodyAll<RequestOrResponse::BodyReadResult::Text>, 0, JSPROP_ENUMERATE),
JS_FN("clone", clone, 0, JSPROP_ENUMERATE),
JS_FS_END,
};

Expand Down
2 changes: 2 additions & 0 deletions builtins/web/fetch/request-response.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ class Response final : public BuiltinImpl<Response> {
static bool redirect(JSContext *cx, unsigned argc, JS::Value *vp);
static bool json(JSContext *cx, unsigned argc, JS::Value *vp);

static bool clone(JSContext *cx, unsigned argc, JS::Value *vp);

public:
static constexpr const char *class_name = "Response";

Expand Down
44 changes: 44 additions & 0 deletions tests/integration/fetch/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,48 @@ export const handler = serveTest(async (t) => {
await request.text();
throws(() => request.clone());
});

t.test('response-clone-bad-calls', () => {
throws(() => new Response.prototype.clone(), TypeError);
throws(() => new Response.prototype.clone.call(undefined), TypeError);
});

await t.test('response-clone-valid', async () => {
{
const response = new Response('test body', {
headers: {
hello: 'world'
},
status: 200,
statusText: 'Success'
});
const newResponse = response.clone();
strictEqual(newResponse instanceof Response, true, 'newResponse instanceof Request');
strictEqual(response.bodyUsed, false, 'response.bodyUsed');
strictEqual(newResponse.bodyUsed, false, 'newResponse.bodyUsed');
deepStrictEqual([...newResponse.headers], [...response.headers], 'newResponse.headers');
strictEqual(newResponse.status, 200, 'newResponse.status');
strictEqual(newResponse.statusText, 'Success', 'newResponse.statusText');
strictEqual(newResponse.body instanceof ReadableStream, true, 'newResponse.body instanceof ReadableStream');
}

{
const response = new Response(null, {
status: 404,
statusText: "Not found",
});
const newResponse = response.clone();
strictEqual(newResponse.bodyUsed, false, 'newResponse.bodyUsed');
strictEqual(newResponse.body, null, 'newResponse.body');
}
});

await t.test('response-clone-invalid', async () => {
const response = new Response('test body', {
status: 200,
statusText: "Success"
});
await response.text();
throws(() => response.clone());
});
});