New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify structured clone behavior for WebAssembly objects. #1074

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@flagxor
Member

flagxor commented May 23, 2017

PTAL
(Particularly around the MAY for secure origins)

Web.md Outdated
### Structured Clone of a `WebAssembly.Memory`

A `WebAssembly.Memory` is under some circumstances a
[cloneable object](https://html.spec.whatwg.org/multipage/infrastructure.html#cloneable-objects), and will impact the

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

"will impact"? How solid is this prediction? Reference?

This comment has been minimized.

@flagxor

flagxor May 23, 2017

Member

Revised to to say "will require changes to"
We could structure these as changes against said standards?
Domenic / Anne, WDYT?

Web.md Outdated

[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

"has a" slot, no?

This comment has been minimized.

@flagxor
Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be value.[[BufferObject].

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

Brackets mismatch.

This comment has been minimized.

@flagxor
Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be value.[[BufferObject].

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

Code quotes mismatch on value compared to above? Or is that on purpose?

Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be value.[[BufferObject].

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

There's no such slot according to this snippet from above:

A WebAssembly.Module object represents the stateless result of compiling a WebAssembly binary-format module and contains one internal slot: [...]

Web.md Outdated

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be value.[[BufferObject].
2. If ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer) is true, then:

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

What's with the bang? "If it's not shared is true"?

This comment has been minimized.

@annevk

annevk May 23, 2017

Member

The bang means not throwing. It's a convention from the JavaScript Standard.

Web.md Outdated
[[AgentCluster]]: the [current Realm Record](https://tc39.github.io/ecma262/#current-realm)'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) }.
3. Otherwise:
1. Let `dataCopy` be [CreateByteDataBlock](https://tc39.github.io/ecma262/#sec-createbytedatablock)(size).
NOTE: This can throw a RangeError exception on allocation failure.

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

RangeError? Ew. That's standard?

This comment has been minimized.

@annevk

annevk May 23, 2017

Member

Yeah, see the link.

Web.md Outdated
1. If `forStorage` is true, then throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#datacloneerror).
2. Set `serialized` to { [[Type]]: "WebAssembly.Memory",
[[ArrayBufferData]]: value.[[BufferObject]].[[ArrayBufferData],

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

Mismatched bracket.

This comment has been minimized.

@flagxor
Web.md Outdated
value.[[BufferObject]].[[ArrayBufferData]], 0, size).
3. Set `serialized` to { [[Type]]: "WebAssembly.Memory",
[[ArrayBufferData]]: dataCopy, [[ArrayBufferByteLength]]: size }.

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

What if it's the reverse of 2. above with the weird bang? Doesn't seem to have a sequel for the opposite.

This comment has been minimized.

@flagxor

flagxor May 23, 2017

Member

Shuffled round.

Web.md Outdated
@@ -124,6 +124,81 @@ MIME type mismatch or `opaque` response types
[reject](http://tc39.github.io/ecma262/#sec-rejectpromise) the Promise with a
`WebAssembly.CompileError`.

### Structured Clone of a `WebAssembly.Memory`

A `WebAssembly.Memory` is under some circumstances a

This comment has been minimized.

@annevk

annevk May 23, 2017

Member

"Under some circumstances"?

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Right, it's always a serializable object. But sometimes that serialization throws.

@annevk

This comment has been minimized.

Member

annevk commented May 23, 2017

I'm a bit confused. This seems to address #1048 to some extent, yet you didn't address the design questions raised there.

Web.md Outdated
* Otherwise, if `value` has an [[WebAssembly.Module]] internal slot, then:
1. If `forStorage` is true, implementations MAY chose to exclude
"insecure origins", in which case they should throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#datacloneerror).

This comment has been minimized.

@annevk

annevk May 23, 2017

Member

This isn't precise enough. (Needs a source to base "insecure origins" on and the should needs to be a must. It also doesn't seem optimal to have varying behavior between implementations here.

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Yes. This needs to decide which object's origin is being tested, per whatwg/html#2518 (comment), and needs to link to https://w3c.github.io/webappsec-secure-contexts/#settings-object applied to one of the settings objects involved (depending on which one you choose).

This comment has been minimized.

@flagxor

flagxor May 23, 2017

Member

Revised.
JF + others. Are other browsers willing to require the more strict behavior on IDB + secure origins?

Web.md Outdated
@@ -124,6 +124,81 @@ MIME type mismatch or `opaque` response types
[reject](http://tc39.github.io/ecma262/#sec-rejectpromise) the Promise with a
`WebAssembly.CompileError`.

### Structured Clone of a `WebAssembly.Memory`

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

This should be Structured serialization; structured cloning is no longer a thing at the spec level (although it can be used as an informal term outside of specs).

This comment has been minimized.

@flagxor
Web.md Outdated
### Structured Clone of a `WebAssembly.Memory`

A `WebAssembly.Memory` is under some circumstances a
[cloneable object](https://html.spec.whatwg.org/multipage/infrastructure.html#cloneable-objects), and will impact the

This comment has been minimized.

@domenic

This comment has been minimized.

@flagxor
Web.md Outdated
@@ -124,6 +124,81 @@ MIME type mismatch or `opaque` response types
[reject](http://tc39.github.io/ecma262/#sec-rejectpromise) the Promise with a
`WebAssembly.CompileError`.

### Structured Clone of a `WebAssembly.Memory`

A `WebAssembly.Memory` is under some circumstances a

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Right, it's always a serializable object. But sometimes that serialization throws.

Web.md Outdated

[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Nit: generally we use value for variables, not value.

This comment has been minimized.

@flagxor
Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if `value` has an [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be value.[[BufferObject].

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Is size a number or an object?

This comment has been minimized.

@flagxor

flagxor May 23, 2017

Member

number, changed (was the wrong one).

Web.md Outdated
1. Let `size` be value.[[BufferObject].
2. If ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer) is true, then:
1. If `forStorage` is true, then throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#datacloneerror).

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Formatting:

"`[DataCloneError](https://heycam.github.io/webidl/#datacloneerror)" `[DOMException](https://heycam.github.io/webidl/#dfn-DOMException)`

This comment has been minimized.

@flagxor
Web.md Outdated
[[ArrayBufferData]]: value.[[BufferObject]].[[ArrayBufferData],
[[ArrayBufferByteLength]]: size,
[[AgentCluster]]: the [current Realm Record](https://tc39.github.io/ecma262/#current-realm)'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) }.
3. Otherwise:

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

So WebAssembly.Memory objects can be backed by non-shared array buffers!? Wow, interesting.

This comment has been minimized.

@jfbastien

jfbastien May 23, 2017

Member

Yeah memories and modules need to opt in to being shared. Can't instantiate a not-share-aware module with a shared memory.

This comment has been minimized.

@flagxor
Web.md Outdated
* Otherwise, if `value` has an [[WebAssembly.Module]] internal slot, then:
1. If `forStorage` is true, implementations MAY chose to exclude
"insecure origins", in which case they should throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#datacloneerror).

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Yes. This needs to decide which object's origin is being tested, per whatwg/html#2518 (comment), and needs to link to https://w3c.github.io/webappsec-secure-contexts/#settings-object applied to one of the settings objects involved (depending on which one you choose).

Web.md Outdated
whose [[Module]] internal slot value is `serialized`.[[Module]].

The semantics of a structured clone is as-if the binary source, from which the
`WebAssembly.Module` was compiled, were cloned and recompiled into the target realm.

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

"serialized, then deserialized and recompiled the target realm"

This comment has been minimized.

@flagxor
Web.md Outdated
The semantics of a structured clone is as-if the binary source, from which the
`WebAssembly.Module` was compiled, were cloned and recompiled into the target realm.
Engines should attempt to share/reuse internal compiled code when performing
a structured clone although, in corner cases like CPU upgrade or browser

This comment has been minimized.

@domenic

domenic May 23, 2017

Member

Nit: comma should be maybe "when performing structured serialization, although in corner cases..."

This comment has been minimized.

@flagxor
@domenic

This comment has been minimized.

Member

domenic commented May 23, 2017

This seems to address #1048 to some extent, yet you didn't address the design questions raised there.

Right, it seems to go the route of treating these as non-Web IDL-defined objects. Which is fine for now, I guess, since they aren't defined using Web IDL yet.

@annevk

This comment has been minimized.

Member

annevk commented May 23, 2017

Well, #1068 added some IDL syntax. It would be a lot better to first make a choice there as the current mismatch will just end up being super confusing the moment anyone outside the people closely involved starts looking at this.

@flagxor flagxor force-pushed the flagxor:clone1 branch 2 times, most recently from d0d94d9 to 63ceaaa May 23, 2017

@flagxor

This comment has been minimized.

Member

flagxor commented May 23, 2017

PTAL
JF + others, are you ok with the secure origin requirement around forStorage?

Not sure how to resolve the syntax concern, advice appreciated.

@jfbastien

This comment has been minimized.

Member

jfbastien commented May 23, 2017

Let me ping others in my team about the details.

Web.md Outdated
@@ -124,6 +124,86 @@ MIME type mismatch or `opaque` response types
[reject](http://tc39.github.io/ecma262/#sec-rejectpromise) the Promise with a
`WebAssembly.CompileError`.

### Structured serialization of a `WebAssembly.Memory`

A `WebAssembly.Memory` is under some circumstances a

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Remove "under some circumstances", here and below. It's always a serializable object; sometimes serialization throws.

This comment has been minimized.

@flagxor
Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if *value* has a [[WebAssembly.Memory]] internal slot, then:
1. Let `size` be the number value.[[BufferObject]].[[ArrayBufferByteLength]].

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nit: size, forStorage should all be italicized, not code-ified.

This comment has been minimized.

@flagxor
Web.md Outdated
2. Otherwise:
1. Let *dataCopy* be [CreateByteDataBlock](https://tc39.github.io/ecma262/#sec-createbytedatablock)(size).
NOTE: This can throw a RangeError exception on allocation failure.
2. Perform `CopyDataBlockBytes`(https://tc39.github.io/ecma262/#sec-copydatablockbytes)

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Link, not code, for CopyDataBlockBytes

This comment has been minimized.

@flagxor
Web.md Outdated
1. Let `size` be the number value.[[BufferObject]].[[ArrayBufferByteLength]].
2. If ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer)(*value*) is true, then:
1. If `forStorage` is true, then throw a
"`[DataCloneError](https://heycam.github.io/webidl/#datacloneerror)"

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Missing closing backtick

This comment has been minimized.

@flagxor
Web.md Outdated
* Otherwise, if *serialized*.[[Type]] is "WebAssembly.Memory", then:
1. If *targetRealm*'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) is not
*serialized*.[[AgentCluster]], then throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#dfn-DOMException).

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Use the above formatting for the DOMException.

This comment has been minimized.

@flagxor
Web.md Outdated
1. If *forStorage* is true and the environmental settings
object of *value* is not "Secure" per
[this](https://w3c.github.io/webappsec-secure-contexts/#settings-object)
algorithm, throw a

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

It'd be more conventional to say

per the "Is the environment settings object a secure context?" algorithm

with a link, instead of "this".

This comment has been minimized.

@flagxor
Web.md Outdated
object of *value* is not "Secure" per
[this](https://w3c.github.io/webappsec-secure-contexts/#settings-object)
algorithm, throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#datacloneerror).

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Copy DOMException formatting from above.

This comment has been minimized.

@flagxor
Web.md Outdated
* Otherwise, if *serialized*.[[Type]] is "WebAssembly.Module", then:
1. If *targetRealm*'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) is not
*serialized*.[[AgentCluster]], then throw a
["DataCloneError" DomException](https://heycam.github.io/webidl/#dfn-DOMException).

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Copy DOMException formatting from above.

This comment has been minimized.

@flagxor
Web.md Outdated
whose [[Module]] internal slot value is *serialized*.[[Module]].

The semantics of a structured serialization is as-if the binary source, from which the
`WebAssembly.Module` was compiled, serialized, then deserialized, and recompiled into the target realm.

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Missing word in this sentence? I think it should be "is serialized"

This comment has been minimized.

@flagxor
Web.md Outdated
a structured seraialization, although in corner cases like CPU upgrade or browser
update, this may not be possible and full recompilation may be necessary.

Given the above engine optimizations, structured cloning provides developers

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

structured serialization

This comment has been minimized.

@flagxor

@flagxor flagxor force-pushed the flagxor:clone1 branch from 63ceaaa to 5fa0fbf May 24, 2017

@flagxor

This comment has been minimized.

Member

flagxor commented May 24, 2017

PTAL

@annevk

This comment has been minimized.

Member

annevk commented May 24, 2017

We could structure these as changes against said standards?

Sounds good to me. We usually call these kind of things "monkey patches". It would be good to file an issue upstream (against HTML at least) to make them aware of this. Having said that, this again somewhat depends on your strategy. If you do go with IDL you don't need to change HTML. IDL objects are more extensible than objects that are defined using more primitive means.

@domenic

Mostly nits by now :)

Web.md Outdated
[StructuredSerializeInternal](https://html.spec.whatwg.org/multipage/infrastructure.html#structuredserializeinternal) adds a new step after step 12:

* Otherwise, if *value* has a [[WebAssembly.Memory]] internal slot, then:
1. Let *size* be the number value.[[BufferObject]].[[ArrayBufferByteLength]].

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nit: remove "the number"; italicize value and size everywhere throughout.

This comment has been minimized.

@flagxor
Web.md Outdated
1. Let *size* be the number value.[[BufferObject]].[[ArrayBufferByteLength]].
2. If ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer)(*value*.[[BufferObject]]) is true, then:
1. If *forStorage* is true, then throw a
"`[DataCloneError](https://heycam.github.io/webidl/#datacloneerror)`"

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

It appears I gave bad advice here, viewing the output: https://github.com/flagxor/design/blob/5fa0fbf29437688f86fcff1efeaabcff0770a0bc/Web.md

I think the backticks need to be inside the brackets, e.g.

"[`DataCloneError`](...)"

Here and everywhere.

This comment has been minimized.

@flagxor
Web.md Outdated
1. Let *dataCopy* be [CreateByteDataBlock](https://tc39.github.io/ecma262/#sec-createbytedatablock)(size).
NOTE: This can throw a RangeError exception on allocation failure.
2. Perform [CopyDataBlockBytes](https://tc39.github.io/ecma262/#sec-copydatablockbytes)
(dataCopy, 0,

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nits: no space before the open paren; dataCopy should be italicized here and everywhere

This comment has been minimized.

@flagxor
Web.md Outdated
`[DOMException](https://heycam.github.io/webidl/#dfn-DOMException)`.
2. Otherwise,
if ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer)(*serialized*.[[ArrayBufferData]]) is true, then:
Set *value* to a new WebAssembly.Memory object in *targetRealm*

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nit: just say "then set" instead of "then: Set"

This comment has been minimized.

@flagxor
Web.md Outdated
"`[DataCloneError](https://heycam.github.io/webidl/#datacloneerror)`"
`[DOMException](https://heycam.github.io/webidl/#dfn-DOMException)`.
2. Otherwise,
if ! [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer)(*serialized*.[[ArrayBufferData]]) is true, then:

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

You can't use IsSharedArrayBuffer on a data block (which is the type of [[ArrayBufferData]]). You can only say "is a Shared Data Block"

This comment has been minimized.

@flagxor
Web.md Outdated
2. The `SharedArrayBuffer`'s internal slot [[ArrayBufferByteLength]]
is set to *serialized*.[[BufferObject]].[[ArrayBufferByteLength]].
3. Otherwise,
Set *value* to a new WebAssembly.Memory object in *targetRealm*

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nit: lowercase "set"

This comment has been minimized.

@flagxor
Web.md Outdated
whose [[BufferObject]] internal slot is set to a new
[[ArrayBufferData]] internal slot value is
set to a new `SharedArrayBuffer`, where:
1. The `SharedArrayBuffer`'s internal slot [[ArrayBufferData]]

This comment has been minimized.

@domenic

domenic May 24, 2017

Member

Nit: this should be consistified with the below; this says "The SharedArrayBuffer's internal slot" whereas the below gives a name to it and says "mem's internal slot"

This comment has been minimized.

@flagxor
Web.md Outdated
[[ArrayBufferData]]: value.[[BufferObject]].[[ArrayBufferData]],
[[ArrayBufferByteLength]]: size,
[[AgentCluster]]: the [current Realm Record](https://tc39.github.io/ecma262/#current-realm)'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) }.
2. Otherwise:

This comment has been minimized.

@lukewagner

lukewagner May 26, 2017

Member

I think this "Otherwise:" step is meant to apply to the not-SAB case in which case it should be unindented. Also, the same comment about losing 'maximum' applies. Moreover, looking at the rich preview I think the indenting is messed up in a few other places as well.

This comment has been minimized.

@flagxor

flagxor Jul 16, 2017

Member

Oops. Done.

Web.md Outdated
1. If *targetRealm*'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) is not
*serialized*.[[AgentCluster]], then throw a
"`[DataCloneError](https://heycam.github.io/webidl/#datacloneerror)`"
`[DOMException](https://heycam.github.io/webidl/#dfn-DOMException)`.

This comment has been minimized.

@lukewagner

lukewagner May 26, 2017

Member

If we want to be symmetric with AB/SAB, I think we only need this AgentCluster check for the shared-Memory case.

This comment has been minimized.

@flagxor

flagxor Jul 16, 2017

Member

Ah yes. Done.

Web.md Outdated
Set *value* to a new WebAssembly.Memory object in *targetRealm*
whose [[BufferObject]] internal slot is set to a new
[[ArrayBufferData]] internal slot value is
set to a new `SharedArrayBuffer`, where:

This comment has been minimized.

@lukewagner

lukewagner May 26, 2017

Member

The deserialization of shared and unshared Memory seems to shear off the maximum property.

This comment has been minimized.

@flagxor

flagxor Jul 16, 2017

Member

Good point!. Fixing.

@flagxor flagxor force-pushed the flagxor:clone1 branch 4 times, most recently from a868d19 to 97f0e42 Jul 16, 2017

@flagxor

This comment has been minimized.

Member

flagxor commented Jul 16, 2017

Long delay on this one, sorry about that.

PTAL

@annevk

This comment has been minimized.

Member

annevk commented Jul 17, 2017

So you decided not to go with IDL? That will make it harder once you eventually get to wasm bindings.

Web.md Outdated
1. Let *dataCopy* be [CreateByteDataBlock](https://tc39.github.io/ecma262/#sec-createbytedatablock)(*size*).
NOTE: This can throw a RangeError exception on allocation failure.
2. Perform [CopyDataBlockBytes](https://tc39.github.io/ecma262/#sec-copydatablockbytes)
(*dataCopy*, 0,

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

Nit: there shouldn't be a linebreak before the open paren

This comment has been minimized.

@flagxor
Web.md Outdated
[`DOMException`](https://heycam.github.io/webidl/#dfn-DOMException).
2. Set *value* to a new WebAssembly.Memory object in *targetRealm*
whose [[BufferObject]] internal slot is set to a new
[[ArrayBufferData]] internal slot *value* is

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

It looks like "a new [[ArrayBufferData]] internal slot value is set to a new" should be deleted; this sentence is a bit jumbled.

This comment has been minimized.

@flagxor
Web.md Outdated
[[ArrayBufferData]] internal slot *value* is
set to a new `SharedArrayBuffer` *mem*, where:
1. *mem*'s internal slot [[ArrayBufferData]]
if set to *serialized*.[[BufferObject]].[[ArrayBufferData]].

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

s/if/is

This comment has been minimized.

@flagxor
Web.md Outdated
2. *mem*'s internal slot [[ArrayBufferByteLength]]
is set to *serialized*.[[BufferObject]].[[ArrayBufferByteLength]].
3. *mem*'s internal slot [[WebAssembly.Memory]].[[maximum]]
is set to *serialized*.[[MemoryMaximum]].

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

mem does not have a [[WebAssembly.Memory]] internal slot; value does.

This comment has been minimized.

@flagxor

flagxor Jul 27, 2017

Member

oops, fixed.

Web.md Outdated
*serialized*.[[AgentCluster]], then throw a
"[`DataCloneError`](https://heycam.github.io/webidl/#datacloneerror)"
[`DOMException`](https://heycam.github.io/webidl/#dfn-DOMException).
2. Set *value* to a new WebAssembly.Memory object in *targetRealm*

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

In both branches you need to set value's [[WebAssembly.Memory]] to a new Memory.memory object before you can set properties on [[WebAssembly.Memory]].

It seems like you also need to create some kind of alias between the Memory.memory and the [[ArrayBufferData]]? Like CreateMemoryObject does?

This comment has been minimized.

@flagxor
Web.md Outdated
"[`DataCloneError`](https://heycam.github.io/webidl/#datacloneerror)"
[`DOMException`](https://heycam.github.io/webidl/#dfn-DOMException).
2. Otherwise, set *value* to a new WebAssembly.Module object in *targetRealm*
whose [[Module]] internal slot *value* is *serialized*.[[Module]].

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

s/Module/WebAssembly.Module

This comment has been minimized.

@flagxor
Web.md Outdated
The semantics of a structured serialization is as-if the binary source, from which the
`WebAssembly.Module` was compiled, is serialized, then deserialized, and recompiled into the target realm.
Engines should attempt to share/reuse internal compiled code when performing
a structured seraialization, although in corner cases like CPU upgrade or browser

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

Typo "seraialization"

This comment has been minimized.

@flagxor
Web.md Outdated
[[ArrayBufferByteLength]]: *size*,
[[MemoryMaximum]]: *maximum*,
[[AgentCluster]]: the [current Realm Record](https://tc39.github.io/ecma262/#current-realm)'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) }.
4. Otherwise:

This comment has been minimized.

@lukewagner

lukewagner Jul 18, 2017

Member

Based on today's poll, I think this case would throw (and be asserted not to happen on the deserialize side).

This comment has been minimized.

@binji

binji Jul 18, 2017

Member

I'm curious if this should throw or be a messageerror instead.

This comment has been minimized.

@lukewagner

lukewagner Jul 18, 2017

Member

IIUC, messageerrors only pop out when the error condition is dependent on the recipient; if the error is only depending on the argument's type (and we could consider shared-ness to be part of the memory's type), then I think this would throw just as if you tried to pass a Function.

This comment has been minimized.

@binji

binji Jul 19, 2017

Member

I see, makes sense.

This comment has been minimized.

@flagxor

flagxor Jul 27, 2017

Member

changed to throw in the non-shared case as per our poll.

Web.md Outdated

* Otherwise, if *value* has a [[WebAssembly.Memory]] internal slot, then:
1. Let *size* be *value*.[[BufferObject]].[[ArrayBufferByteLength]].
2. Let *maximum* be *value*.[[WebAssembly.Memory]].[[maximum]].

This comment has been minimized.

@lukewagner

lukewagner Jul 18, 2017

Member

IIUC, [[maximum]] is "reaching into" the memory defined by the core wasm spec. In that case, could you add a link to https://webassembly.github.io/spec/exec/runtime.html#memory-instances, rename to max and note that the value is a u32? (optional u32)?

This comment has been minimized.

@flagxor

flagxor Jul 27, 2017

Member

Reworked to lean on that.

@flagxor flagxor force-pushed the flagxor:clone1 branch 2 times, most recently from 7a9b50d to 10e0424 Jul 27, 2017

@flagxor

This comment has been minimized.

Member

flagxor commented Jul 27, 2017

PTAL

annevk@ Unsure on the whether WebIDL makes more sense for the Web embedding, JS.md likely needs this notation to match up with the ES spec. Will talk more to domenic next week on notation preferences. Mainly trying to narrow what's currently here (as before the PR it just says structured clone).
Do I understand correctly your preference is WebIDL here, or throughout?

@annevk

This comment has been minimized.

Member

annevk commented Jul 27, 2017

@flagxor based on the discussion in #1048 it's my understanding that eventually these objects need to be exposed inside wasm (Wasm?) as well. It's also my understanding that there will be a wasm bindings for Web IDL. It would therefore be much "cheaper" to define these in IDL (and also provide a more consistent API from the eventual wasm side of things).

And as a lesser concern, it matters for how we organize the specifications. If you define these without IDL we'll need to define the serialization/deserialization in HTML, but if you define them with IDL you can define everything in the wasm standard (or wasm API standard or whatever it is).

@domenic

This comment has been minimized.

Member

domenic commented Jul 27, 2017

Although I think it's good to have a discussion about IDL vs. not, I think this PR can proceed without resolving that. This PR does an admiral job of formalizing the semantics of structured serialization, which is sorely needed. It provides a precise specification implementations can use to get interoperable behavior. The spec structuring (IDL vs. not) can be fixed later, without changing any observable behavior.

Now, to continue the review efforts...

@domenic

LGTM, only two nits left. Please feel free to merge after fixing instead of going through another round-trip :).

Web.md Outdated
[StructuredDeserialize](https://html.spec.whatwg.org/multipage/infrastructure.html#structureddeserialize) adds a new step after step 12:

* Otherwise, if *serialized*.[[Type]] is "WebAssembly.Memory", then:
1. Assert [IsSharedArrayBuffer](https://tc39.github.io/ecma262/#sec-issharedarraybuffer)(*serialized*.[[ArrayBufferData]]) is true.

This comment has been minimized.

@domenic

domenic Jul 27, 2017

Member

You can't use IsSharedArrayBuffer on a data block, only on JavaScript objects. Instead, this should be "Assert: serialized.[[ArrayBufferData]] is a Shared Data Block.

This comment has been minimized.

@flagxor
Web.md Outdated
`WebAssembly.Module` was compiled, is serialized, then deserialized, and recompiled into the target realm.
Engines should attempt to share/reuse internal compiled code when performing
a structured serialization, although in corner cases like CPU upgrade or browser
update, this may not be possible and full recompilation may be necessary.

This comment has been minimized.

@domenic

domenic Jul 27, 2017

Member

Nit: s/may/might

This comment has been minimized.

@flagxor
Web.md Outdated
1. *buf*'s internal slot [[ArrayBufferData]]
aliases *mem*'s linear memory.
2. *buf*'s internal slot [[ArrayBufferByteLength]]
is set to the byte length of *mem*'s linear memory.

This comment has been minimized.

@lukewagner

lukewagner Jul 27, 2017

Member

I think there is a spec-level "race" here with concurrent grow_memory: the byte length can be different than when it was tupled on the serialization side. I think the fix is to pull length from the core spec memory instance so it's "fresh". Here (and above, where you have Memory.memory), it'd be good to link to https://webassembly.github.io/spec/exec/runtime.html#memory-instances to say exactly what you're referring to.

There's also a question of whether grow_memory invalidates the tupled [[ArrayBufferData]]. Actually, I'm not clear what the relationship is to the data : vec(byte) field of meminst and [[ArrayBufferData]]. I think there's not a real question here; it's just a matter of getting the spec concepts to line up with reality.

This comment has been minimized.

@rossberg

rossberg Jul 28, 2017

Member

Thinking about this, it probably would make sense if the core spec provided an allochostmem function to delegate to for creating that memory instance, just like it does for functions. (And similarly for tables and globals.) I can add that.

This comment has been minimized.

@lukewagner

lukewagner Jul 28, 2017

Member

Yeah, I was thinking something like that might be necessary. You also have to think about growing and avoiding (at a spec level) leaving "stale" pointers to old buffers around in extant SABs.

This comment has been minimized.

@flagxor

flagxor Aug 12, 2017

Member

@lukewagner Agreed there is a spec race in terms of getting to the other side of serialization, I think I've now fixed that. Additionally (just to make sure we're on the same page), there's an actually race around two parties accessing the buffer (and then later its size). Or said another way: once you've fished out the SAB, it's size stays fixed even if wasm grows the memory.

@rossberg-chromium Adding the op seems to make sense, but trying to clarify here without it for now.

1. If *targetRealm*'s corresponding [agent cluster](https://tc39.github.io/ecma262/#sec-agent-clusters) is not
*serialized*.[[AgentCluster]], then throw a
"[`DataCloneError`](https://heycam.github.io/webidl/#datacloneerror)"
[`DOMException`](https://heycam.github.io/webidl/#dfn-DOMException).

This comment has been minimized.

@lukewagner

lukewagner Jul 27, 2017

Member

@domenic Just to be clear, this means a Module can be post-messaged between some windows (linked by owner and same-origin, same-origin and iframe, and maybe even cross-origin; I'm not sure how to read the could in "could be same-origin" in 8.1.3.10). Is that right?

This comment has been minimized.

@annevk

annevk Jul 28, 2017

Member

Basically windows that can document.domain each other are in the same agent (similar-origin window agent). And therefore also in the same agent cluster.

This comment has been minimized.

@domenic

domenic Jul 28, 2017

Member

Yes, sorry for not getting back to you earlier, but @annevk covered it. Also note that "same origin" and "same origin-domain" are distinct concepts; whether a Window is same origin with another is immutable, but whether it's same origin-domain can change (by changing document.domain). Thus the "could be".

This comment has been minimized.

@lukewagner

lukewagner Jul 28, 2017

Member

Gotcha, thanks. So my root question was whether this definition was permissive enough to allow cross-origin code caching (e.g., in the local IDB of a CDN origin), but it looks like 'no', that'll have to wait for foreign-fetch.

This comment has been minimized.

@flagxor

flagxor Aug 12, 2017

Member

Yep, that matches my understanding too.

This comment has been minimized.

@lukewagner

lukewagner Aug 24, 2017

Member

Actually, it looks like foreign fetch is no longer being proposed. So what's the recommended mechanism for explicit sharing of compiled code that doesn't rely on pure HTTP cache magic? I think this is a really important capability for frameworks/middleware in the future.

This comment has been minimized.

@littledan

littledan Feb 21, 2018

What was the reasoning for restricting Modules from being postMessage'd across agent clusters?

It's hard for me to understand how this squares with serializing Modules in IndexedDB (which this patch seems to allow AFAICT, though I don't see how it specifies the sub-serialization of the agent cluster).

JS.md Outdated
* [[Memory]] : a [`Memory.memory`](https://github.com/WebAssembly/spec/blob/master/interpreter/spec/memory.mli)
* [[BufferObject]] : the current `ArrayBuffer` whose [[ArrayBufferByteLength]]
matches the current byte length of [[Memory]]
* [[WebAssembly.Memory]] : a [`Memory.memory`](https://webassembly.github.io/spec/exec/runtime.html#memory-instances)

This comment has been minimized.

@rossberg

rossberg Jul 28, 2017

Member

s/Memory.memory/memory instance/

This comment has been minimized.

@flagxor

flagxor Aug 12, 2017

Member

done, yeah, that was goofy.

JS.md Outdated
@@ -571,7 +555,7 @@ Return the result of [`CreateMemoryObject`](#creatememoryobject)(`memory`).

### CreateMemoryObject

Given a [`Memory.memory`](https://github.com/WebAssembly/spec/blob/master/interpreter/spec/memory.mli#L1)
Given a [`Memory.memory`](https://webassembly.github.io/spec/exec/runtime.html#memory-instances)

This comment has been minimized.

@rossberg

rossberg Jul 28, 2017

Member

Same here

This comment has been minimized.

@flagxor
JS.md Outdated
@@ -639,7 +623,7 @@ A `WebAssembly.Table` object contains a single [table](Semantics.md#table)
which can be simultaneously referenced by multiple `Instance` objects. Each
`Table` object has two internal slots:

* [[Table]] : a [`Table.table`](https://github.com/WebAssembly/spec/blob/master/interpreter/spec/table.mli#L1)
* [[Table]] : a [`Table.table`](https://webassembly.github.io/spec/exec/runtime.html#table-instances)

This comment has been minimized.

@rossberg

rossberg Jul 28, 2017

Member

s/Table.table/table instance/

This comment has been minimized.

@flagxor
Web.md Outdated
1. *buf*'s internal slot [[ArrayBufferData]]
aliases *mem*'s linear memory.
2. *buf*'s internal slot [[ArrayBufferByteLength]]
is set to the byte length of *mem*'s linear memory.

This comment has been minimized.

@rossberg

rossberg Jul 28, 2017

Member

Thinking about this, it probably would make sense if the core spec provided an allochostmem function to delegate to for creating that memory instance, just like it does for functions. (And similarly for tables and globals.) I can add that.

@flagxor flagxor force-pushed the flagxor:clone1 branch from 10e0424 to 4c24594 Aug 12, 2017

@flagxor flagxor force-pushed the flagxor:clone1 branch from 4c24594 to 6e33a99 Aug 12, 2017

@flagxor

This comment has been minimized.

Member

flagxor commented Aug 12, 2017

PTAL

@domenic

A few more questions and nits, but still seems mergeable from my perspective. I'm not sure I fully understand all the memory instance stuff though.

aliases `M.[[Memory]]` and whose
[[[ArrayBufferByteLength]]](http://tc39.github.io/ecma262/#sec-properties-of-the-arraybuffer-prototype-object)
is set to the new byte length of `M.[[Memory]]`.
If `M.[[BufferObject]]` is a not Shared Data Block:

This comment has been minimized.

@domenic

domenic Aug 14, 2017

Member

Should be M.[[BufferObject]].[[ArrayBufferData]]

This comment has been minimized.

@lukewagner

lukewagner Aug 14, 2017

Member

Also, I think you need to drop the "not"

aliases `M.[[WebAssembly.Memory]]` and whose
[[[ArrayBufferByteLength]]](http://tc39.github.io/ecma262/#sec-properties-of-the-arraybuffer-prototype-object)
is set to the new byte length of `M.[[WebAssembly.Memory]]`.
2. Do the same in all Shared Data Block which mirror `M.[[BufferObject]]`,

This comment has been minimized.

@domenic

domenic Aug 14, 2017

Member

"Blocks", not "Block", I guess

This comment has been minimized.

@lukewagner

lukewagner Aug 14, 2017

Member

I was expecting this to look more like: "Perform step 1 to all WebAssembly.Memory objects N in the same Agent Cluster as M where N.[[BufferObject]].[[ArrayBufferData]] == M.[[BufferObject]].[[ArrayBufferData]]..."

2. Do the same in all Shared Data Block which mirror `M.[[BufferObject]]`,
doing so atomically in relation to other updates of such blocks.
Otherwise:
1. perform [`DetachArrayBuffer`](http://tc39.github.io/ecma262/#sec-detacharraybuffer)(`M.[[BufferObject]]`).

This comment has been minimized.

@domenic

domenic Aug 14, 2017

Member

Uppercase "P"

* Otherwise, if *value* has a [[WebAssembly.Memory]] internal slot, then:
1. Let *memory* be *value*.[[WebAssembly.Memory]], which is a
[`memory instance`](https://webassembly.github.io/spec/exec/runtime.html#memory-instances).
2. Let *data* be *memory*'s `data` field, which is a sizable vector of bytes.

This comment has been minimized.

@domenic

domenic Aug 14, 2017

Member

Is "sizable" standard terminology here? If not, "resizable" is probably more clear, as I usually think of "sizable" as a synonym for "large"

* Otherwise, if *value* has a [[WebAssembly.Memory]] internal slot, then:
1. Let *memory* be *value*.[[WebAssembly.Memory]], which is a
[`memory instance`](https://webassembly.github.io/spec/exec/runtime.html#memory-instances).
2. Let *data* be *memory*'s `data` field, which is a sizable vector of bytes.

This comment has been minimized.

@domenic

domenic Aug 14, 2017

Member

I'm confused. Is data a Shared Data Block, or a sizable vector of bytes? Are the two concepts the same somehow?

is set to the new byte length of `M.[[WebAssembly.Memory]]`.
2. Do the same in all Shared Data Block which mirror `M.[[BufferObject]]`,
doing so atomically in relation to other updates of such blocks.
Otherwise:

This comment has been minimized.

@lukewagner

lukewagner Aug 14, 2017

Member

nit: markdown puts this at the end of item 2, I think you need a \n before or something to render correctly

@@ -571,7 +555,7 @@ Return the result of [`CreateMemoryObject`](#creatememoryobject)(`memory`).

### CreateMemoryObject

Given a [`Memory.memory`](https://github.com/WebAssembly/spec/blob/master/interpreter/spec/memory.mli#L1)
Given a [memory instance](https://webassembly.github.io/spec/exec/runtime.html#memory-instances)
`m`, to create a `WebAssembly.Memory`:

Let `buffer` be a new `ArrayBuffer` whose

This comment has been minimized.

@lukewagner

lukewagner Aug 14, 2017

Member

Should this be changed to create a new SharedArrayBuffer depending on whether m is shared? Or is that a separate proposal?

This comment has been minimized.