Implement valid Request and Response clone() method

RESOLVED FIXED in Firefox 38

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nsm, Assigned: bkelly)

Tracking

({dev-doc-needed})

unspecified
mozilla38
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

Waiting for it to be specced.
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
(Assignee)

Updated

3 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
To implement this well we need bug 1100398.
Depends on: 1100398
(Assignee)

Comment 3

3 years ago
Created attachment 8557194 [details] [diff] [review]
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
Attachment #8557194 - Flags: review?(nsm.nikhil)
Attachment #8557194 - Flags: review?(amarchesini)
(Assignee)

Comment 4

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

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

Comment 5

3 years ago
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 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.
Attachment #8557194 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8557194 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
Created attachment 8566215 [details] [diff] [review]
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!
Attachment #8557194 - Attachment is obsolete: true
Attachment #8566215 - Flags: review?(nsm.nikhil)
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.
Attachment #8566215 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

3 years ago
Created attachment 8566712 [details] [diff] [review]
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.
Attachment #8566215 - Attachment is obsolete: true
Attachment #8566712 - Flags: review+
(Assignee)

Comment 16

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/39c5fd86a4a0
https://hg.mozilla.org/mozilla-central/rev/a71a4b10000a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.