1134609 - make Fetch Request constructor less destructive to input on error

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

The Fetch spec has been updated to make the Request constructor less destructive to the input when an error is hit:

  https://github.com/slightlyoff/ServiceWorker/issues/629

Comment 1

[Josh Matthews [:jdm]]

Relevant code is the Request constructor in dom/fetch/Request.cpp.

Comment 2

[Kapil Bakshi[geekspark]]

I would like to work on this . Please assign it to me .

Comment 3

[Kapil Bakshi[geekspark]]

Could you please elaborate more on the problem ? Does it mean that the constructor should not destroy a passed in object if the constructor algorithm results in an error?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Right.  Previously, the Request constructor set the body used flag on the input immediately.  This effectively consumed the input Request even if the constructor threw an error.

The new spec text is careful not to set the body used flag until success is guaranteed.

The text can be found here:

  https://fetch.spec.whatwg.org/#dom-request

See the logic around `temporaryBody` and setting the input body used flag, etc.  Compare that to the code in dom/fetch/Request.cpp's Constructor() method.

Comment 5

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1134609-RequestConstructorLessDestructive

Hello Ben, 

I've tried to implement the spec, feedback is appreciated.

Also there are some outdated comments that point a different step number than the current spec, should that be fixed as a separate bug or should I include it in this one?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635213 [details] [diff] [review]
bug-1134609-RequestConstructorLessDestructive

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

This looks pretty good to me.

Can you add a test, though?  I think something along the lines of:

  var r1 = new Request('http://example.com', { method: 'POST', body: 'hello world' });
  try {
    var r2 = new Request(r1, { method: 'GET' });
  } catch() {
    // should throw
  }
  ok(!r1.bodyUsed, 'initial request should not be consumed by failed Request constructor');

::: dom/fetch/Request.cpp
@@ +137,2 @@
>    nsRefPtr<InternalRequest> request;
>    bool inputRequestHasBody = false;

I think you can remove inputRequestHasBody now.

@@ +325,5 @@
>        requestHeaders->Append(NS_LITERAL_CSTRING("Content-Type"),
>                               contentType, aRv);
>      }
>  
>      if (aRv.Failed()) {

While you're here, can you add an NS_WARN_IF() to this Failed() check?

Comment 7

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1134609-RequestConstructorLessDestructive

Added test case, there are several other aRv.Failed() checks without NS_WARN_IF() in the constructor, should I also change them in this patch?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8645879 [details] [diff] [review]
bug-1134609-RequestConstructorLessDestructive

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

Thanks! r=me

We don't need to change the other failure checks to warn right now.  Lets just do the one we're touching.

Comment 9

[Antonio de Luna [:A-deLuna]]

I'm trying to push to Try, but I'm getting this error, what could be the cause? 

>>pushing to ssh://hg.mozilla.org/try
>>no revisions specified to push; using . to avoid pushing multiple heads
>>searching for changes
>>remote: abort: could not lock working directory of /repo/hg/mozilla/try: Permission denied                                                                    
>>abort: stream ended unexpectedly (got 0 bytes, expected 4)

patch message is "try: -b do -p all -u all[x64] -t none"

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Do you have your ssh key enabled for hg?  I use ssh-agent and ssh-add to do this.

Also, what command are you using to do the push?  hg push -f ssh://hg.mozilla.org/try?

Comment 11

[Antonio de Luna [:A-deLuna]]

It's all fixed now, there was a problem with my account permissions.

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

Comment 12

[Antonio de Luna [:A-deLuna]]

I'm running into trouble when implementing this part of the spec:

>>35. If input is a Request object and input's body is non-null, run these substeps:
>>    1. Set input's body to null.
>>    2. Set input's used flag. 

But InternalRequest has an assertion that prevents us from doing this. https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.h#322

What should be done in this case?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Looking at the steps here:

  https://fetch.spec.whatwg.org/#dom-request

I don't think InternalRequest::GetRequestConstructorCopy should be copying the body.  I don't see it in:

  "Set request to a new request whose url is request's current url, method is request's method, header list is a copy of request's header list, unsafe-request flag is set, client is entry settings object, window is window, origin is origin, force-Origin-header flag is set, same-origin data-URL flag is set, referrer is request's referrer, referrer policy is request's referrer policy, destination is the empty string, mode is request's mode, credentials mode is request's credentials mode, cache mode is request's cache mode, redirect mode is request's redirect mode, and integrity metadata is request's integrity metadata."

With your changes it seems the body will get set on the new request via your temporaryBody variable after the copy is made.

So I *think* you can remove this line:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp#32

I would recommend replacing it with a comment that the body is handled separately by the Request::Constructor algorithm.

Then you just need to deal with the SetBody(nullptr) case.  I think you can address that by changing the SetBody() assert to MOZ_ASSERT_IF(aStream, !mBodyStream).

Comment 14

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1134609-RequestConstructorLessDestructive

Added change to InternalRequest::SetBody() assertion.

Tried to remove the line you suggested: https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp#32

But when running dom/tests/mochitest/fetch/test_fetch_basic_http.html I get the following error:

>>39 INFO SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the JSON data
>>40 INFO JSON.parse: unexpected end of data at line 1 column 1 of the JSON data

Comment 15

[Antonio de Luna [:A-deLuna]]

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

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8649020 [details] [diff] [review]
bug-1134609-RequestConstructorLessDestructive

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

Well, if all the tests pass without removing that line, LGTM!

Can you update your tree and do another try, though, to see whats up with those leaks?  They seem unrelated, but lets verify.  Thanks.

Comment 17

[Antonio de Luna [:A-deLuna]]

All right all tests passed, I'll mark for checkin needed. Thanks again Ben!

try build:

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4ca69ff980

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4ca69ff980

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e4ca69ff980