Fix MessagePort.postMessage to create clones in correct Realm. #2277
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The target realm to which a message gets delivered is unknown until the task posted to the port message queue runs. So make sure the MessageEvent, and actual structured clone of the data is done inside that task. This requires structured cloning twice, similar to how BroadcastChannel.postMessage does two clones of the data.
|
I'll try to review this soon. I'm a bit worried about whether the warnings at https://html.spec.whatwg.org/#performing-structured-clones-from-other-specifications apply, but maybe they do not when cloning objects produced by the structured-clone algorithm... |
|
If such concerns apply, the presumably already apply to the similar logic in the BroadcastChannel.postMessage algorithm? But yeah, using a "user agent defined Realm" does feel a bit icky, and possibly that part needs to be worked out more? |
domenic
approved these changes
Jan 20, 2017
There was a problem hiding this comment.
This LGTM with one line-wrapping tweak that I pushed. Will you be able to write web platform tests for this? It seems like there are a couple interesting things to test:
- Ensure that errors are rethrown trying to clone something bad (e.g. an object with a throwing getter), even if there is no targetPort
- Ensure that errors are rethrown trying to clone something bad, even if doomed is true
- Ensure that in the case of transferring the targetPort to a different Realm, the end result in the
dataattribute ends up being created in the right Realm.
domenic
added
the
needs tests
annevk
requested changes
Jan 21, 2017
| @@ -95326,6 +95311,25 @@ interface <dfn>MessagePort</dfn> : <span>EventTarget</span> { | |||
| <li><p>Let <var>target</var> be the <code>MessagePort</code> in whose <span>port message | |||
| queue</span> the event <var>e</var> now finds itself.</p></li> | |||
There was a problem hiding this comment.
This e is no longer defined now. I tried to do this patch once before and this is where I got stuck.
annevk
added
the
do not merge yet
|
Another problem with this algorithm is what we should do if cloning fails. (E.g., you get a RangeError when allocating an ArrayBuffer.) |
domenic
added a commit
that referenced
this pull request
Mar 20, 2017
This rewrites most of the cloneable and transferable object infrastructure to better reflect the reality that structured cloning requires separate serialization and deserialization steps, instead of a single operation that creates a new object in the target Realm. This is most evident in the case of MessagePorts, as noted in #2277. It also allows us to avoid awkward double-cloning with an intermediate "user-agent defined Realm", as seen in e.g. history.state or IndexedB; instead we can simply store the serialized form and later deserialize. Concretely, this: * Replaces the concept of cloneable objects with serializable objects. For platform objects, instead of defining a [[Clone]]() internal method, serializable platform objects are annotated with the new [Serializable] IDL attribute, and include serialization and deserialization steps in their definition. * Updates the concept of transferable objects. For platform objects, instead of defining a [[Transfer]]() internal method, transferable platform objects are annotated with the new [Transferable] IDL attribute, and include transfer and transfer-receiving steps. Additionally, the [[Detached]] internal slot for such objects is now managed more automatically. * Removes the StructuredClone() abstract operation in favor of separate StructuredSerialize() and StructuredDeserialize() abstract operations. In practice we found that performing a structured clone alone is never necessary in specs. It is always either coupled with a transfer list, for which StructuredCloneWithTransfer() can be used, or it is best expressed as separate serialization and deserialization steps. * Removes IsTransferable() and Transfer() abstract operations. When defined more properly, these became less useful by themselves, so they were inlined into the rest of the machinery. * Introduces StructuredSerialzieWithTransfer() and StructuredDeserializeWithTransfer(), which can be used by other specifications which need to define their own postMessage()-style algorithm but for which StructuredCloneWithTransfer() is not sufficient. Closes #785. Closes #935. Closes #2277. Closes #1162. Sets the stage for #936 and #2260/#2361.
alice
pushed a commit
to alice/html
that referenced
this pull request
Jan 8, 2019
This rewrites most of the cloneable and transferable object infrastructure to better reflect the reality that structured cloning requires separate serialization and deserialization steps, instead of a single operation that creates a new object in the target Realm. This is most evident in the case of MessagePorts, as noted in whatwg#2277. It also allows us to avoid awkward double-cloning with an intermediate "user-agent defined Realm", as seen in e.g. history.state or IndexedB; instead we can simply store the serialized form and later deserialize. Concretely, this: * Replaces the concept of cloneable objects with serializable objects. For platform objects, instead of defining a [[Clone]]() internal method, serializable platform objects are annotated with the new [Serializable] IDL attribute, and include serialization and deserialization steps in their definition. * Updates the concept of transferable objects. For platform objects, instead of defining a [[Transfer]]() internal method, transferable platform objects are annotated with the new [Transferable] IDL attribute, and include transfer and transfer-receiving steps. Additionally, the [[Detached]] internal slot for such objects is now managed more automatically. * Removes the StructuredClone() abstract operation in favor of separate StructuredSerialize() and StructuredDeserialize() abstract operations. In practice we found that performing a structured clone alone is never necessary in specs. It is always either coupled with a transfer list, for which StructuredCloneWithTransfer() can be used, or it is best expressed as separate serialization and deserialization steps. * Removes IsTransferable() and Transfer() abstract operations. When defined more properly, these became less useful by themselves, so they were inlined into the rest of the machinery. * Introduces StructuredSerialzieWithTransfer() and StructuredDeserializeWithTransfer(), which can be used by other specifications which need to define their own postMessage()-style algorithm but for which StructuredCloneWithTransfer() is not sufficient. Closes whatwg#785. Closes whatwg#935. Closes whatwg#2277. Closes whatwg#1162. Sets the stage for whatwg#936 and whatwg#2260/whatwg#2361.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The target realm to which a message gets delivered is unknown until the task
posted to the port message queue runs. So make sure the MessageEvent, and
actual structured clone of the data is done inside that task. This requires
structured cloning twice, similar to how BroadcastChannel.postMessage does
two clones of the data.