Make step 3 of XHR's SetResponseType method match the specification #9562

timvandermeij · 2016-02-05T22:24:37Z

Fixes #9552.

highfive · 2016-02-05T22:24:41Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-02-05T22:29:26Z

@bors-servo r+

Looks good enough to me. Thanks for taking up the issue!

bors-servo · 2016-02-05T22:29:27Z

📌 Commit 6343e64 has been approved by KiChjang

bors-servo · 2016-02-05T23:19:13Z

⌛ Testing commit 6343e64 with merge f4214cd...


        Auto merge of #9562 - timvandermeij:setresponsetype-spec, r=KiChjang

Make step 3 of XHR's SetResponseType method match the specification Fixes #9552.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9562)

bors-servo · 2016-02-06T00:06:01Z

💔 Test failed - linux-rel

KiChjang · 2016-02-06T00:18:06Z

This code actually threw errors on the WPT tests:

Tests with unexpected results:
  ▶ Unexpected subtest result in /XMLHttpRequest/responsetype.html:
  │ FAIL [expected PASS] Set responseType to "" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ FAIL [expected PASS] Set responseType to "json" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ FAIL [expected PASS] Set responseType to "document" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ FAIL [expected PASS] Set responseType to "arraybuffer" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ FAIL [expected PASS] Set responseType to "blob" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ FAIL [expected PASS] Set responseType to "text" when readyState is DONE and the sync flag is set.
  │   → assert_throws: function "function () {
      xhr.responseType = type;
    }" threw object "InvalidAccessError: The object does not support the opera..." that is not a DOMException InvalidStateError: property "code" is equal to 15, expected 11
  │ 
  │ @http://web-platform.test:8000/XMLHttpRequest/responsetype.html:80:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ @http://web-platform.test:8000/XMLHttpRequest/responsetype.html:75:1
  └ @http://web-platform.test:8000/XMLHttpRequest/responsetype.html:7:1

KiChjang · 2016-02-06T00:20:17Z

components/script/dom/xmlhttprequest.rs

@@ -676,12 +676,12 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
    fn SetResponseType(&self, response_type: XMLHttpRequestResponseType) -> ErrorResult {
        match self.global() {
            GlobalRoot::Worker(_) if response_type == XMLHttpRequestResponseType::Document
-            => return Ok(()),
+                => return Ok(()),
+            GlobalRoot::Window(_) if self.sync.get() => return Err(Error::InvalidAccess),


I suspect that the test failures are due to the re-ordering of steps. The match-arm for GlobalRoot::Worker(_) above is Step 1, whereas the match-arm for GlobalRoot::Window(_) if self.sync.get() is Step 3, and down there the match for self.ready_state.get() is Step 2.

timvandermeij · 2016-02-06T14:40:54Z

Aw, I had hoped that that would work. I have pushed an updated patch that executes the steps in the exact order as provided by the specification. Could you check this again and re-trigger the tests?

KiChjang · 2016-02-06T20:58:11Z

components/script/dom/xmlhttprequest.rs

+            _ => {}
+        }
+        match self.global() {
+            GlobalRoot::Window(_) if self.sync.get() => Err(Error::InvalidAccess),


if let (GlobalRoot::Window(_), true) = (self.global(), self.sync.get()) { ... } else { ... }

Done in the new commit.

KiChjang · 2016-02-06T20:59:45Z

components/script/dom/xmlhttprequest.rs

-            XMLHttpRequestState::Loading | XMLHttpRequestState::Done => Err(Error::InvalidState),
-            _ if self.sync.get() => Err(Error::InvalidAccess),
+            XMLHttpRequestState::Loading | XMLHttpRequestState::Done => return Err(Error::InvalidState),
+            _ => {}


The next check can be done here, so that we don't need an explicit return keyword.

Done in the new commit.

KiChjang · 2016-02-06T21:24:15Z

@bors-servo try

bors-servo · 2016-02-06T21:24:17Z

⌛ Trying commit 40980e0 with merge a6fb768...


        Auto merge of #9562 - timvandermeij:setresponsetype-spec, r=<try>

Make step 3 of XHR's SetResponseType method match the specification Fixes #9552.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9562)


        Make step 3 of XHR's SetResponseType method match the specification

KiChjang · 2016-02-06T21:45:29Z

@bors-servo r+

bors-servo · 2016-02-06T21:45:30Z

📌 Commit 816c65a has been approved by KiChjang

bors-servo · 2016-02-06T21:45:32Z

⌛ Testing commit 816c65a with merge 2f52a16...


        Auto merge of #9562 - timvandermeij:setresponsetype-spec, r=KiChjang

Make step 3 of XHR's SetResponseType method match the specification Fixes #9552.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9562)

bors-servo · 2016-02-06T22:55:57Z

💔 Test failed - mac-rel-wpt

wafflespeanut · 2016-02-06T23:00:45Z

@bors-servo retry

#9063

bors-servo · 2016-02-06T23:00:46Z

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

bors-servo · 2016-02-06T23:05:04Z

💔 Test failed - mac-rel-css

wafflespeanut · 2016-02-06T23:07:18Z

@bors-servo retry

#7791

bors-servo · 2016-02-06T23:07:19Z

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

bors-servo · 2016-02-07T00:10:22Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Feb 5, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 5, 2016

KiChjang self-assigned this Feb 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 6, 2016

KiChjang reviewed Feb 6, 2016
View changes

KiChjang added S-needs-code-changes and removed S-tests-failed labels Feb 6, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 6, 2016

KiChjang reviewed Feb 6, 2016
View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Feb 6, 2016

Make step 3 of XHR's SetResponseType method match the specification

Loading status checks…

816c65a

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 6, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 6, 2016

highfive added the S-tests-failed label Feb 6, 2016

bors-servo merged commit 816c65a into servo:master Feb 7, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Feb 7, 2016

Clean up XHR API #9383

Merged

timvandermeij deleted the timvandermeij:setresponsetype-spec branch Feb 7, 2016

servo / servo

Make step 3 of XHR's SetResponseType method match the specification #9562

Make step 3 of XHR's SetResponseType method match the specification #9562

timvandermeij commented Feb 5, 2016

highfive commented Feb 5, 2016

KiChjang commented Feb 5, 2016

bors-servo commented Feb 5, 2016

bors-servo commented Feb 5, 2016

bors-servo commented Feb 6, 2016

KiChjang commented Feb 6, 2016

This comment has been minimized.

timvandermeij commented Feb 6, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Feb 6, 2016

bors-servo commented Feb 6, 2016

KiChjang commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

wafflespeanut commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

wafflespeanut commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 7, 2016

servo / servo

Join GitHub today

Make step 3 of XHR's SetResponseType method match the specification #9562

Make step 3 of XHR's SetResponseType method match the specification #9562

Conversation

timvandermeij commented Feb 5, 2016

highfive commented Feb 5, 2016

KiChjang commented Feb 5, 2016

bors-servo commented Feb 5, 2016

bors-servo commented Feb 5, 2016

bors-servo commented Feb 6, 2016

KiChjang commented Feb 6, 2016

This comment has been minimized.

KiChjang Feb 6, 2016

timvandermeij commented Feb 6, 2016

This comment has been minimized.

KiChjang Feb 6, 2016

This comment has been minimized.

timvandermeij Feb 6, 2016

This comment has been minimized.

KiChjang Feb 6, 2016

This comment has been minimized.

timvandermeij Feb 6, 2016

KiChjang commented Feb 6, 2016

bors-servo commented Feb 6, 2016

KiChjang commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

wafflespeanut commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 6, 2016

wafflespeanut commented Feb 6, 2016

bors-servo commented Feb 6, 2016

bors-servo commented Feb 7, 2016