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

Fix crash in label with binding #2131

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Conversation

andydotxyz
Copy link
Member

Moved to a different setup model where we don't recreate listeners all the time.
Fixes #2125

If this is a desirable change we could move other Bind/Unbind widget to match.

Checklist:

  • Tests included. <- race
  • Lint and formatter run with no errors.
  • Tests all pass.

Moved to a different setup model where we don't recreate listeners all the time.
Fixes fyne-io#2125
@andydotxyz andydotxyz requested a review from fpabl0 April 2, 2021 18:08
Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I will try to explore the connected flag in other PR to see if it is viable :)

@fpabl0
Copy link
Member

fpabl0 commented Apr 2, 2021

It seems that there is something more we have to do here, sometimes I got an error inside convert.go:

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

goroutine 8 [running]:
fyne.io/fyne/v2/data/binding.(*stringFromFloat).Get(0x0, 0xc000048f78, 0x42545fa, 0xc0003bcdd8, 0x40638e0)
        .../fyne/data/binding/convert.go:109 +0x26
fyne.io/fyne/v2/widget.(*Label).createListener.func1()
        .../fyne/widget/label.go:71 +0x45
fyne.io/fyne/v2/data/binding.(*listener).DataChanged(0xc00018e9c0)
        .../fyne/data/binding/binding.go:63 +0x2e
fyne.io/fyne/v2/data/binding.processItems()
        .../fyne/data/binding/queue.go:56 +0x94
created by fyne.io/fyne/v2/data/binding.init.0
        .../fyne/data/binding/queue.go:15 +0x35
exit status 2

@fpabl0 fpabl0 self-requested a review April 2, 2021 20:37
@andydotxyz
Copy link
Member Author

It seems that there is something more we have to do here, sometimes I got an error inside convert.go:

Those line numbers don't mat this PR... do you have local changes?

@fpabl0
Copy link
Member

fpabl0 commented Apr 2, 2021

Yes, but they are commented. I copied your changes and paste in a local branch. I will try to reproduce it, by cloning directly your branch, although I think it would be the same 🤔. I will investigate. However it seems that this PR is ok and this can be solved in a different one?

@fpabl0
Copy link
Member

fpabl0 commented Apr 2, 2021

Ok, I have tested your branch without any change, this is the error trace:

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

goroutine 8 [running]:
fyne.io/fyne/v2/data/binding.(*stringFromFloat).Get(0x0, 0xc0002b0508, 0xc00009a470, 0xc0002b05b8, 0x40643a0)
        /Users/pablofuentes/Desktop/fyne/data/binding/convert.go:109 +0x26
fyne.io/fyne/v2/widget.(*Label).createListener.func1()
        /Users/pablofuentes/Desktop/fyne/widget/label.go:118 +0x45
fyne.io/fyne/v2/data/binding.(*listener).DataChanged(0xc000338008)
        /Users/pablofuentes/Desktop/fyne/data/binding/binding.go:55 +0x28
fyne.io/fyne/v2/data/binding.processItems()
        /Users/pablofuentes/Desktop/fyne/data/binding/queue.go:56 +0x94
created by fyne.io/fyne/v2/data/binding.init.0
        /Users/pablofuentes/Desktop/fyne/data/binding/queue.go:15 +0x35

@fpabl0
Copy link
Member

fpabl0 commented Apr 2, 2021

This is really weird, it seems that there is a chance that s could be nil here:

func (s *stringFromFloat) Get() (string, error) {
	val, err := s.from.Get()
	if err != nil {
		return "", err
	}

	return fmt.Sprintf(s.format, val), nil
}

I added a fmt.Printf("%p\n", s) just before the s.from.Get(), and when the crash occurs I get 0x0.

Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found something interesting that I didn't know before (probably it was logical but I hadn't realized it). Nil objects can call its methods without actually crashing the whole program:
https://play.golang.org/p/tOzpRaZTt3m

widget/label.go Outdated Show resolved Hide resolved
Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change should be done on Unbind method.

widget/label.go Show resolved Hide resolved
@andydotxyz andydotxyz requested a review from fpabl0 April 7, 2021 09:48
@andydotxyz andydotxyz merged commit 728302f into fyne-io:develop Apr 7, 2021
@andydotxyz andydotxyz deleted the fix/2125 branch April 7, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants