Re: [whatwg/streams] Support transferable streams (postMessage) (#1053)

@domenic commented on this pull request.

Mostly-editorial feedback. It's great to see this coming together!

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">

I'd prefer to put these in a new section at the bottom, like asynchronous iteration.

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let

Can we avoid using incumbent in new code? Probably the relevant settings object of |value| would be ideal.

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let

Let's phrase this as "Let _port1_ be the result of creating a new MessagePortObject whose..."

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let

I will write tests to see if what the spec actually does for MessageChannel...

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port1| be that object.
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port2| be that object.
+ 1. [=Entangle=] the |port1| and |port2| objects.
+ 1. If ! [$IsReadableStreamLocked$](|value|) is true, throw a {{TypeError}} exception.

Should we do this earlier to avoid unnecessary work?

> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port1| be that object.
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port2| be that object.
+ 1. [=Entangle=] the |port1| and |port2| objects.
+ 1. If ! [$IsReadableStreamLocked$](|value|) is true, throw a {{TypeError}} exception.
+ 1. Let |writable| be a [=new=] {{WritableStream}}.
+ 1. Perform ! [$SetUpCrossRealmTransformWritable$](|writable|, |port1|).
+ 1. Let |promise| be ! [$ReadableStreamPipeTo$](|value|, |writable|, false, false, false, undefined).

No need to pass undefined, it seems.

> +<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port1| be that object.
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port2| be that object.
+ 1. [=Entangle=] the |port1| and |port2| objects.
+ 1. If ! [$IsReadableStreamLocked$](|value|) is true, throw a {{TypeError}} exception.
+ 1. Let |writable| be a [=new=] {{WritableStream}}.
+ 1. Perform ! [$SetUpCrossRealmTransformWritable$](|writable|, |port1|).
+ 1. Let |promise| be ! [$ReadableStreamPipeTo$](|value|, |writable|, false, false, false, undefined).
+ 1. Set |promise|.\[[PromiseIsHandled]] to true.
+ 1. Let |messagePortDataHolder| be { \[[Type]]: `"MessagePort"` }.
+ 1. Perform {{MessagePort}}'s transfer steps <!-- TODO(ricea): Link this somehow --> with |port2|

I think it would work to do

1. Set _dataHolder_.[[port]] [StructuredSerializeWithTransfer](https://html.spec.whatwg.org/#structuredserializewithtransfer)(undefined, « |port2| »).[[TransferDataHolders]][0].

(or some expanded version of that for clarity)

> + 1. Let |writable| be a [=new=] {{WritableStream}}.
+ 1. Perform ! [$SetUpCrossRealmTransformWritable$](|writable|, |port1|).
+ 1. Let |promise| be ! [$ReadableStreamPipeTo$](|value|, |writable|, false, false, false, undefined).
+ 1. Set |promise|.\[[PromiseIsHandled]] to true.
+ 1. Let |messagePortDataHolder| be { \[[Type]]: `"MessagePort"` }.
+ 1. Perform {{MessagePort}}'s transfer steps <!-- TODO(ricea): Link this somehow --> with |port2|
+    and |messagePortDataHolder|.
+ 1. Set |dataHolder|.\[[port]] to |messagePortDataHolder|.
+
+</div>
+
+<div algorithm="ReadableStream transfer receiving steps">
+Their [=transfer-receiving steps=], given |dataHolder| and |value|, are:
+
+ 1. Let |port| be a new {{MessagePort}}.
+ 1. Perform {{MessagePort}}'s transfer-receiving steps with |dataHolder|.\[[port]] and |port|.

Similarly these seems best done with a call to https://html.spec.whatwg.org/#structureddeserializewithtransfer


> @@ -745,6 +755,36 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
   immutable, this could allow interference between the two branches.
 </dl>
 
+<div algorithm="ReadableStream transfer steps">
+{{ReadableStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value| and
+|dataHolder|, are:
+
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port1| be that object.
+ 1. [=Create a new MessagePort object=] whose [=owner=] is the [=incumbent settings object=] and let
+    |port2| be that object.
+ 1. [=Entangle=] the |port1| and |port2| objects.
+ 1. If ! [$IsReadableStreamLocked$](|value|) is true, throw a {{TypeError}} exception.

I wonder if "DataCloneError" DOMException would be more idiomatic.

> @@ -4894,6 +4972,37 @@ side=], or to terminate or error the stream.
   <p>Returns a {{WritableStream}} representing the [=writable side=] of this transform stream.
 </dl>
 
+<div algorithm="TransformStream transfer steps">
+{{TransformStream}} objects are [=transferable objects=]. Their [=transfer steps=], given |value|
+and |dataHolder|, are:
+
+<!-- Find a way to linkify ReadableStream transfer steps and WritableStream transfer steps -->
+
+ 1. If ! [$IsReadableStreamLocked$](|value|.\[[readable]]) is true, throw a {{TypeError}} exception.
+ 1. If ! [$IsWritableStreamLocked$](|value|.\[[writable]]) is true, throw a {{TypeError}} exception.
+ 1. Let |readableDataHolder| be { \[[Type]]: `"ReadableStream"` }.

Is the [[Type]] tagging used anywhere? I have this question generally, although in this case it seems particularly clear that it's not used.

> + 1. Perform {{WritableStream}} transfer steps with |value|.\[[writable]] and |writableDataHolder|.
+ 1. Set |dataHolder|.\[[writable]] to |writableDataHolder|.
+
+</div>
+
+<div algorithm="TransformStream transfer receiving steps">
+Their [=transfer-receiving steps=], given |dataHolder| and |value|, are:
+
+ 1. Perform {{ReadableStream}} transfer-receiving steps given |dataHolder|.\[[readable]] and
+    |value|.\[[readable]].
+ 1. Perform {{WritableStream}} transfer-receiving steps given |dataHolder|.\[[writable]] and
+    |value|.\[[writable]].
+
+<p class="note">These steps intentionally leave the \[[backpressure]],
+\[[backpressureChangePromise]] and \[[transformStreamController]] slots uninitialized. They are not
+used in a transferred TransformStream.</p>

I'm not sure that we have a well-defined concept of "uninitialized". Every other path in the spec sets all internal slots on new objects, I believe. So I think it'd be better to explicitly set them to undefined.

> @@ -15,6 +15,10 @@ Markup Shorthands: markdown yes
 spec:webidl; type:dfn; text:resolve
 spec:webidl; type:dfn; text:new
 spec:infra; type:dfn; text:list
+spec:html; type:dfn; text:entangle

I'm surprised these are needed; there are really other specs that define "create a new MessagePort object" and friends?

Well, hopefully we can eliminate these, even if it requires some HTML edits. We'll see.

> @@ -5740,6 +5849,146 @@ for="value-with-size">value</dfn> and <dfn for="value-with-size">size</dfn>.
  1. Set |container|.\[[queueTotalSize]] to 0.
 </div>
 
+<h3 id="transferrable-streams">Transferable streams</h3>
+
+Transferable streams are implemented using a special kind of identity transform which has the
+[=writable side=] in one realm and the [=readable side=] in another realm. The following abstract
+operations are used to implement these "cross-realm transforms".
+
+<div algorithm>
+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).
+ 1. Perform [$CreateDataProperty$](|message|, `"type"`, |type|).

```suggestion
 1. Perform [$CreateDataProperty$](|message|, "`type`", |type|).
```

and elsewhere

> @@ -5740,6 +5849,146 @@ for="value-with-size">value</dfn> and <dfn for="value-with-size">size</dfn>.
  1. Set |container|.\[[queueTotalSize]] to 0.
 </div>
 
+<h3 id="transferrable-streams">Transferable streams</h3>
+
+Transferable streams are implemented using a special kind of identity transform which has the
+[=writable side=] in one realm and the [=readable side=] in another realm. The following abstract
+operations are used to implement these "cross-realm transforms".
+
+<div algorithm>
+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).
+ 1. Perform [$CreateDataProperty$](|message|, `"type"`, |type|).
+ 1. Perform [$CreateDataProperty$](|message|, `"value"`, |value|).
+ 1. Let |targetPort| be the port with which |port| is entangled, if any; otherwise let it be null.
+ 1. Let |options| be «[ `"transfer"` → « » ]».

It'd be ideal if HTML made this linkable; TODO for me.

> +Transferable streams are implemented using a special kind of identity transform which has the
+[=writable side=] in one realm and the [=readable side=] in another realm. The following abstract
+operations are used to implement these "cross-realm transforms".
+
+<div algorithm>
+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).
+ 1. Perform [$CreateDataProperty$](|message|, `"type"`, |type|).
+ 1. Perform [$CreateDataProperty$](|message|, `"value"`, |value|).
+ 1. Let |targetPort| be the port with which |port| is entangled, if any; otherwise let it be null.
+ 1. Let |options| be «[ `"transfer"` → « » ]».
+ 1. Run the [=message port post message steps=] providing |targetPort|, |message| and |options|.
+
+ <p class="note">A JavaScript object is used for transfer to avoid having to duplicate the [=message

Hmm. Although I don't think the duplication would be that bad, I think a JS object would still have to be involved, so I think this is the best we can do, indeed.

> +operations are used to implement these "cross-realm transforms".
+
+<div algorithm>
+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).
+ 1. Perform [$CreateDataProperty$](|message|, `"type"`, |type|).
+ 1. Perform [$CreateDataProperty$](|message|, `"value"`, |value|).
+ 1. Let |targetPort| be the port with which |port| is entangled, if any; otherwise let it be null.
+ 1. Let |options| be «[ `"transfer"` → « » ]».
+ 1. Run the [=message port post message steps=] providing |targetPort|, |message| and |options|.
+
+ <p class="note">A JavaScript object is used for transfer to avoid having to duplicate the [=message
+ port post message steps=]. The prototype of the object is set to null to avoid interference from
+ the global object.</p>

"interference from `Object.prototype`" is probably more accurate.

> + 1. Let |result| be [$PackAndPostMessage$](|port|, |type|, |value|).
+ 1. If |result| is an abrupt completion,
+  1. Perform ! [$CrossRealmTransformSendError$](|port|, |result|.\[[Value]]).
+  1. Return [=a promise rejected with=] |result|.\[[Value]].
+ 1. Otherwise, return [=a promise resolved with=] undefined.
+
+</div>
+
+<div algorithm>
+ <dfn abstract-op lt="CrossRealmTransformSendError">CrossRealmTransformSendError(|port|,
+ |error|)</dfn> performs the following steps:
+
+ 1. Perform [$PackAndPostMessage$](|port|, `"error"`, |error|), discarding the result.
+
+ <p class="note">As we are already in an errored state when this abstract operation is performed, we
+ cannot handle further errors, so we just discard them.</p>

Should we set [[PromiseIsHandled]] to true?

> @@ -5740,6 +5849,146 @@ for="value-with-size">value</dfn> and <dfn for="value-with-size">size</dfn>.
  1. Set |container|.\[[queueTotalSize]] to 0.
 </div>
 
+<h3 id="transferrable-streams">Transferable streams</h3>
+
+Transferable streams are implemented using a special kind of identity transform which has the
+[=writable side=] in one realm and the [=readable side=] in another realm. The following abstract
+operations are used to implement these "cross-realm transforms".
+
+<div algorithm>
+ <dfn abstract-op lt="PackAndPostMessage">PackAndPostMessage(|port|, |type|, |value|)</dfn> performs
+ the following steps:
+
+ 1. Let |message| be [$OrdinaryObjectCreate$](null).

Put `!`s before all the infallible abstract ops. Generally every abstract op call that you're not going to explicitly handle needs a ! or ?.

> +
+ 1. Perform [$PackAndPostMessage$](|port|, `"error"`, |error|), discarding the result.
+
+ <p class="note">As we are already in an errored state when this abstract operation is performed, we
+ cannot handle further errors, so we just discard them.</p>
+
+</div>
+
+<div algorithm>
+ <dfn abstract-op lt="SetUpCrossRealmTransformReadable">SetUpCrossRealmTransformReadable(|stream|,
+ |port|)</dfn> performs the following steps:
+
+ 1. Perform ! [$InitializeReadableStream$](|stream|).
+ 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:

"Add a handler" is not well-defined. I've run into this problem before; you need something like https://wicg.github.io/kv-storage/#add-a-simple-event-listener. Maybe we can upstream that into DOM.

> +
+ <p class="note">As we are already in an errored state when this abstract operation is performed, we
+ cannot handle further errors, so we just discard them.</p>
+
+</div>
+
+<div algorithm>
+ <dfn abstract-op lt="SetUpCrossRealmTransformReadable">SetUpCrossRealmTransformReadable(|stream|,
+ |port|)</dfn> performs the following steps:
+
+ 1. Perform ! [$InitializeReadableStream$](|stream|).
+ 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.

How could these bad cases happen?

> + <p class="note">As we are already in an errored state when this abstract operation is performed, we
+ cannot handle further errors, so we just discard them.</p>
+
+</div>
+
+<div algorithm>
+ <dfn abstract-op lt="SetUpCrossRealmTransformReadable">SetUpCrossRealmTransformReadable(|stream|,
+ |port|)</dfn> performs the following steps:
+
+ 1. Perform ! [$InitializeReadableStream$](|stream|).
+ 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.
+  1. Let |type| be ? [$Get$](|data|, `"type"`).

Should exceptions in these cases error the stream, or be thrown? If they're thrown, I guess they'll be handled like any event listener error, and go to `window.onerror`? Do these cases have tests?

> + 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.
+  1. Let |type| be ? [$Get$](|data|, `"type"`).
+  1. Let |value| be ? [$Get$](|data|, `"value"`).
+  1. If ! [$Type$](|type|) is not String, return.
+  1. If |type| is `"chunk"`,
+   1. If ! [$ReadableStreamDefaultControllerCanCloseOrEnqueue$](|controller|) is true,
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](|controller|, |value|).
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to [=a new promise=].
+  1. Otherwise, if |type| is `"close"`,
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](|controller|).
+    1. Disentangle |port|.

Wow, it's weird that this is not a defined term.

> + 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.
+  1. Let |type| be ? [$Get$](|data|, `"type"`).
+  1. Let |value| be ? [$Get$](|data|, `"value"`).
+  1. If ! [$Type$](|type|) is not String, return.
+  1. If |type| is `"chunk"`,
+   1. If ! [$ReadableStreamDefaultControllerCanCloseOrEnqueue$](|controller|) is true,
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](|controller|, |value|).
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to [=a new promise=].
+  1. Otherwise, if |type| is `"close"`,
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](|controller|).
+    1. Disentangle |port|.
+  1. Otherwise, if |type| is `"error"`,
+   1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |value|)

```suggestion
   1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |value|).
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1053#pullrequestreview-445883331