1073231 - Implement valid Request and Response clone() method

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Waiting for it to be specced.

Comment 1

[Arkadiusz Michalski (Spirit)]

Request.clone() and Response.clone() should throw a TypeError when used flag is set, but now Firefox doesn't check this.
https://fetch.spec.whatwg.org/#dom-request-clone
https://fetch.spec.whatwg.org/#dom-response-clone
https://fetch.spec.whatwg.org/#concept-body-used-flag

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

To implement this well we need bug 1100398.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Implement Request and Response Clone() methods. (v0)

Implement Request/Response clone().  This depends on the patches from bug 1100398 that are currently in review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4269619404a9

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Patch queue is here if you want to try it locally:

  https://github.com/wanderview/gecko-patches/tree/dev-stream-clone

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

The try in comment 3 showed an issue with the nsStringInputStream's clone.  Here is an updated try with that fixed:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f32d96bd7908

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8557194 [details] [diff] [review]
Implement Request and Response Clone() methods. (v0)

Review of attachment 8557194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalRequest.h
@@ +75,5 @@
>      , mUseURLCredentials(false)
>    {
>    }
>  
> +  explicit InternalRequest(InternalRequest& aOther);

Nit: Since this is public, please add a comment that this clones the body and is an expensive operation. Is stream cloning main thread only? In that case, add such a note too.

::: dom/fetch/InternalResponse.cpp
@@ +39,5 @@
> +  nsCOMPtr<nsIInputStream> replacement;
> +
> +  nsresult rv = NS_CloneInputStream(aOther.mBody, getter_AddRefs(clone),
> +                                    getter_AddRefs(replacement));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return; }

Under what conditions can this fail when you do pass a replacement? If it is only OOM, this can just be changed to assert NS_SUCCEEDED(). If there are other reasons, is it ok to return a response that doesn't have a body? The spec doesn't say anything, but it seems it would be better to fail the Clone() (add an InitSuccess() method or something that is checked in Clone().

Same in InternalRequest.

::: dom/fetch/Response.cpp
@@ +201,1 @@
>    nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mOwner);

mOwner is already an nsIGlobalObject. It wasn't when I wrote this stub. Could you fix this too? Thanks!

::: dom/workers/test/fetch/worker_test_request.js
@@ +31,5 @@
>                body: "Sample body",
>                mode: "same-origin",
>                credentials: "same-origin",
> +            });
> +  var clone = orig.clone();

I can't find anything in the spec that says this is not allowed:

  var clone1 = orig.clone()
  var clone2 = orig.clone()
  var clone3 = clone2.clone()

If this is indeed allowed until none of the bodies have been consumed, please add a test that tests for multiple clones too.

@@ +57,5 @@
> +    ok(clone.bodyUsed, "Clone body is consumed.");
> +
> +    try {
> +      orig.clone()
> +      ok(false, "Original should not be able to be cloned twice.");

The spec only says anything about bodyUsed preventing cloning. Could you change the text of the test to "Cannot clone Request whose body is already consumed"?

@@ +59,5 @@
> +    try {
> +      orig.clone()
> +      ok(false, "Original should not be able to be cloned twice.");
> +    } catch (e) {
> +      is(e.name, "TypeError", "Second clone() should throw TypeError");

Same here.

::: dom/workers/test/fetch/worker_test_response.js
@@ +44,5 @@
> +    ok(clone.bodyUsed, "Clone body is consumed.");
> +
> +    try {
> +      orig.clone()
> +      ok(false, "Original should not be able to be cloned twice.");

Same as in Request tests, and also test for multiple clones.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8557194 [details] [diff] [review]
Implement Request and Response Clone() methods. (v0)

Review of attachment 8557194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalRequest.cpp
@@ +73,5 @@
> +  nsCOMPtr<nsIInputStream> replacement;
> +
> +  nsresult rv = NS_CloneInputStream(aOther.mBodyStream, getter_AddRefs(clone),
> +                                    getter_AddRefs(replacement));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return; }

Probably it's better to have a ::Clone() method that can return this error.
Otherwise we have an object without a mBodyStream.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

One other thing I found in testing was that this patch does not currently copy the Request InternalHeaders object.  I need to fix this and write a test to demonstrate that the clone has a separate headers object.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I want to see bug 1100398 stick in the tree for a few days before moving forward on this one.  It seems like a decent possibility of getting backed out since the clone changes were pretty invasive to nsPipe.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Implement Request and Response Clone() methods. (v1)

Addressed action items.  Nikhil, can you look this over one more time since what I did effects the filtering methods?  Thanks!

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8566215 [details] [diff] [review]
Implement Request and Response Clone() methods. (v1)

Review of attachment 8566215 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at the assertions in the filters.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d50c2b27871

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Implement Request and Response Clone() methods. (v2)

The try build indicates that the filtered responses can have their body set.  Modify the code to simply steal the bodies.  Restore the comment indicating that this is the behavior.

Waiting on tree closure to run another try build.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1478a6abab

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/39c5fd86a4a0

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I missed a .rej file in the mozilla-inbound rebase.  Had to do a follow-up to fix:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/a71a4b10000a

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39c5fd86a4a0
https://hg.mozilla.org/mozilla-central/rev/a71a4b10000a