Closed Bug 1184550 Opened 9 years ago Closed 8 years ago

Request constructor should always throw if used flag is set, even if body is null

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bkelly, Assigned: thomaskuyper, Mentored)

Details

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

Attachments

(2 files, 10 obsolete files)

Hi Sir! 

I would like to work on this bug. Where should I look for this part of code?
Flags: needinfo?(bkelly)
Essentially, you need to move the BodyUsed() check to occur before the if(body) block here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp#140
Flags: needinfo?(bkelly)
Assignee: nobody → sanchitkum
Attached patch bug1184550_BodyUsedfix.diff (obsolete) — Splinter Review
Moved BodyUsed() check to occur before the if(body) block.
Attachment #8635748 - Flags: feedback?(bkelly)
Comment on attachment 8635748 [details] [diff] [review]
bug1184550_BodyUsedfix.diff

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

This is the right idea.  Can you take a look at the existing tests in test_request.js:

  https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_request.js

And add something to verify this behavior?  I think something like this should do it:

  var req = new Request(url, {body: 'hello'});
  fetch(req);
  fetch(req);  // this should reject with new changes

Or maybe just:

  var req = new Request(url, {body: 'hello'});
  var req2 = new Request(req);
  var req3 = new Request(req);  // this should throw with new changes

You can run these tests with:
  
  ./mach mochitest dom/tests/mochitest/fetch/test_request.js

Also, just FYI, I'm on travel this week and may be slow to respond to feedback and review requests.

::: dom/fetch/Request.cpp
@@ +138,5 @@
> +    if (inputReq->BodyUsed()) {
> +      aRv.ThrowTypeError(MSG_FETCH_BODY_CONSUMED_ERROR);
> +      return nullptr;
> +    } else {
> +      inputReq->SetBodyUsed();

I think the SetBodyUsed() call should just happen inside the if (body) check right after inputRequestHasBody = true statement.
Attachment #8635748 - Flags: feedback?(bkelly) → feedback+
Attached patch bug1184550_BodyUsedfix.diff (obsolete) — Splinter Review
Moved SetBodyUsed() call inside the if (body)
Attachment #8637891 - Flags: feedback?(bkelly)
Attached patch bug1184550_AddedTests.diff (obsolete) — Splinter Review
Hi Sir,

Initially, without the added test case, all tests passed successfully.

After Addition of these test cases, there was one case which failed:

failed | uncaught exception - TypeError: HEAD or GET Request cannot have a body. at http://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_request.js:460

Is this the expected behaviour? Am I missing something?

Sorry for the late response, I have a presentation in the coming week which is consuming most of my time.

Thanks :)
Attachment #8637892 - Flags: feedback?(bkelly)
Comment on attachment 8637892 [details] [diff] [review]
bug1184550_AddedTests.diff

Sorry, the sketch of the test I gave you is not 100% complete.

The error you are running into means you need to specify the POST method in the new Request:

  var req = new Request("/index.html", {method: 'POST', body: 'hello'});

Also, the test needs to verify that the promise returned from the second fetch rejects.  You can do something like this:

  var req = new Request("/index.html", {method: 'POST', body: 'hello'});
  fetch(req);
  return fetch(req).then(function(resp) {
    ok(false, 'second fetch with same request should not succeed');
  }).catch(function(err) {
    is(err.name, 'TypeError', 'second fetch with same request should fail');
  });

And then you have to invoke your new test function in the list of async tests in the promise chain.

Hope that helps.  Thanks!
Attachment #8637892 - Flags: feedback?(bkelly)
Attachment #8637891 - Flags: feedback?(bkelly) → feedback+
Attached patch Bug fix for 1184550 (obsolete) — Splinter Review
I saw this was has not been touched for about 5 months and figured I'd have a go at it.
Attachment #8705750 - Flags: review?(bkelly)
Attached patch Test for Bug 1184550 (obsolete) — Splinter Review
Attachment #8705752 - Flags: review?(bkelly)
Attached patch bug1184550.patch (obsolete) — Splinter Review
Sorry, disregard the other two patches. The first one is empty and the other has both the bug fix and the test code in it. This is (I double checked) just the bug code.
Attachment #8705750 - Attachment is obsolete: true
Attachment #8705750 - Flags: review?(bkelly)
Attachment #8705820 - Flags: review?(bkelly)
Attached patch bug1184550Test.patch (obsolete) — Splinter Review
Attachment #8705752 - Attachment is obsolete: true
Attachment #8705752 - Flags: review?(bkelly)
Attachment #8705822 - Flags: review?(bkelly)
Comment on attachment 8705820 [details] [diff] [review]
bug1184550.patch

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

Thanks!  r=me
Attachment #8705820 - Flags: review?(bkelly) → review+
Comment on attachment 8705822 [details] [diff] [review]
bug1184550Test.patch

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

r=me with comment addressed.  Thanks!

::: dom/tests/mochitest/fetch/test_request.js
@@ +127,5 @@
> +// Bug 1184550 - Request constructor should always throw if used flag is set,
> +// even if body is null
> +function testBug1184550() {
> +  var req = new Request("", { method: 'post', body: "Test" });
> +  fetch(req);

Please add an assertion after the first fetch() on bodyUsed.  Something like:

  ok(req.bodyUsed, 'request body should be used immediately after fetch()');
Attachment #8705822 - Flags: review?(bkelly) → review+
Attached patch 1184550test.diff (obsolete) — Splinter Review
Can I be put as the assigned person on this bug?

Thanks
Attachment #8705822 - Attachment is obsolete: true
Attachment #8707249 - Flags: review?(bkelly)
Attachment #8707249 - Flags: review?(bkelly) → review+
Assignee: sanchitkum → thomaskuyper
Now that both patches have been reviewed, do I just add the checkin-needed flag? Or do I need to check out the most recent version and reapply the patches to make sure they work before doing that?
Flags: needinfo?(bkelly)
Please:

1) Mark old patches obsolete by clicking details, "edit details", and then check obsolete.
2) Reload the test patch with a commit message including the bug number, short description, and r=bkelly
3) Push a try build.  If you don't have permissions for this I can do it for you:

  https://wiki.mozilla.org/Build:TryServer#How_to_push_to_try

You need L1 access to have permissions to try:

  https://www.mozilla.org/en-US/about/governance/policies/commit/
4) If the try build comes back green then you can add the checkin-needed flag.

Does that all make sense?  Thanks again!
Flags: needinfo?(bkelly)
Attachment #8635748 - Attachment is obsolete: true
Attachment #8637891 - Attachment is obsolete: true
Attachment #8637892 - Attachment is obsolete: true
Attached patch 1184550.diff (obsolete) — Splinter Review
Updated patch notes.
Attachment #8705820 - Attachment is obsolete: true
Attached patch 1184550test.diff (obsolete) — Splinter Review
Updated patch notes per bkelly.

Is one of the attachment flags for requesting a push to the try server? I don't have access, so will need this done by someone with access.
Attachment #8707249 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Here is a try build:

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

Can you also export patches with user info?  See step 1 here:

  https://www.mercurial-scm.org/wiki/QuickStart

Also, please change the test patch commit message to mention it contains a test for the request constructor.

Thanks.
Flags: needinfo?(bkelly) → needinfo?(thomaskuyper)
Attached patch 1184550.diffSplinter Review
Added user info to the patch file
Attachment #8707706 - Attachment is obsolete: true
Flags: needinfo?(thomaskuyper)
Attached patch 1184550test.diffSplinter Review
Added user info to patch, updated commit message to mention test for Request constructor.
Attachment #8707707 - Attachment is obsolete: true
(In reply to Ben Kelly [:bkelly] from comment #19)
> Here is a try build:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0776059803
> 
The build seems to be stuck at 97%, but the vast majority of builds are successful and none of the errors look to be in the request module or added test. I haven't found any criteria on the Mozilla wiki or Code Firefox videos for determining if a try build is successful or not. Is it just a value judgement?
Flags: needinfo?(bkelly)
There was some sort of glitch earlier today that dropped a few builds on the floor, which is probably where your other 3% are.  Basically as long as you don't see any relevant patterns in the test failures, you're good to check in.  Your tryserver run looks fine to me.
Keywords: checkin-needed
Yep.  Everything looks good to me too.  Thanks for working this!
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #24)
> Yep.  Everything looks good to me too.  Thanks for working this!

Thank you for mentoring me! You made my introduction to contributing to Mozilla a very positive experience. I will definitely continue to contribute, and I hope I get to work with you on other bugs!
https://hg.mozilla.org/mozilla-central/rev/e8a0923f14df
https://hg.mozilla.org/mozilla-central/rev/11498fa5b56a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.