Fix #6379

[boghison]

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5450

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[jdm]

-S-awaiting-review +S-needs-code-changes

Can you run the websocket tests and see if any new failures are reported? ./mach test-wpt websockets should do the trick.

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{components/script/dom/websocket.rs, line 152 [r1] (raw file):}
http://doc.servo.org/websocket/header/struct.Origin.html
request.headers.set(Origin(global.get_url().serialize()));

---

Comments from the review on Reviewable.io

[boghison]

@jdm I ran the tests (both with this commit and without it). There's a very large output and as far as I can see a lot of tests aren't passed in both cases. Is there any way to test specifically against the origin header?

[jdm]

You can run the specific test that @Ms2ger mentioned in the issue with ./mach test path/to/test.html. That being said, the actual failures that are reported by the harness are summarized at the end of the run. Those are tests where the output is different than expected; we have a lot of tests that do not succeed, but that is an expected failure so it doesn't get reported.

[boghison]

@jdm OK, I ran the specific test, but it panicked "thread 'ScriptTask PipelineId(0)' panicked at 'called Result::unwrap() on an Err value: ResponseError("Sec-WebSocket-Accept is invalid")', src/libcore/result.rs:731

[boghison]

Umm, excuse me, is someone going to merge this or not (it now sends the origin header, I tested it myself)

[Ms2ger]

It looks like there are outstanding review comments on reviewable.

[boghison]

@Ms2ger Oh sorry, I didn't see the line comment, gonna change it now

[jdm]

@bors-servo: r+
-S-awaiting-review +S-awaiting-merge

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 5d8ab71 has been approved by jdm

[bors-servo]

⌛ Testing commit 5d8ab71 with merge 54a18b5...

[bors-servo]

💔 Test failed - mac1

[jdm]

/websockets/constructor/010.html
--------------------------------
TIMEOUT WebSockets: protocol in response but no requested protocol
TIMEOUT expected CRASH [Parent]
/websockets/constructor/011.html
--------------------------------
TIMEOUT WebSockets: protocol mismatch
TIMEOUT expected CRASH [Parent]
/websockets/constructor/012.html
--------------------------------
TIMEOUT WebSockets: no protocol in response
TIMEOUT expected CRASH [Parent]
/websockets/cookies/002.html
----------------------------
FAIL WebSockets: Set-Cookie in response
OK expected CRASH [Parent]
/websockets/cookies/004.html
----------------------------
OK expected CRASH [Parent]
/websockets/cookies/007.html
----------------------------
FAIL WebSockets: when to process set-cookie fields in ws response
OK expected CRASH [Parent]
/websockets/cookies/005.html
----------------------------
TIMEOUT WebSockets: setting HttpOnly cookies in ws response, checking ws request
TIMEOUT expected CRASH [Parent]
/websockets/opening-handshake/005.html
--------------------------------------
TIMEOUT WebSockets: proper first line
TIMEOUT expected CRASH [Parent]

[SimonSapin]

@boghison For future PRs, could you write more descriptive titles? Thanks! (“Fix #6379” alone does not say much when you don’t know from memory what #6379 is about.)

[boghison]

@SimonSapin Ok
@jdm I am not sure whether these errors are caused by my changes

[jdm]

@boghison They are all differences in the test output caused by your changes, so the test expectation files need to be updated to match them, eg. http://mxr.mozilla.org/servo/source/tests/wpt/metadata/websockets/opening-handshake/005.html.ini

[boghison]

@jdm I've never edited these, what do I have to do?

[boghison]

Actually nevermind, I just have to replace CRASH with what it prints that happened, right?

[jdm]

Precisely. For the tests with new FAIL/TIMEOUT output (rather than a FOO expected BLAH), you need a new entry like:

[output message goes here]
  expected: FAIL

[jdm]

@boghison The other option is to follow the steps at http://mxr.mozilla.org/servo/source/tests/wpt/README.md#71 and update all the expectations automatically.

[boghison]

@jdm I already started doing that ~5 mins ago, thanks for writing a guide (for noobs like me) :)

[bors-servo]

☔ The latest upstream changes (presumably #6610) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

Oh sorry, github doesn't notify us when a new push happens so I didn't see the new commit. Unfortunately there are a lot of WPT test expectations here that don't belong, so I'll try rebasing this locally.

[boghison]

@jdm OK