Panic in boundStringListItem.Get()

[macbutch]

Describe the bug:

I believe this to be a race condition because it is not reproducible every time (sometimes it is apparently easy and sometimes it disappears completely). In my application I show the results of some processing in a list. I have a bound list created with binding.NewStringList().

When processing starts I reset it like this: dataList.Set([]string{}). Then I update it in the same way during processing. Items are added and removed during processing so I update the list like this:dataList.Set(sortedKeys).

Sometimes this panics like so (other indices are possible):

panic: runtime error: index out of range [7] with length 7

goroutine 57 [running]:
fyne.io/fyne/v2/data/binding.(*boundStringListItem).Get(0xc002762340, 0x0, 0x0, 0x0, 0x0)
	/Users/mark/go/pkg/mod/fyne.io/fyne/v2@v2.1.1/data/binding/bindlists.go:1039 +0xc9
fyne.io/fyne/v2/widget.(*Label).createListener.func1()
	/Users/mark/go/pkg/mod/fyne.io/fyne/v2@v2.1.1/widget/label.go:145 +0x48
fyne.io/fyne/v2/data/binding.(*listener).DataChanged(0xc0000b4088)
	/Users/mark/go/pkg/mod/fyne.io/fyne/v2@v2.1.1/data/binding/binding.go:55 +0x28
fyne.io/fyne/v2/data/binding.queueItem.func1.1()
	/Users/mark/go/pkg/mod/fyne.io/fyne/v2@v2.1.1/data/binding/queue.go:19 +0x4b
created by fyne.io/fyne/v2/data/binding.queueItem.func1
	/Users/mark/go/pkg/mod/fyne.io/fyne/v2@v2.1.1/data/binding/queue.go:17 +0x51

To Reproduce:

This is hard to reproduce reliably; it's almost certainly down to timing. Could be user error?

Example code:

Call this repeatedly:

someBoundStringArray.Set(someStringArray)

Very rarely you will get a panic (see above) which points to this code in bindlists.go:

func (b *boundStringListItem) Get() (string, error) {
	b.lock.Lock()
	defer b.lock.Unlock()

	return (*b.val)[b.index], nil // <--- panic here!
}

Adding the following appears results in a large number of errors logged to the console (but no panic):

func (b *boundStringListItem) Get() (string, error) {
	b.lock.Lock()
	defer b.lock.Unlock()

	if b.index < 0 || b.index >= len(*b.val) {
		return "", errOutOfBounds
	}

	return (*b.val)[b.index], nil
}

I don't think this is a good fix (probably there should be a check earlier so that we're not attempting to fetch items that have been removed) but this does seem to address the immediate problem.

Device:

• OS: MacOS
• Version: Mojava 10.14.6
• Go version: go1.16.6 darwin/amd64
• Fyne version: 2.1.1

[macbutch]

I believe there are other cases which aren't quite right. For example, here we should check the bounds after getting the lock:

func (l *boundFloatList) GetValue(i int) (float64, error) {
	if i < 0 || i >= l.Length() {
		return 0.0, errOutOfBounds
	}
	l.lock.RLock()
	defer l.lock.RUnlock()

	return (*l.val)[i], nil
}

If the underlying data changes between the bounds check and acquiring the lock then it could still be invalid (and panic).

[andydotxyz]

Fixed on release/v2.1.x

[macbutch]

Thanks @andydotxyz