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

x/mobile: proposal to support passing null to Go #20330

Closed
karalabe opened this issue May 11, 2017 · 2 comments
Closed

x/mobile: proposal to support passing null to Go #20330

karalabe opened this issue May 11, 2017 · 2 comments

Comments

@karalabe
Copy link
Contributor

karalabe commented May 11, 2017

Currently if an Android app tries to call a gomobile bounded method with a null parameter, that will hard panic inside the proxy classes. I assume the same holds true for iOS too.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x7ca427a078]

goroutine 17 [running, locked to thread]:
golang.org/x/mobile/bind/seq.(*Ref).Get(0x0, 0x0, 0x4420318068)
  /work/src/golang.org/x/mobile/bind/seq/ref.go:113 +0x18
[...]

Looking at the code, it's fairly obvious what happens: gomobile assumes that any object passed to it actually originates from inside Go, and is a proxy/wrapper object. This makes perfect sense, since it's meaningless to pass a Java object to Go directly as it doesn't know what to do with it.

Edit: Opened a CL with the proper code to support this.

That being said, I think null warrants a special behavior whereby func (r *Ref) Get() interface{} { would simply short circuit execution in such cases. I'm unsure if this is the only change needed or not (can't test it right now). But if yes, it would be an elegant way to support crossing null over as nil from Java to Go.

The rationale behind supporting this is that in Go, code often allows setting some parameters to nil to use default values (simply so that the caller doesn't have to care what those defaults are). However this "don't care" feature cannot be done, so the package needs to surface an extra constant which essentially acts as the "default/nil" placeholder.

@gopherbot gopherbot added this to the Unreleased milestone May 11, 2017
@gopherbot
Copy link

CL https://golang.org/cl/43253 mentions this issue.

@rsc
Copy link
Contributor

rsc commented May 15, 2017

/cc @hakim @crawshaw

@golang golang locked and limited conversation to collaborators May 24, 2018
imWildCat pushed a commit to imWildCat/go-mobile that referenced this issue Apr 10, 2021
Currently the generated bindings assume that any object
passed to Go as a method argument is actually a valid one
originating from Go. The `null` object is however a corner
case to this assumption, which should be accepted for Go
pointer types, since they can cleanly convert into `nil`.

This CL modifies the generated wrapper code so any `nil`
reference is permitted for Go pointer types, which until
now produced a nil pointer dereference error.

Fixes golang/go#20330

Change-Id: If1ab9cf9df7ac3808486d23ccf2db8d32fb89426
Reviewed-on: https://go-review.googlesource.com/43253
Reviewed-by: Elias Naur <elias.naur@gmail.com>
imWildCat pushed a commit to imWildCat/go-mobile that referenced this issue Apr 11, 2021
Currently the generated bindings assume that any object
passed to Go as a method argument is actually a valid one
originating from Go. The `null` object is however a corner
case to this assumption, which should be accepted for Go
pointer types, since they can cleanly convert into `nil`.

This CL modifies the generated wrapper code so any `nil`
reference is permitted for Go pointer types, which until
now produced a nil pointer dereference error.

Fixes golang/go#20330

Change-Id: If1ab9cf9df7ac3808486d23ccf2db8d32fb89426
Reviewed-on: https://go-review.googlesource.com/43253
Reviewed-by: Elias Naur <elias.naur@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants