Fix a few things regarding redirects #25357

Eijebong · 2019-12-21T14:22:13Z

See individual commits

highfive · 2019-12-21T14:22:19Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/fetch.rs, components/script/dom/response.rs
@KiChjang: components/net/http_loader.rs, components/net_traits/lib.rs, components/script/fetch.rs, components/net_traits/response.rs, components/script/dom/response.rs

Eijebong · 2019-12-21T14:22:22Z

@bors-servo try=wpt

bors-servo · 2019-12-21T14:22:26Z

⌛ Trying commit 1589101 with merge 6d50e3e...


        Auto merge of #25357 - Eijebong:redirects, r=<try>

Fix a few things regarding redirects See individual commits

bors-servo · 2019-12-21T15:03:57Z

💔 Test failed - status-taskcluster

Eijebong · 2019-12-21T15:05:23Z

▶ TIMEOUT [expected OK] /fetch/api/redirect/redirect-method.any.html
  │ TIMEOUT [expected PASS] Redirect 301 with POST

👀

Eijebong · 2019-12-21T15:07:35Z

I can repro locally, guess I failed some rebase before pushing...

Eijebong · 2019-12-21T15:09:30Z

@bors-servo try=wpt

bors-servo · 2019-12-21T15:09:35Z

⌛ Trying commit 2ed6ed5 with merge 6b14697...


        Auto merge of #25357 - Eijebong:redirects, r=<try>

Fix a few things regarding redirects See individual commits

bors-servo · 2019-12-21T15:44:56Z

💔 Test failed - status-taskcluster

Eijebong · 2019-12-21T15:51:30Z

@bors-servo retry

1 unexpected results that are NOT known-intermittents:
  ▶ FAIL [expected PASS] /css/CSS2/ui/outline-color-101.xht
  │   → /css/CSS2/ui/outline-color-101.xht 54a9df64f1476dd12020019d7cf22ac34d727bc0
  │   → /css/CSS2/reference/boxes_same_color_100px_black_50px_margin.xht bb2751bfe2e9b16331e4522f296d899b22e12085
  └   → Screenshot is solid color 0xFFFFFF for /css/CSS2/ui/outline-color-101.xht

#24726

bors-servo · 2019-12-21T15:51:34Z

⌛ Trying commit 2ed6ed5 with merge b5d4abd...


        Auto merge of #25357 - Eijebong:redirects, r=<try>

Fix a few things regarding redirects See individual commits

bors-servo · 2019-12-21T16:08:32Z

💔 Test failed - status-taskcluster

jdm · 2019-12-21T16:57:08Z

  ▶ Unexpected subtest result in /xhr/send-redirect-to-cors.htm:
  │ FAIL [expected PASS] XMLHttpRequest: send() - Redirect to CORS-enabled resource (303 LALA with string and explicit Content-Type safelisted)
  │   → assert_equals: expected "application/x-pony" but got "NO"
  │ FAIL [expected PASS] XMLHttpRequest: send() - Redirect to CORS-enabled resource (301 POST with string and explicit Content-Type safelisted)
  │   → assert_equals: expected "application/x-pony" but got "NO"
  │ 
  │ redirect/</client.onreadystatechange<@http://web-platform.test:8000/xhr/send-redirect-to-cors.htm:41:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1984:25
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:2009:35
  ▶ Unexpected subtest result in /xhr/send-redirect-to-cors.htm:
  │ FAIL [expected PASS] XMLHttpRequest: send() - Redirect to CORS-enabled resource (301 POST with string and explicit Content-Type)
  │   → assert_equals: expected 0 but got 200
  │ FAIL [expected PASS] XMLHttpRequest: send() - Redirect to CORS-enabled resource (302 POST with string and explicit Content-Type)
  │   → assert_equals: expected 0 but got 200
  │ 
  │ redirect/</client.onreadystatechange<@http://web-platform.test:8000/xhr/send-redirect-to-cors.htm:45:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1984:25
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:2009:35
  ▶ Unexpected subtest result in /xhr/send-redirect-post-upload.htm:
  └ PASS [expected FAIL] XMLHttpRequest: The send() method: POSTing to URL that redirects (301)
  ▶ Unexpected subtest result in /xhr/send-redirect-post-upload.htm:
  └ PASS [expected FAIL] XMLHttpRequest: The send() method: POSTing to URL that redirects (302)
  ▶ Unexpected subtest result in /xhr/send-redirect-post-upload.htm:
  └ PASS [expected FAIL] XMLHttpRequest: The send() method: POSTing to URL that redirects (303)

Eijebong · 2020-04-18T14:16:25Z

So I'm very confused right now. Looking at /xhr/send-redirect-to-cors.htm failures.

I looked at firefox's implementation if the step 11 of redirect-fetch (https://fetch.spec.whatwg.org/#http-redirect-fetch), which is here: https://searchfox.org/mozilla-central/rev/aec63591821712236a522f7f55116f582ed7c920/dom/fetch/FetchDriver.cpp#1318

At first I thought I was going crazy because the condition is exactly the same but firefox passes those tests. But then I removed the line actually skipping those headers in the redirected request at https://searchfox.org/mozilla-central/rev/aec63591821712236a522f7f55116f582ed7c920/dom/fetch/FetchDriver.cpp#1444 and the test still passed. Apparently, firefox never goes through that AsyncOnChannelRedirect method for XHR requests but does for fetch ones. Now the question is: Why ?

The spec here says that for XHR requests, you should be using the fetch API even on XHR (https://xhr.spec.whatwg.org/#the-send()-method) so you should be removing those headers, right ?

Am I missing something important here ? Is the spec wrong ? Are the tests wrong ? Is my code wrong ? Is servo's handling of XHR wrong ?

jdm · 2020-04-21T20:50:28Z

Gecko doesn't use Fetch internally for its XHR implementation. Instead it uses XMLHttpRequestMainThread::AsyncOnChannelRedirect.

jdm · 2020-04-21T21:22:52Z

I suspect the XHR code ends up hitting https://searchfox.org/mozilla-central/rev/3446310d6cc5c85cde16a82eccf560e9b71a3d44/netwerk/protocol/http/HttpBaseChannel.cpp#3625-3632 and https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#84 doesn't include Content-Type.

jdm · 2020-04-21T21:50:18Z

Welp. The test doesn't match the specification, and the right behaviour is not clear to me. I have filed web-platform-tests/wpt#23156 upstream about resolving the mismatch between the Fetch and XHR tests somehow; I propose we merge these changes with the expected failures.


        Strip request-body-header when redirecting from a POST to GET

This doesn't change any expectation because we're not setting response.redirected properly so all the tests fail later on when it's asserted to be true. Fixes #25257


        Fix the redirected attribute for Response


        Fix a mistake in the redirect fetch code

The spec says to ignore both HEAD and GET in step 11

Eijebong · 2020-05-05T19:06:29Z

@bors-servo try=wpt


        Auto merge of #25357 - Eijebong:redirects, r=<try>

Fix a few things regarding redirects See individual commits

bors-servo · 2020-05-05T19:06:34Z

⌛ Trying commit d689921 with merge 8f2e822...


        Remove outdated HTTP redirection handling code.


        Update test expectations

bors-servo · 2020-05-05T19:43:27Z

☀️ Test successful - status-taskcluster
State: approved= try=True

jdm · 2020-05-05T19:51:16Z

@bors-servo r+

bors-servo · 2020-05-05T19:51:17Z

📌 Commit d86a429 has been approved by jdm

bors-servo · 2020-05-05T19:51:22Z

⌛ Testing commit d86a429 with merge c8c1fec...


        Auto merge of #25357 - Eijebong:redirects, r=jdm

Fix a few things regarding redirects See individual commits

bors-servo · 2020-05-05T20:23:45Z

💔 Test failed - status-taskcluster

Eijebong · 2020-05-05T20:27:56Z

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /css/css-transitions/properties-value-inherit-003.html:
  │ FAIL [expected PASS] outline-offset length-em(em) / events
  │   → assert_equals: Expected TransitionEnd events triggered on .transition expected "" but got "outline-offset:2s"
  │ 
  │ assertExpectedEventsFunc/<@http://web-platform.test:8000/css/css-transitions/support/generalParallelTest.js:207:26
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1975:25
  │ done@http://web-platform.test:8000/css/css-transitions/properties-value-inherit-003.html:134:34
  │ concludeSlice/</</<@http://web-platform.test:8000/css/css-transitions/support/runParallelAsyncHarness.js:122:59
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1975:25
  │ concludeSlice/</<@http://web-platform.test:8000/css/css-transitions/support/runParallelAsyncHarness.js:121:38
  │ concludeSlice/<@http://web-platform.test:8000/css/css-transitions/support/runParallelAsyncHarness.js:120:23
  └ concludeSlice@http://web-platform.test:8000/css/css-transitions/support/runParallelAsyncHarness.js:118:19

Sounds unrelated

jdm · 2020-05-05T20:59:07Z

@bors-servo retry

#26435

bors-servo · 2020-05-05T20:59:12Z

⌛ Testing commit d86a429 with merge 2471e9d...

bors-servo · 2020-05-05T21:19:58Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 2471e9d to master...

highfive assigned jdm Dec 21, 2019

highfive added the S-awaiting-review label Dec 21, 2019

highfive added the S-tests-failed label Dec 21, 2019

Eijebong force-pushed the Eijebong:redirects branch from 1589101 to 2ed6ed5 Dec 21, 2019

highfive removed the S-tests-failed label Dec 21, 2019

highfive added the S-tests-failed label Dec 21, 2019

pshaughn mentioned this pull request Dec 22, 2019

XHR POST to an URL that redirects is behaving badly #24951

Open

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 21, 2020

jdm force-pushed the Eijebong:redirects branch from 2ed6ed5 to 552e44e Apr 22, 2020

Eijebong added 3 commits Dec 21, 2019

Strip request-body-header when redirecting from a POST to GET

566147d

This doesn't change any expectation because we're not setting response.redirected properly so all the tests fail later on when it's asserted to be true. Fixes #25257

Fix the redirected attribute for Response

aa43ce8

Fix a mistake in the redirect fetch code

2b28042

The spec says to ignore both HEAD and GET in step 11

Eijebong force-pushed the Eijebong:redirects branch from 23065ad to d689921 May 5, 2020

jdm and others added 2 commits Apr 22, 2020

Remove outdated HTTP redirection handling code.

4a1732a

Update test expectations

Loading status checks…

d86a429

Eijebong force-pushed the Eijebong:redirects branch from d689921 to d86a429 May 5, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels May 5, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels May 5, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels May 5, 2020

bors-servo merged commit 2471e9d into servo:master May 5, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 5, 2020

servo / servo

Fix a few things regarding redirects #25357

Fix a few things regarding redirects #25357

Eijebong commented Dec 21, 2019

highfive commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

Eijebong commented Dec 21, 2019 •

edited

Eijebong commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

jdm commented Dec 21, 2019

Eijebong commented Apr 18, 2020

jdm commented Apr 21, 2020

jdm commented Apr 21, 2020

jdm commented Apr 21, 2020

Eijebong commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

jdm commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

Eijebong commented May 5, 2020

jdm commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

servo / servo

Join GitHub today

Fix a few things regarding redirects #25357

Fix a few things regarding redirects #25357

Conversation

Eijebong commented Dec 21, 2019

highfive commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

Eijebong commented Dec 21, 2019 • edited

Eijebong commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

Eijebong commented Dec 21, 2019

bors-servo commented Dec 21, 2019

bors-servo commented Dec 21, 2019

jdm commented Dec 21, 2019

Eijebong commented Apr 18, 2020

jdm commented Apr 21, 2020

jdm commented Apr 21, 2020

jdm commented Apr 21, 2020

Eijebong commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

jdm commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

Eijebong commented May 5, 2020

jdm commented May 5, 2020

bors-servo commented May 5, 2020

bors-servo commented May 5, 2020

Eijebong commented Dec 21, 2019 •

edited