Skip to content
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

wasi.start() throws type error #33415

Closed
jtenner opened this issue May 15, 2020 · 5 comments
Closed

wasi.start() throws type error #33415

jtenner opened this issue May 15, 2020 · 5 comments
Labels
wasi Issues and PRs related to the WebAssembly System Interface.

Comments

@jtenner
Copy link

jtenner commented May 15, 2020

  • Version: v14.2.0
  • Platform: windows
  • Subsystem: none

What steps will reproduce the bug?

I am not sure exactly how to reproduce this bug, but when I use jest, I run the following code.

      try {
        if (this.wasi) {
          console.log(this.instance.exports._start);
          console.log(this.instance instanceof WebAssembly.Instance);
          this.wasi.start(this.instance);
          console.log("ran!");
        } else {
          // collect all the top level function pointers, tests, groups, and logs
          this.wasm._start();
        }
      } catch (ex) {
        console.log(ex);
      }
  console.log
    [Function: 62]
      at TestContext.visit (src/test/TestContext.ts:298:19)
  console.log
    true
      at TestContext.visit (src/test/TestContext.ts:299:19)
  console.log
    TypeError [ERR_INVALID_ARG_TYPE]: The "instance" argument must be an WebAssembly.Instance. Received an instance of Instance
        at WASI.start (wasi.js:75:13)

How often does it reproduce? Is there a required condition?

I doubt this is jest related, and it looks like wasi seems to have a different version of WebAssembly.Instance, because it logs true.

Related question: Is it possible that jest would modify the WebAssembly class?

What is the expected behavior?

I suppose that the module shouldn't throw an error, because it appears to be an instance of an Instance.

What do you see instead?

    TypeError [ERR_INVALID_ARG_TYPE]: The "instance" argument must be an WebAssembly.Instance. Received an instance of Instance
        at WASI.start (wasi.js:75:13)
        ... unrelated stack frames

Additional information

I will be happy to help debug this, but it's currently 3am. Will update this with an appropriate script to replicate this without jest.

EDIT: Removed postfix ! operators because they are typescript related. Whoops.

@bnoordhuis
Copy link
Member

Jest runs your code in a VM sandbox (think require('vm').runInNewContext('/* your code */')) and that sandbox has its own copies of the built-in globals.

When your code calls out to the outer context, the instance instanceof WebAssembly.Instance check in lib/wasi.js fails because it's a different instance of, er, Instance.

I think that check and the WebAssembly.Memory check can be removed without ill effects, as long as the argument duck-types as a WebAssembly.Instance object.

Do you want to open a PR? It should come with tests in test/wasi that checks various kinds of valid and invalid inputs.

@bnoordhuis bnoordhuis added the wasi Issues and PRs related to the WebAssembly System Interface. label May 15, 2020
@jtenner
Copy link
Author

jtenner commented May 15, 2020

Glad I wasn't crazy. I had to implement a very hacky workaround for my testing software involving obtaining all the symbols directly and calling the setMemory function directly.

I suppose I could submit a lib/wasi.js fix for this.

Might need some help with making the tests.

@jtenner
Copy link
Author

jtenner commented May 15, 2020

@bnoordhuis , I'm a first time contributer. Is there any chance I could get some help with starting my pull request?

Edit: Actually, my computer has only about 3 gigs worth of space left on it, and compiling all the test modules seems to eat it all up. I'm currently doing most of my dev work on a microsoft surface, so I won't be able to contribute to fix this.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 16, 2020
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: nodejs#33415
@bnoordhuis
Copy link
Member

No problem. I've opened #33431.

@jtenner
Copy link
Author

jtenner commented May 16, 2020

Workaround is not fun, but it's doable.

        wasi.start = jest.fn((instance) => {
          const symbols = Object.getOwnPropertySymbols(wasi);
          const kStartedSymbol = symbols.filter((symbol) =>
            symbol.toString().includes("kStarted"),
          )[0];
          const setMemorySymbol = symbols.filter((symbol) =>
            symbol.toString().includes("setMemory"),
          )[0];
          wasi[setMemorySymbol](instance.exports.memory);
          wasi[kStartedSymbol] = true;
          instance.exports._start();
        });

I hope this helps someone in the meantime.

codebytere pushed a commit that referenced this issue Jun 18, 2020
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: #33415

PR-URL: #33431
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: #33415

PR-URL: #33431
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: #33415

PR-URL: #33431
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this issue Jul 8, 2020
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: #33415

PR-URL: #33431
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants