Closed Bug 1134609 Opened 5 years ago Closed 4 years ago

make Fetch Request constructor less destructive to input on error

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: A-deLuna, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 2 obsolete files)

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
Relevant code is the Request constructor in dom/fetch/Request.cpp.
Mentor: bkelly
I would like to work on this . Please assign it to me .
Assignee: nobody → technozoom88
Status: NEW → ASSIGNED
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?
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.
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?
Attachment #8635213 - Flags: review?(bkelly)
Assignee: technozoom88 → tonydelun
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?
Attachment #8635213 - Flags: review?(bkelly)
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?
Attachment #8635213 - Attachment is obsolete: true
Attachment #8645879 - Flags: review?(bkelly)
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.
Attachment #8645879 - Flags: review?(bkelly) → review+
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"
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?
It's all fixed now, there was a problem with my account permissions.

try build: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f81f0a8676e9
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?
Flags: needinfo?(bkelly)
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).
Flags: needinfo?(bkelly)
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
Attachment #8645879 - Attachment is obsolete: true
Attachment #8649020 - Flags: review?(bkelly)
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.
Attachment #8649020 - Flags: review?(bkelly) → review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e4ca69ff980
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.