Several bug fixes related to MessageChannel/MessagePort #21540

TimothyGu · 2018-06-26T04:47:45Z

Currently, transferring the port on which postMessage is called causes a
segmentation fault, and transferring the target port causes a subsequent
port.onmessage setting to throw, or a deadlock if onmessage is set
before the postMessage. Fix both of these behaviors and align the
methods more closely with the normative definitions in the HTML
Standard.

Also, per spec postMessage must not throw just because the ports are
disentangled. Implement that behavior.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-06-26T04:47:46Z

@TimothyGu build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/163/pipeline

addaleax

Thank you a lot! :)

addaleax · 2018-06-26T09:30:01Z

src/node_messaging.cc

+    env_->isolate()->ThrowException(
+        env_->domexception_function()
+            ->NewInstance(env_->context(), arraysize(argv), argv)
+            .ToLocalChecked());


Using .ToLocalChecked() here is going to be problematic in the presence of worker.terminate(). It should be fine to just return if NewInstance() returns an empty handle

Also, maybe add a CHECK(!env_->domexception_function().IsEmpty());?

addaleax · 2018-06-26T09:32:34Z

src/node_messaging.cc

+                                     Local<Value> transfer_v) {
+  auto isolate = env->isolate();
+  auto obj = object(isolate);
+  auto context = obj->CreationContext();


Can you avoid auto for all of these?

But… auto is too exciting!

For reals though, I can change it of course, but I personally thought the variable names were pretty self explanatory in terms of types.

They are, except maybe in the case of obj … but I’d personally really prefer to keep auto for cases where the exact type is hard to spell out/unknown – obviously, other people will have different experiences, but for me using auto usually makes writing the code easier and reading it harder, even in these “trvial” cases…

I think mostly this code is read a lot more than it is written - while I use auto in my own code in Node.js I had zero (well ~2) C++ commits but I've reviewed at least a hundred PRs. So 👍 on spelling up the types.

addaleax · 2018-06-26T09:33:44Z

src/node_messaging.cc

+
+  // Check if the target port is posted to itself.
+  if (data_->sibling_ != nullptr) {
+    for (const auto& port : transferred_ports) {


Same here, using MessagePort* port : transferred_ports is cleaner imo :)

addaleax · 2018-06-26T09:35:18Z

src/node_messaging.cc

+          env->isolate()->ThrowException(
+              env->domexception_function()
+                  ->NewInstance(context, arraysize(argv), argv)
+                  .ToLocalChecked());


Same here for .ToLocalChecked()

addaleax · 2018-06-26T09:38:44Z

src/node_messaging.cc

+  // is closed or detached.
+  if (data_ == nullptr) {
+    USE(msg.Serialize(env, context, message_v, transfer_v));
+    return Just(true);


If we want to fulfill the Maybe<T> contract for this function, we should return an empty Maybe if msg.Serialize() returns an empty Maybe, right?

addaleax · 2018-06-26T09:42:18Z

src/node_messaging.cc

+        transferred_ports.push_back(port);
+      }
+    }
+  }


Can we integrate this with the loop in Message::Serialize? There’s a lot of common code, and I’m not sure we really need to unpack the list twice?

I actually had that approach initially, but then I realized that I would need to access MessagePort and/or MessagePortData internals from Message. I could of course add a friend but I decided that object serialization should be independent of the channel communication. Also, aligning with the HTML Standard (whose algorithm is written in this style) would be good.

I realize that it’s tricky, but I’d really like to merge these … if you want, I can try to figure out something myself?

I think it we can make it work if we added a MessagePort* source argument to Serialize() that we check against, and add a getter for Message::message_ports_ that we check after serialization?

Also, at least Firefox and Chrome invoke getters for transferList only once, as an extra data point

@addaleax

I think it we can make it work if we added a MessagePort* source argument to Serialize() that we check against, and add a getter for Message::message_ports_ that we check after serialization?

Sure, that would work. However, checking things after serialization would mean ArrayBuffers have already been neutered, etc.

Also, at least Firefox and Chrome invoke getters for transferList only once, as an extra data point

Yeah, browsers usually have an IDL conversion layer, and use native data types for all internal operations. We could do the same and convert transferList to a std::vector.

I think it we can make it work if we added a MessagePort* source argument to Serialize() that we check against, and add a getter for Message::message_ports_ that we check after serialization?

Sure, that would work. However, checking things after serialization would mean ArrayBuffers have already been neutered, etc.

To clarify, the first check (for source) could still happen before the actual serialization, while still unpacking the transfer list. The second check would happen afterwards, but that’s also what this PR currently does (and intentionally, if I read the added test-worker-message-port-transfer-target.js correctly?)

benjamingr · 2018-06-26T13:32:11Z

lib/internal/dom-exception.js

+    super();
+    this[kMessage] = `${message}`;
+    this[kName] = `${name}`;
+    Error.captureStackTrace(this, DOMException);


Is this required for stuff that extends Error?

Let me double-check.

benjamingr · 2018-06-26T13:34:51Z

src/node_messaging.cc

+    uint32_t length = transfer_list->Length();
+    for (uint32_t i = 0; i < length; ++i) {
+      Local<Value> entry;
+      if (!transfer_list->Get(context, i).ToLocal(&entry))


Can you add a comment for why this is needed?

Hm, what do you mean? This is how to access array elements through the V8 API.

addaleax · 2018-06-26T17:18:31Z

src/node_messaging.cc

-          .IsNothing()) {
-    return;
+
+  // Per spec, we need to serialize the input message even if the MessagePort


The MessagePort/MessageChannel API is pretty close to the Messaging part of the HTML spec, and not related to Workers per se.

At least I wrote it with that spec in mind and using it as a reference document.

While workers are a different matter, the channel messaging API is in my opinion simple enough so that almost* full compatibility with HTML is a worthwhile goal.

* EventEmitter notwithstanding.

benjamingr · 2018-06-26T13:37:51Z

lib/internal/dom-exception.js

@@ -0,0 +1,68 @@
+'use strict';


Why are we calling these DOM exceptions?

WebWorkers (assuming that's the behavior we want to mimic) says:

The term DOM is used to refer to the API set made available to scripts in Web applications, and does not necessarily imply the existence of an actual Document object or of any other Node objects as defined in the DOM Core specifications. [DOM]

I would prefer a different name?

The spec for MessagePort calls it DOMException. The interface is actually specified in Web IDL, which is a spec we want to follow as well. So the name isn't up to us to change.

To clarify, we could of course use a different name, but then it would make us deviate from browsers in an area that I don't think we need to.

+1 to using DOMException. -1 to dash in filename

@devsnek Fair enough.

benjamingr · 2018-06-26T13:39:12Z

test/parallel/test-worker-message-port-transfer-self.js

+
+assert.throws(common.mustCall(() => {
+  port1.postMessage(null, [port1]);
+}), common.mustCall((err) => {


Can this be an expectsError?

Per spec, message is an enumerable getter on the DOMException prototype, rather than a non-enumerable own property on the DOMException object, which expectsError expects:

node/test/common/index.js

Lines 704 to 706 in a9d9d76

const descriptor = Object.getOwnPropertyDescriptor(error, 'message');

assert.strictEqual(descriptor.enumerable,

false, 'The error message should be non-enumerable');

benjamingr · 2018-06-26T13:39:47Z

Good work and nice behaviour change cc @nodejs/workers

TimothyGu · 2018-06-26T22:10:27Z

@addaleax @benjamingr PTAL.

addaleax

Thanks, nice work!

addaleax · 2018-06-26T22:28:44Z

src/node_messaging.cc

+  Local<Object> obj = object(isolate);
+  Local<Context> context = obj->CreationContext();
+
+  std::vector<MessagePort*> transferred_ports;


Is this unused now?

Yes, indeed!

TimothyGu · 2018-06-26T22:51:46Z

@addaleax In case you didn't see: I pushed another commit (1cb3f13bfa071f4a80a41841eb067a67e07d7984) fixing some more issues around the observable behaviors after port.close() has been called, but uv_close() not yet finished (so the MessagePort still has an intact MessagePortData pointer.)

jasnell · 2018-06-29T19:16:13Z

lib/internal/domexception.js

+
+const nameToCodeMap = new Map();
+
+class DOMException extends Error {


Should there be anything added to the documentation about this new error type?

Not sure, but I'm inclined to say no, as it's pretty different from rest of errors.md, and DOMException is not exposed to userland by default.

TimothyGu · 2018-07-03T21:12:59Z

Ping @addaleax. Would be great if you could take a look after #21540 (comment).

addaleax

Still LGTM :)

Currently, transferring the port on which postMessage is called causes a segmentation fault, and transferring the target port causes a subsequent port.onmessage setting to throw, or a deadlock if onmessage is set before the postMessage. Fix both of these behaviors and align the methods more closely with the normative definitions in the HTML Standard. Also, per spec postMessage must not throw just because the ports are disentangled. Implement that behavior.

TimothyGu · 2018-07-04T01:29:23Z

Rebased.

New CI: https://ci.nodejs.org/job/node-test-pull-request/15720/

TimothyGu · 2018-07-04T02:54:38Z

Landed in 602da64...f374d6a.

PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently, transferring the port on which postMessage is called causes a segmentation fault, and transferring the target port causes a subsequent port.onmessage setting to throw, or a deadlock if onmessage is set before the postMessage. Fix both of these behaviors and align the methods more closely with the normative definitions in the HTML Standard. Also, per spec postMessage must not throw just because the ports are disentangled. Implement that behavior. PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2018-07-04T05:14:17Z

Looks like a subsystem was made up for this one. ಠ_ಠ

This probably should have been lib,src: and not messaging:.

(Tooling flags messaging: as a problem. Are you landing commits manually? Get with the node-core-utils awesomeness!)

PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently, transferring the port on which postMessage is called causes a segmentation fault, and transferring the target port causes a subsequent port.onmessage setting to throw, or a deadlock if onmessage is set before the postMessage. Fix both of these behaviors and align the methods more closely with the normative definitions in the HTML Standard. Also, per spec postMessage must not throw just because the ports are disentangled. Implement that behavior. PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 26, 2018

TimothyGu requested a review from addaleax June 26, 2018 04:47

TimothyGu added the worker Issues and PRs related to Worker support. label Jun 26, 2018

addaleax reviewed Jun 26, 2018

View reviewed changes

benjamingr reviewed Jun 26, 2018

View reviewed changes

addaleax approved these changes Jun 26, 2018

View reviewed changes

jasnell reviewed Jun 29, 2018

View reviewed changes

jasnell approved these changes Jun 29, 2018

View reviewed changes

addaleax approved these changes Jul 3, 2018

View reviewed changes

TimothyGu added 2 commits July 3, 2018 21:26

messaging: use actual DOMException for DataCloneError

3151390

TimothyGu force-pushed the messaging-bug-fixes branch from b711b8d to 46701ab Compare July 4, 2018 01:27

TimothyGu closed this Jul 4, 2018

TimothyGu deleted the messaging-bug-fixes branch July 4, 2018 02:54

TimothyGu added a commit that referenced this pull request Jul 4, 2018

messaging: use actual DOMException for DataCloneError

5f3bdb0

PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Jul 4, 2018

messaging: use actual DOMException for DataCloneError

221c8bd

PR-URL: #21540 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

	const descriptor = Object.getOwnPropertyDescriptor(error, 'message');
	assert.strictEqual(descriptor.enumerable,
	false, 'The error message should be non-enumerable');


		const nameToCodeMap = new Map();

		class DOMException extends Error {

Several bug fixes related to MessageChannel/MessagePort #21540

Several bug fixes related to MessageChannel/MessagePort #21540

Conversation

TimothyGu commented Jun 26, 2018

Checklist

nodejs-github-bot commented Jun 26, 2018

addaleax left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Jun 26, 2018 • edited Loading

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

benjamingr commented Jun 26, 2018

TimothyGu commented Jun 26, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jul 3, 2018 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

TimothyGu commented Jul 4, 2018

TimothyGu commented Jul 4, 2018

Trott commented Jul 4, 2018

TimothyGu Jun 26, 2018 •

edited

Loading

TimothyGu commented Jul 3, 2018 •

edited

Loading