Closed
Bug 1184550
Opened 9 years ago
Closed 9 years ago
Request constructor should always throw if used flag is set, even if body is null
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
854 bytes,
patch
|
Details | Diff | Splinter Review | |
1.25 KB,
patch
|
Details | Diff | Splinter Review |
Spec change:
https://github.com/whatwg/fetch/issues/55
Comment 1•9 years ago
|
||
Hi Sir!
I would like to work on this bug. Where should I look for this part of code?
Flags: needinfo?(bkelly)
Reporter | ||
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → sanchitkum
Comment 3•9 years ago
|
||
Moved BodyUsed() check to occur before the if(body) block.
Attachment #8635748 -
Flags: feedback?(bkelly)
Reporter | ||
Comment 4•9 years ago
|
||
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+
Comment 5•9 years ago
|
||
Moved SetBodyUsed() call inside the if (body)
Attachment #8637891 -
Flags: feedback?(bkelly)
Comment 6•9 years ago
|
||
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)
Reporter | ||
Comment 7•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8637891 -
Flags: feedback?(bkelly) → feedback+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8705752 -
Flags: review?(bkelly)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8705752 -
Attachment is obsolete: true
Attachment #8705752 -
Flags: review?(bkelly)
Attachment #8705822 -
Flags: review?(bkelly)
Reporter | ||
Comment 12•9 years ago
|
||
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+
Reporter | ||
Comment 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
Can I be put as the assigned person on this bug?
Thanks
Attachment #8705822 -
Attachment is obsolete: true
Attachment #8707249 -
Flags: review?(bkelly)
Reporter | ||
Updated•9 years ago
|
Attachment #8707249 -
Flags: review?(bkelly) → review+
Reporter | ||
Updated•9 years ago
|
Assignee: sanchitkum → thomaskuyper
Assignee | ||
Comment 15•9 years ago
|
||
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)
Reporter | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8635748 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8637891 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8637892 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Updated patch notes.
Attachment #8705820 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
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)
Reporter | ||
Comment 19•9 years ago
|
||
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)
Assignee | ||
Comment 20•9 years ago
|
||
Added user info to the patch file
Attachment #8707706 -
Attachment is obsolete: true
Flags: needinfo?(thomaskuyper)
Assignee | ||
Comment 21•9 years ago
|
||
Added user info to patch, updated commit message to mention test for Request constructor.
Attachment #8707707 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 24•9 years ago
|
||
Yep. Everything looks good to me too. Thanks for working this!
Flags: needinfo?(bkelly)
Assignee | ||
Comment 25•9 years ago
|
||
(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!
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a0923f14df
https://hg.mozilla.org/integration/mozilla-inbound/rev/11498fa5b56a
Keywords: checkin-needed
Comment 27•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8a0923f14df
https://hg.mozilla.org/mozilla-central/rev/11498fa5b56a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•