Tested on Version 66.0.3339.0 (Official Build) canary (64-bit)

letz@

Could you provide a repro case for this issue? That'll help me to triage/fix it.

I am cc-ing WebAssembly folks to get some help on this.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webaudio/AudioWorkletNode.cpp?q=AudioWorkletNode&sq=package:chromium&dr=CSs&l=319

This is where the constructor option is being passed to the worklet thread. Do we need a special switch to pass a WASM instance via SerializedScriptValue?

Here is a (bit manually..) hacked example.

Deleted: clarinetMIDI-processor.js 52.8 KB

clarinetMIDI-processor.js 52.8 KB View Download

Deleted: clarinetMIDI.html 335 KB

clarinetMIDI.html 335 KB View Download

Had a chat with letz@ about how cloning on WASM module/instance works. To summarize:

- Module is thread/scope-agnostic. Thus the compilation can happen anywhere and 
  be passed to the other scope.
- Instance is thread/scope-specific. It has a strong tie with V8 so the instantiation
  should happen in the scope where it is actually used.

In short, we have to pass the module instead of the instance.

Also note:
AFAIK, new WebAssmebly.instance() does not have the size limit of the module when it is used in a worker/worklet thread.

Same issue with the was module, I get:

DOMException: Failed to construct 'AudioWorkletNode': #<Module> could not be cloned.
    at new clarinetMIDINode (http://127.0.0.1:8000/clarinetMIDI.html:5445:9)
    at http://127.0.0.1:8000/clarinetMIDI.html:5661:23
clarinetMIDI-processor.js:160 constructor

Attached files.

Deleted: clarinetMIDI-processor.js 52.9 KB

clarinetMIDI-processor.js 52.9 KB View Download

Deleted: clarinetMIDI.html 335 KB

clarinetMIDI.html 335 KB View Download

letz@

Unfortunately, it turned out that structure cloning of WASM module is behind the flag:
chrome://flags/#enable-webassembly
(or use --enable-features=WebAssembly for the command line)

Relevant spec discussion:
https://github.com/WebAssembly/design/issues/972

This feature is currently disabled by:
https://bugs.chromium.org/p/chromium/issues/detail?id=686457

Corrected processor code

Deleted: clarinetMIDI-processor.js 52.9 KB

clarinetMIDI-processor.js 52.9 KB View Download

letz@

Can you try with the latest canary? (66.0.3356.0) I saw some reports that now MessagePort can pass the WASM module and the SharedArrayBuffer correctly.

The thing is that the MessagePort can only be setup in the AudioWorkletProcessor constructor, and messages can be retrieved in an asynchronous manner (by setting an "handleMessage" callback). So WASM module cannot be handled in the constructor itself ? 

So the chain would have to be:

FaustAudioWorkletProcessor::constructor ==> setup handleMessage

FaustAudioWorkletProcessor::process ==> would have to produce some kind of null cycles since the WASM module is still not there

FaustAudioWorkletProcessor::handleMessage ==> be notified with the WASM module, create the WAMS instance, setup everything needed, then FaustAudioWorkletProcessor::process could be run normally?

Well this is quite complex, being able to do everything in a synchronous manner in the constructor would be *much* preferred.

Why can the WASM module be cloned in a MessagePort, and not in the constructor "option" parameter as discussed before?

So you're still experiencing the same issue as before?

Passing `option` argument is implemented, but the special treatment for WASM module has been added recently (by WASM team). I was asking if that recent change made any difference for you but it doesn't seem to be the case.

Then I need to investigate why that fix doesn't work for AudioWorklet.

You were actually speaking about MessagePort, not `option` argument !?, so I'm confused..

Anyway I tested again:

- the WASM module is indeed transferred from Node to Worklet side

- the WASM instance can be created with WebAssembly.Instance function

- the WASM instance can somewhat be used, some exported functions can be called...

- but then keeping ii as a field of the Worklet object, to be used later one in the code, still crash.

Attached a modified clarinetMIDI-processor.js file with comments.

Deleted: clarinetMIDI-processor.js 53.6 KB

clarinetMIDI-processor.js 53.6 KB View Download

letz@

Just tested Version 67.0.3364.0 (MacOS) with the JS file in #19 and the HTML file in #7. I don't see any crash, but also I can't hear anything from the instrument when I press my MIDI keyboard.

FYI, all the console.log between l.212~l.214 prints out data without error.

Can you please test it?

By the way, I had to use `--enable-features=WebAssembly`flag to pass the WASM module correctly. I don't think the feature is enabled by default.

Same crash with 67.0.3364.0.

Version of clarinetMIDI-processor.js  that cause the crash, see the commented lines (<=== WHEN THIS LINE IN ON, IT CRASHES)

Deleted: clarinetMIDI-processor.js 53.7 KB

clarinetMIDI-processor.js 53.7 KB View Download

letz@

I could reproduce the crash with the example. Will investigate it further! I think this is related to the WASM module serialization.

Updates:

- With the deserialization option for reading WASM module from the byte stream, I actually get `null` for the entire option data.
- Without that option, the build crashes when JS code accesses the passed WASM module.

The build still needs `--enable-features=WebAssembly` for this to happen.

I started looking into this -- there's something strange going on. The module is properly serialized, and some functions can be called successfully (as noted in #23), but it seems to always fail in the wasm compute() function.

I spent some time looking at the generated code for this function, but couldn't figure out what is causing it to fail. The crash doesn't actually occur in that function's generated code, but somewhere else (a builtin perhaps?) I thought it might be Math.exp, which is the only imported function called by compute, but replacing that import w/ another function didn't seem to fix it.

I'll take a closer look again tomorrow.

OK, looked closer at this. Looks like it is crashing in the RecordWrite builtin, but it never should have gotten there, I think.

The real issue is that compute has a large enough call frame that it generates a stack overflow check (see https://cs.chromium.org/chromium/src/v8/src/compiler/x64/code-generator-x64.cc?l=3147).

```
   0x71666f92040:	push   %rbp
   0x71666f92041:	mov    %rsp,%rbp
   0x71666f92044:	pushq  $0xa
   0x71666f92046:	movabs $0x2aea85595c68,%r10
   0x71666f92050:	mov    (%r10),%r10
   0x71666f92053:	add    $0x740,%r10
   0x71666f9205a:	cmp    %r10,%rsp
   0x71666f9205d:	jae    0x71666f92086
```


The `address_of_real_stack_limit` seems to always be incorrect (not sure why). 0x2aea85595c68 has 0x7fffaae06b00, but rsp at that point is 0x7fff93605420.

So it falls through to the ThrowWasmStackOverflow delayed call.

```
   0x71666f92063:	xor    %esi,%esi
   0x71666f92065:	xor    %eax,%eax
   0x71666f92067:	movabs $0x7fffe58f5da0,%rbx => v8::internal::Runtime_ThrowWasmStackOverflow
   0x71666f92071:	callq  0x71666f95e40
... (trampoline)
   0x71666f95e40:	movabs $0x2118e6115dc0,%r10
   0x71666f95e4a:	jmpq   *%r10
```

But this seems to be the wrong trampoline, as `rbx` is ignored and 0x2118e6115dc0 is called instead, which is `RecordWrite`.

The crash occurs because RecordWrite accesses `rsi`, which is 0.

```
   0x2118e6115dc0:	push   %rbp
   0x2118e6115dc1:	mov    %rsp,%rbp
   0x2118e6115dc4:	pushq  $0x18
   0x2118e6115dc6:	sub    $0x28,%rsp
   0x2118e6115dca:	mov    %rdx,-0x20(%rbp)
=> 0x2118e6115dce:	mov    (%rsi),%rdx
```

I don't really understand enough about the code generation to understand which part is going wrong, though.

@titzer?

Note that the same wasm module perfectly works when the wasm code is loaded, compiled and executed on AudioWorkletProcessor side. The crash occurs when using the "transfer arbitrary data between AudioWorkletNode and AudioWorkletProcessor" model. Is there any change in wasm to machine code generation in this case?

Note that CallRuntimeDelayed loads the address of the runtime function into rbx and then calls the CEntryStub: https://cs.chromium.org/chromium/src/v8/src/x64/macro-assembler-x64.cc?l=483

So if we accidentally call RecordWrite instead of CEntryStub, it looks like code patching / relocation after deserialization went wrong.
I am on vacation till 3rd of April, so cannot help debugging further. Ping me if it's still relevant in April.

binji@

Thanks for looking into this. Is the related CL[1] still necessary to make this work?

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/905086

letz@

Let us clarify the situation:

1. This crash only happens when you pass WASM module via AudioWorkletNode constructor.
2. You do not have this crash when you're using MessagePort to do the same thing.

Is this correct?

1. This crash only happens when you pass WASM module via AudioWorkletNode constructor.

==> It happens there yes.

2. You do not have this crash when you're using MessagePort to do the same thing.

==> I've never tried that, but it is much less easy to use, see Comment #17 

It's rather about pinpointing a problem than about "usability".

I am assuming the internal transfer mechanism of MessagePort is identical to passing the WASM module via the constructor, but if 1) does not work while 2) works then it is clearly a problem of the Serialize/Deserialize functions.

I think there are internal issues with WebAssembly module serialization that are keeping this from working. I don't think MessagePort will make a difference, as the internal serialization is the same in both cases.

I'm still looking into it, but here is some more info:

`address_of_real_stack_limit` is wrong because it is thread local and is not relocated, so the comparison is always done against the stack limit of the initial thread.

The `ThrowWasmStackOverflow` trampoline is incorrect, but I'm not quite sure why. When I stepped through the deserialization code, the relocation seemed to be skipped so the proper trampoline was not generated. Though I may be misunderstanding something.

hongchan@ comment 31:

I haven't applied your CL, this is just running on a pretty recent Chrome and passing `--enable-features=WebAssembly`

See partial fix here: https://chromium-review.googlesource.com/c/v8/v8/+/981687

This prevents the crash, but still incorrectly throws a stack overflow exception.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fae1ab03a93834f1c7cf388b0c366bbc415cda32

commit fae1ab03a93834f1c7cf388b0c366bbc415cda32
Author: Ben Smith <binji@chromium.org>
Date: Tue Mar 27 18:34:06 2018

[wasm] Fix crash serializing modules w/ big frames

When a wasm function has a large stack frame, the x64 code generator
performs the stack overflow check before constructing the frame. This
requires using the `address_of_real_stack_limit` external reference, as
well as the `ThrowWasmStackOverflow` runtime function.

`ThrowWasmStackOverflow` is called via a generated trampoline, but it is
not a builtin, so the serializer adds it to the `stub_lookup_` map. This
map is encoded by using a monotonically increasing `stub_id` that starts
at 0.

When the function is serialized, a stub is differentiated from a builtin
by which half of the `i32` bits is used, upper or lower. A stub only
uses the lower 16 bits and a builtin only uses the upper 16 bits.

The deserializer checks whether the lower 16 bits are 0; if so, it is
determined to be a builtin. But if the `stub_id` is 0, then it will be
confused with builtin 0 (`RecordWrite`). Calling the builtin instead of
the stub causes a crash.

This CL starts all `stub_id`s at 1, which prevents the builtin/stub
confusion.

There is an additional bug that is not fixed by this CL:
`ThrowWasmStackOverflow` shouldn't be called at all. Currently it is
called because `address_of_real_stack_limit` is a thread-local value
that is not properly relocated.

Bug:  chromium:808848 
Change-Id: I06b3e650ea58ad717dcc47a3716443e16582e711
Reviewed-on: https://chromium-review.googlesource.com/981687
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Ben Smith <binji@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52252}
[modify] https://crrev.com/fae1ab03a93834f1c7cf388b0c366bbc415cda32/src/wasm/wasm-serialization.cc
[add] https://crrev.com/fae1ab03a93834f1c7cf388b0c366bbc415cda32/test/mjsunit/regress/wasm/regress-808848.js

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fae8a588e8c58eb8e525fddba497b7cc770cf40b

commit fae8a588e8c58eb8e525fddba497b7cc770cf40b
Author: Ben Smith <binji@chromium.org>
Date: Tue Apr 03 17:42:45 2018

[wasm] Reloc external refs when deserializing

This is a follow-up to https://chromium-review.googlesource.com/981687.

When a wasm function has a large stack frame, the x64 code generator
performs the stack overflow check before constructing the frame. This
requires the use of the `address_of_real_stack_limit` external
reference.

This reference is thread local, so if it is not relocated the stack
overflow check will always fail.

Bug:  chromium:808848 
Change-Id: I0edf3fe5a006242fc50d0bff44cd9dd0e7d85bd9
Reviewed-on: https://chromium-review.googlesource.com/982906
Commit-Queue: Ben Smith <binji@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52330}
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/arm/assembler-arm-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/arm64/assembler-arm64-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/assembler.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/ia32/assembler-ia32-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/mips/assembler-mips-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/mips64/assembler-mips64-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/ppc/assembler-ppc-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/s390/assembler-s390-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/wasm/wasm-serialization.cc
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/src/x64/assembler-x64-inl.h
[modify] https://crrev.com/fae8a588e8c58eb8e525fddba497b7cc770cf40b/test/mjsunit/regress/wasm/regress-808848.js

OK, with these two v8 CLs landed, the crash bug should be fixed (with the flag enabled) once they roll into Chromium.

I have tested the repro case with the Canary 67.0.3389.0 with V8 6.7.222.0 (#52354):
`--enable-features=WebAssembly` flag did not work, but chrome://flag#enable-webassembly worked without any crash.

Thanks for your fix, binji@ :D

letz@

Could you re-test your page with the version above?

Works here also.

I'll be able to greatly simplify all this wasm code, thanks!

Fixed and verified. Waiting for this feature to be release off the flag.