Implement WebSocket API #2088

TimothyGu · 2017-12-20T01:41:20Z

Depends on web-platform-tests/wpt#8746 to be merged and rolled before this could be applied.

Fixes #1195. Closes #2075.

Travis CI will fail due to the tests not being current.

TimothyGu · 2017-12-24T03:08:08Z

test/web-platform-tests/to-run.yaml

+Secure-Close-1005-verify-code.htm: [fail, See https://github.com/websockets/ws/issues/1257]
+closing-handshake/003.html*: [fail, See https://github.com/websockets/ws/issues/1257]
+interfaces/WebSocket/close/close-basic.html*: [fail, Mutates globals]
+interfaces/WebSocket/close/close-connecting.html*: [fail, Potentially buggy test as Chrome fails it too]


Firefox doesn’t fail it locally, however (even though wpt.fyi says otherwise). Fixing it seems to require digging through ws module's guts, which I didn't do.

This assertion fails in ws if close() is called when connecting as the 'close' event is emitted on the same tick.

This patch should fix the issue:

diff --git a/lib/websocket.js b/lib/websocket.js index e0ed7c5..0730510 100644 --- a/lib/websocket.js +++ b/lib/websocket.js @@ -152,8 +152,6 @@ class WebSocket extends EventEmitter { this._closeMessage = reason; this._closeCode = code; - if (this._finalized) return; - if (code === 1005) this.close(); else this.close(code, reason); }; @@ -213,15 +211,14 @@ class WebSocket extends EventEmitter { * @private */ emitClose () { - this.readyState = WebSocket.CLOSED; - - this.emit('close', this._closeCode, this._closeMessage); - if (this._extensions[PerMessageDeflate.extensionName]) { this._extensions[PerMessageDeflate.extensionName].cleanup(); } - this.removeAllListeners(); + process.nextTick(() => { + this.readyState = WebSocket.CLOSED; + this.emit('close', this._closeCode, this._closeMessage); + }); } /** @@ -282,7 +279,9 @@ class WebSocket extends EventEmitter { } if (this.readyState === WebSocket.CLOSING) { - if (this._closeFrameSent && this._closeFrameReceived) this._socket.end(); + if (this._closeFrameSent && this._closeFrameReceived && !this._finalized) { + this._socket.end(); + } return; }

Let me know if you need it or open a PR :)

The patch does not fix the issue, unfortunately :( Seems like the actual error thrown is

Error: Parse Error at Socket.socketOnData (_http_client.js:439:20) at Socket.emit (events.js:159:13) at addChunk (_stream_readable.js:265:12) at readableAddChunk (_stream_readable.js:252:11) at Socket.Readable.push (_stream_readable.js:209:10) at TCP.onread (net.js:608:20)

I assume we can't do much about this.

Hmm this is weird.

TimothyGu · 2017-12-24T03:09:19Z

test/web-platform-tests/to-run.yaml

+
+DIR: websockets
+
+Create-Secure-extensions-empty.htm: [fail, Buggy test as the test does not take into account the mandatory permessage-deflate extension]


All browsers fail this test, with Chrome and Firefox failing the same way we do: https://wpt.fyi/websockets/Create-Secure-extensions-empty.htm

TimothyGu · 2017-12-24T03:09:52Z

test/web-platform-tests/to-run.yaml

+
+Create-Secure-extensions-empty.htm: [fail, Buggy test as the test does not take into account the mandatory permessage-deflate extension]
+Secure-Close-1005-verify-code.htm: [fail, See https://github.com/websockets/ws/issues/1257]
+closing-handshake/003.html*: [fail, See https://github.com/websockets/ws/issues/1257]


These two have a chance of being fixed upstream soon. See linked ticket.

TimothyGu · 2017-12-24T03:16:31Z

test/web-platform-tests/to-run.yaml

+Create-Secure-extensions-empty.htm: [fail, Buggy test as the test does not take into account the mandatory permessage-deflate extension]
+Secure-Close-1005-verify-code.htm: [fail, See https://github.com/websockets/ws/issues/1257]
+closing-handshake/003.html*: [fail, See https://github.com/websockets/ws/issues/1257]
+interfaces/WebSocket/close/close-basic.html*: [fail, Mutates globals]


This test passes because we implement things correctly, but because the wrappers are shared across JSDOM windows it will affect the next test, thus causing failures. (This one was a pain to debug as one can imagine.)

I know at least one more test which behaves this way, though it's among the CSSOM tests and not included here yet. Annoying to debug, indeed.

Since we eventually want to run most failing tests to make sure that they're still failing, we ought to add a new failure reason ([mutates-globals] or something more general?) for these tests to make sure they won't ever run and affect others later. That doesn't need to be done in this pull request though.

Agreed on all counts.

We also need a new failure reason for flaky tests. I experienced one of them while auditing which tests we could enable after the WPT roll.

domenic · 2017-12-24T04:42:06Z

From what I can tell this also helps with #1629 / #1559, right? (Maybe #1911?) If so a test would be helpful.

domenic · 2017-12-24T04:42:25Z

Also it'd be good to add a WPT for the first commit.

TimothyGu · 2017-12-26T08:28:26Z

Comments addressed. WPT rolled. #1559 fixed. Travis is green.

This PR is ready for review :)

rgbkrk · 2017-12-26T18:34:01Z

This is so great to see, thank you @TimothyGu. Wish I could be of help in review, I'll just watch and learn from this one.

lpinca · 2017-12-28T17:00:46Z

lib/jsdom/living/websockets/WebSocket-impl.js

+        resolve();
+        this._onConnectionClosed(...args);
+      });
+      this._ws.on("headers", headers => {


This will be renamed to 'upgrade' in version 4. Listeners will receive the http.IncomingMessage instance.

Zirro

Great work, I can only offer nitpicks.

While not directly related to your code, I notice that the WPT Python server becomes rather noisy during the WebSocket tests. I wonder if we can do something about that without silencing potentially useful information.

Regardless, LGTM :-)

Zirro · 2017-12-28T17:49:38Z

lib/jsdom/living/events/EventModifierMixin-impl.js

+  // Event's constructor assumes all options correspond to IDL attributes with the same names, and sets them on `this`.
+  // That is not the case for these modifier boolean options, but since the options are set on `this` anyway we'll
+  // access them that way. The spec doesn't say much about the case where keyArg is not one of the valid ones
+  // (https://www.w3.org/TR/uievents-key/#keys-modifier), but at least Chrome returns false for invalid modifiers. Since


I think we always try to reference the latest available version of a spec even if that means a draft, so this should probably be https://w3c.github.io/uievents-key/#keys-modifier instead.

Yup, my bad.

Bump, in case this was missed.

Zirro · 2017-12-28T18:41:15Z

test/web-platform-tests/to-run.yaml

+Create-Secure-extensions-empty.htm: [fail, Buggy test as the test does not take into account the mandatory permessage-deflate extension]
+Secure-Close-1005-verify-code.htm: [fail, See https://github.com/websockets/ws/issues/1257]
+closing-handshake/003.html*: [fail, See https://github.com/websockets/ws/issues/1257]
+interfaces/WebSocket/close/close-basic.html*: [fail, Mutates globals]


I know at least one more test which behaves this way, though it's among the CSSOM tests and not included here yet. Annoying to debug, indeed.

Since we eventually want to run most failing tests to make sure that they're still failing, we ought to add a new failure reason ([mutates-globals] or something more general?) for these tests to make sure they won't ever run and affect others later. That doesn't need to be done in this pull request though.

Zirro · 2017-12-28T18:51:58Z

test/web-platform-tests/to-upstream/uievents/getModifierState.html

@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html dir="rtl">


Is the "rtl"-direction relevant here, or merely the result of copy-paste?

It was a copypasta issue 😕 thanks for spotting!

TimothyGu · 2017-12-29T00:08:48Z

Hey @Zirro, thanks for the review. I’ll fix the issues as soon as I fully wake up.

@lpinca thanks for giving us the heads-up. how soon do you anticipate ws v4.x to be released? If it’s going to be soon I might just put this PR on hold until it is.

~~I’ll test out the patch you have for the closing-when-connecting issue.~~ Done; it does not fix the issue. Not much we can do unfortunately.

Another issue we are seeing is around bufferedAmount – basically as it is currently implemented it does something else completely than intended (websockets/ws#492). For now we have a working workaround in jsdom but I’ll take a look at fixing it in ws as well.

TimothyGu · 2017-12-29T00:13:38Z

@lpinca Something else that could be help greatly is if you could release some RC versions of ws so we could test against it right now, and not have to wait until the formal release. I guess that also helps flesh out the bugs that may have creeped in the major release of ws :)

lpinca · 2017-12-29T06:59:23Z

I'm only waiting for someone to take a look at websockets/ws@4e8bd12 to see if error messages/types are good enough. They have been criticized a lot in the past.

Changes are breaking but trivial so it doesn't have much sense to go through a beta/RC phase.

This brings the event accessor handling more in-line with Web IDL.

TimothyGu · 2018-01-08T08:07:10Z

Permanently upgraded to ws@4. Only 5 failing tests left, none of which we can do much about.

domenic · 2018-01-16T05:12:29Z

lib/jsdom/living/events/UIEvent-impl.js


    // undefined check included so that we can omit the property in internal usage.
-    if (eventInitDict.view !== null && eventInitDict.view !== undefined) {
+    if (eventInitDict && eventInitDict.view !== null && eventInitDict.view !== undefined) {


When could eventInitDict be falsy? I would have though the Web IDL layer would have guaranteed it to be an object by now.

When we get lazy and call it like SomeEvent.createImpl(['name'])

domenic · 2018-01-16T05:14:08Z

test/web-platform-tests/to-run.yaml

 blob/Blob-constructor.html: [fail, "- Blob is not a function
                                    - a date is not instanceof Date
                                    - a regexp is not instanceof Regexp
                                    - strange v8 behaviour when error triggered in overridden array length
                                    - HTMLSelectElement does not have indexed properties
                                    - MessageChannel not implemented
                                    - element attributes does not have indexed properties"]
+file/File-constructor-endings.html: [needs-await]
+file/File-constructor.html: [fail, WPT bug with fix available at https://github.com/w3c/web-platform-tests/pull/8802]


Merged the PR if you want to roll again

domenic · 2018-01-16T05:18:11Z

lib/jsdom/living/events/TouchEvent.webidl

+    sequence<Touch> changedTouches = [];
+};
+
+[Constructor(DOMString type, optional TouchEventInit eventInitDict),


Please note in the commit message that this also adds the TouchEvent constructor

Well, I guess it won't work anyway, since we don't implement Touch instances or TouchList.

The `key in wrapper` check is fragile as it is susceptible to changes in the wrapper's prototype object.

Fixes jsdom#1559. Closes jsdom#1629.

TimothyGu · 2018-01-16T06:25:24Z

This should be ready to merge.

TimothyGu mentioned this pull request Dec 23, 2017

Empty closing handshake should use 1005 error code websockets/ws#1257

Closed

1 task

TimothyGu commented Dec 24, 2017

View reviewed changes

TimothyGu force-pushed the websocket branch from dc3adaf to 79306b6 Compare December 26, 2017 08:17

This was referenced Dec 26, 2017

Supported WebSocket implementations #1195

Closed

[WIP] Add WebSocket Support #2075

Closed

TimothyGu force-pushed the websocket branch 2 times, most recently from 6ed777e to f19090d Compare December 26, 2017 08:26

TimothyGu force-pushed the websocket branch from f19090d to f58c8df Compare December 26, 2017 08:41

TimothyGu mentioned this pull request Dec 27, 2017

Use new webidl2js bundle-entry.js feature #2092

Open

lpinca reviewed Dec 28, 2017

View reviewed changes

Zirro approved these changes Dec 28, 2017

View reviewed changes

TimothyGu force-pushed the websocket branch from f58c8df to 24af247 Compare December 29, 2017 03:19

TimothyGu added 4 commits January 7, 2018 23:56

Implement BlobPropertyBag#endings

e06c901

Make HashChangeEvent and PopStateEvent trusted

57d903a

Roll whatwg-url

8af9de9

Allow non-callable objects for event IDL attributes

5b9b699

This brings the event accessor handling more in-line with Web IDL.

TimothyGu force-pushed the websocket branch from 24af247 to 6be445e Compare January 8, 2018 08:06

domenic reviewed Jan 16, 2018

View reviewed changes

TimothyGu added 2 commits January 15, 2018 22:15

Roll WPT

b2e7cd5

Protect against global mutation in Event classes

8754073

The `key in wrapper` check is fragile as it is susceptible to changes in the wrapper's prototype object.

TimothyGu added 5 commits January 15, 2018 22:15

Copy default params in Event constructor

25e85cb

Fixes jsdom#1559. Closes jsdom#1629.

Implement Keyboard/MouseEvent#getModifierState

8d6e0e3

Set view attribute in HTMLElement#click

66b229c

Enable currently passing uievents WPTs

8e80548

Implement WebSocket

fc27418

TimothyGu force-pushed the websocket branch from 6be445e to fc27418 Compare January 16, 2018 06:24

domenic merged commit 144b39b into jsdom:master Jan 16, 2018

TimothyGu deleted the websocket branch January 16, 2018 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WebSocket API #2088

Implement WebSocket API #2088

TimothyGu commented Dec 20, 2017

TimothyGu Dec 24, 2017

lpinca Dec 27, 2017 •

edited

TimothyGu Dec 29, 2017

lpinca Dec 29, 2017

TimothyGu Dec 24, 2017

TimothyGu Dec 24, 2017

TimothyGu Dec 24, 2017

Zirro Dec 28, 2017 •

edited

TimothyGu Dec 29, 2017

domenic commented Dec 24, 2017

domenic commented Dec 24, 2017

TimothyGu commented Dec 26, 2017 •

edited

rgbkrk commented Dec 26, 2017

lpinca Dec 28, 2017

Zirro left a comment

Zirro Dec 28, 2017 •

edited

TimothyGu Dec 28, 2017

Zirro Jan 12, 2018

Zirro Dec 28, 2017 •

edited

Zirro Dec 28, 2017 •

edited

TimothyGu Dec 28, 2017

TimothyGu commented Dec 29, 2017 •

edited

TimothyGu commented Dec 29, 2017

lpinca commented Dec 29, 2017 •

edited

TimothyGu commented Jan 8, 2018

domenic Jan 16, 2018

TimothyGu Jan 16, 2018

domenic Jan 16, 2018

domenic Jan 16, 2018

domenic Jan 16, 2018

TimothyGu commented Jan 16, 2018


		DIR: websockets

		Create-Secure-extensions-empty.htm: [fail, Buggy test as the test does not take into account the mandatory permessage-deflate extension]

Implement WebSocket API #2088

Implement WebSocket API #2088

Conversation

TimothyGu commented Dec 20, 2017

Choose a reason for hiding this comment

lpinca Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro Dec 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Dec 24, 2017

domenic commented Dec 24, 2017

TimothyGu commented Dec 26, 2017 • edited

rgbkrk commented Dec 26, 2017

Choose a reason for hiding this comment

Zirro left a comment

Choose a reason for hiding this comment

Zirro Dec 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro Dec 28, 2017 • edited

Choose a reason for hiding this comment

Zirro Dec 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Dec 29, 2017 • edited

TimothyGu commented Dec 29, 2017

lpinca commented Dec 29, 2017 • edited

TimothyGu commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jan 16, 2018

lpinca Dec 27, 2017 •

edited

Zirro Dec 28, 2017 •

edited

TimothyGu commented Dec 26, 2017 •

edited

Zirro Dec 28, 2017 •

edited

Zirro Dec 28, 2017 •

edited

Zirro Dec 28, 2017 •

edited

TimothyGu commented Dec 29, 2017 •

edited

lpinca commented Dec 29, 2017 •

edited