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

misc/wasm: improve error message when wasm file is not supplied #37000

Open
Gregory-Ledray opened this issue Feb 3, 2020 · 6 comments
Open

misc/wasm: improve error message when wasm file is not supplied #37000

Gregory-Ledray opened this issue Feb 3, 2020 · 6 comments
Assignees

Comments

@Gregory-Ledray
Copy link

@Gregory-Ledray Gregory-Ledray commented Feb 3, 2020

What version of Go are you using (go version)?

$ go version

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

O111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/greg/.cache/go-build"
GOENV="/home/greg/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/greg/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/greg/wasmbrowsertest/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build552182410=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Tried running a WASM binary with wasm_exec.js but didn't properly supply a .wasm file, or perhaps some other error happened. The specifics aren't important - I've seen this error a couple different ways when I did something wrong.

I saw this:

TypeError: Cannot read property 'exports' of undefined
at global.Go.run (http://localhost:49700/wasm_exec.js:439:40)
at http://localhost:49700/:59:13

Which was a bit baffling the first time I read it until I poked around.

I want to see this:
wasm instance was not supplied to go.run
or a similar error message.

You can get this effect by modifying wasm_exec.js a bit:

// existing code before here...
async run(instance) {
    if (instance == null) {
        throw new Error("wasm instance was not supplied to go.run")
    }
// existing code continues...

I believe this is the only public function in wasm_exec.js where someone might conceivably run into an error and have no real understanding of how Javascript works since the other functions in wasm_exec.js seem rather low-level.

@smasher164 smasher164 changed the title Better error message in wasm_exec.js when there's no wasm file passed in wasm: improve error message when wasm file is not supplied Feb 3, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 3, 2020

/cc @neelance

@agnivade
Copy link
Contributor

@agnivade agnivade commented Feb 4, 2020

I think this should be handled in wasmbrowsertest rather than inside the run function. I checked the code and the error paths can be improved to show a better error for cases like these.

If you use the node runner, then you don't get an error like this. So I think it falls on wasmbrowsertest to handle this case.

@Gregory-Ledray
Copy link
Author

@Gregory-Ledray Gregory-Ledray commented Feb 4, 2020

I saw this before I ever used wasmbrowsertest, when I was messing around with index.html to switch it from clicking a 'Run' button to starting on window load / from the instantiatestreaming callback. I'll have to modify it again when I start messing with service workers in earnest.

My point is people who actually use index.html in a real application aren't going to have a run button and will definitely modify index.html's code which calls into wasm_exec.js. Once they do that their index.html is going to start to look nothing like the current index.html. Presumably the plan is for such apps to continue to use wasm_exec.js unmodified to make it easier to take changes from the Go repo. When such apps do so, I think it's reasonable to expect wasm_exec.js to throw exceptions when you use its public APIs incorrectly instead of throwing errors which require devs to read and understand wasm_exec.js's code.

@neelance
Copy link
Member

@neelance neelance commented Feb 4, 2020

Please keep in mind that wasm_exec.js's API is not fully "public" until js/wasm is stable. Right now it is meant for early adopters and can change at any time. However, I agree with you that a better error message can ease this pitfall at almost zero cost. I'll add it in the next release cycle.

@neelance neelance self-assigned this Feb 4, 2020
@agnivade
Copy link
Contributor

@agnivade agnivade commented Feb 4, 2020

Once they do that their index.html is going to start to look nothing like the current index.html.

Ah yes, that's a good point.

@odeke-em odeke-em changed the title wasm: improve error message when wasm file is not supplied misc/wasm: improve error message when wasm file is not supplied Feb 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2020

Change https://golang.org/cl/266117 mentions this issue: misc/wasm: check type of argument to Go.run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.